From 89a5a60da623ca9e7f91a93cd34b35785e30ab7e Mon Sep 17 00:00:00 2001 From: =?utf8?q?Felix=20Geisend=C3=B6rfer?= Date: Tue, 27 Aug 2024 20:46:33 +0200 Subject: [PATCH] internal/trace: refactor Stack.Frames to return iter.Seq The Frames function is almost an iter.Seq, except for its bool return value. Since none of the callers in the Go tree rely on the bool, we can remove it. However, doing so might still obscure the intended usage as an iterator. This refactor changes the API to return iter.Seq, making the intended usage explicit. Refactoring the existing callers to take advantage of the new interface will be done in a follow-up CL. Change-Id: I03e4d6d762910e418cc37d59a6c519eb7f39b3b0 Reviewed-on: https://go-review.googlesource.com/c/go/+/608855 LUCI-TryBot-Result: Go LUCI Reviewed-by: Dmitri Shuralyov Reviewed-by: Michael Knyszek --- src/cmd/trace/gstate.go | 2 +- src/cmd/trace/pprof.go | 4 +-- src/cmd/trace/regions.go | 2 +- src/cmd/trace/viewer.go | 2 +- src/internal/trace/event.go | 36 ++++++++++++---------- src/internal/trace/summary.go | 2 +- src/internal/trace/testtrace/validation.go | 2 +- src/internal/trace/trace_test.go | 4 +-- src/runtime/trace_cgo_test.go | 2 +- 9 files changed, 29 insertions(+), 27 deletions(-) diff --git a/src/cmd/trace/gstate.go b/src/cmd/trace/gstate.go index 638d492670..76c58073b3 100644 --- a/src/cmd/trace/gstate.go +++ b/src/cmd/trace/gstate.go @@ -364,7 +364,7 @@ func (gs *gState[R]) rangeEnd(ts trace.Time, name string, stack trace.Stack, ctx func lastFunc(s trace.Stack) string { var last trace.StackFrame - s.Frames(func(f trace.StackFrame) bool { + s.Frames()(func(f trace.StackFrame) bool { last = f return true }) diff --git a/src/cmd/trace/pprof.go b/src/cmd/trace/pprof.go index c3e5a3a045..856b97b75f 100644 --- a/src/cmd/trace/pprof.go +++ b/src/cmd/trace/pprof.go @@ -307,7 +307,7 @@ func (m *stackMap) profile() []traceviewer.ProfileRecord { for stack, record := range m.stacks { rec := *record i := 0 - stack.Frames(func(frame trace.StackFrame) bool { + stack.Frames()(func(frame trace.StackFrame) bool { rec.Stack = append(rec.Stack, &trace.Frame{ PC: frame.PC, Fn: frame.Func, @@ -327,7 +327,7 @@ func (m *stackMap) profile() []traceviewer.ProfileRecord { // pcsForStack extracts the first pprofMaxStack PCs from stack into pcs. func pcsForStack(stack trace.Stack, pcs *[pprofMaxStack]uint64) { i := 0 - stack.Frames(func(frame trace.StackFrame) bool { + stack.Frames()(func(frame trace.StackFrame) bool { pcs[i] = frame.PC i++ return i < len(pcs) diff --git a/src/cmd/trace/regions.go b/src/cmd/trace/regions.go index cb04190fd8..cae38355a5 100644 --- a/src/cmd/trace/regions.go +++ b/src/cmd/trace/regions.go @@ -72,7 +72,7 @@ func fingerprintRegion(r *trace.UserRegionSummary) regionFingerprint { func regionTopStackFrame(r *trace.UserRegionSummary) trace.StackFrame { var frame trace.StackFrame if r.Start != nil && r.Start.Stack() != trace.NoStack { - r.Start.Stack().Frames(func(f trace.StackFrame) bool { + r.Start.Stack().Frames()(func(f trace.StackFrame) bool { frame = f return false }) diff --git a/src/cmd/trace/viewer.go b/src/cmd/trace/viewer.go index 79c9583b0d..c367c7d636 100644 --- a/src/cmd/trace/viewer.go +++ b/src/cmd/trace/viewer.go @@ -15,7 +15,7 @@ import ( // used to store the frames to reduce allocations. func viewerFrames(stk trace.Stack) []*trace.Frame { var frames []*trace.Frame - stk.Frames(func(f trace.StackFrame) bool { + stk.Frames()(func(f trace.StackFrame) bool { frames = append(frames, &trace.Frame{ PC: f.PC, Fn: f.Func, diff --git a/src/internal/trace/event.go b/src/internal/trace/event.go index 7d869e885f..90effce653 100644 --- a/src/internal/trace/event.go +++ b/src/internal/trace/event.go @@ -6,6 +6,7 @@ package trace import ( "fmt" + "iter" "math" "strings" "time" @@ -265,24 +266,25 @@ type Stack struct { } // Frames is an iterator over the frames in a Stack. -func (s Stack) Frames(yield func(f StackFrame) bool) bool { - if s.id == 0 { - return true - } - stk := s.table.stacks.mustGet(s.id) - for _, pc := range stk.pcs { - f := s.table.pcs[pc] - sf := StackFrame{ - PC: f.pc, - Func: s.table.strings.mustGet(f.funcID), - File: s.table.strings.mustGet(f.fileID), - Line: f.line, +func (s Stack) Frames() iter.Seq[StackFrame] { + return func(yield func(StackFrame) bool) { + if s.id == 0 { + return } - if !yield(sf) { - return false + stk := s.table.stacks.mustGet(s.id) + for _, pc := range stk.pcs { + f := s.table.pcs[pc] + sf := StackFrame{ + PC: f.pc, + Func: s.table.strings.mustGet(f.funcID), + File: s.table.strings.mustGet(f.fileID), + Line: f.line, + } + if !yield(sf) { + return + } } } - return true } // NoStack is a sentinel value that can be compared against any Stack value, indicating @@ -796,7 +798,7 @@ func (e Event) String() string { if s.Stack != NoStack { fmt.Fprintln(&sb) fmt.Fprintln(&sb, "TransitionStack=") - s.Stack.Frames(func(f StackFrame) bool { + s.Stack.Frames()(func(f StackFrame) bool { fmt.Fprintf(&sb, "\t%s @ 0x%x\n", f.Func, f.PC) fmt.Fprintf(&sb, "\t\t%s:%d\n", f.File, f.Line) return true @@ -809,7 +811,7 @@ func (e Event) String() string { if stk := e.Stack(); stk != NoStack { fmt.Fprintln(&sb) fmt.Fprintln(&sb, "Stack=") - stk.Frames(func(f StackFrame) bool { + stk.Frames()(func(f StackFrame) bool { fmt.Fprintf(&sb, "\t%s @ 0x%x\n", f.Func, f.PC) fmt.Fprintf(&sb, "\t\t%s:%d\n", f.File, f.Line) return true diff --git a/src/internal/trace/summary.go b/src/internal/trace/summary.go index fa3e3359c7..1cd506ac5a 100644 --- a/src/internal/trace/summary.go +++ b/src/internal/trace/summary.go @@ -394,7 +394,7 @@ func (s *Summarizer) Event(ev *Event) { if stk != NoStack { var frame StackFrame var ok bool - stk.Frames(func(f StackFrame) bool { + stk.Frames()(func(f StackFrame) bool { frame = f ok = true return true diff --git a/src/internal/trace/testtrace/validation.go b/src/internal/trace/testtrace/validation.go index ec492110e2..42a561f1f0 100644 --- a/src/internal/trace/testtrace/validation.go +++ b/src/internal/trace/testtrace/validation.go @@ -351,7 +351,7 @@ func (v *Validator) getOrCreateThread(e *errAccumulator, ev trace.Event, m trace func checkStack(e *errAccumulator, stk trace.Stack) { // Check for non-empty values, but we also check for crashes due to incorrect validation. i := 0 - stk.Frames(func(f trace.StackFrame) bool { + stk.Frames()(func(f trace.StackFrame) bool { if i == 0 { // Allow for one fully zero stack. // diff --git a/src/internal/trace/trace_test.go b/src/internal/trace/trace_test.go index 1929069cc5..dcf9d05fb4 100644 --- a/src/internal/trace/trace_test.go +++ b/src/internal/trace/trace_test.go @@ -148,7 +148,7 @@ func TestTraceCPUProfile(t *testing.T) { if hogRegion != nil && ev.Goroutine() == hogRegion.Goroutine() { traceSamples++ var fns []string - ev.Stack().Frames(func(frame trace.StackFrame) bool { + ev.Stack().Frames()(func(frame trace.StackFrame) bool { if frame.Func != "runtime.goexit" { fns = append(fns, fmt.Sprintf("%s:%d", frame.Func, frame.Line)) } @@ -438,7 +438,7 @@ func TestTraceStacks(t *testing.T) { stackMatches := func(stk trace.Stack, frames []frame) bool { i := 0 match := true - stk.Frames(func(f trace.StackFrame) bool { + stk.Frames()(func(f trace.StackFrame) bool { if f.Func != frames[i].fn { match = false return false diff --git a/src/runtime/trace_cgo_test.go b/src/runtime/trace_cgo_test.go index f0db3b7ffb..28b6f7833e 100644 --- a/src/runtime/trace_cgo_test.go +++ b/src/runtime/trace_cgo_test.go @@ -94,7 +94,7 @@ func mustFindLogV2(t *testing.T, trc io.Reader, category string) trace.Event { // dumpStack returns e.Stack() as a string. func dumpStackV2(e *trace.Event) string { var buf bytes.Buffer - e.Stack().Frames(func(f trace.StackFrame) bool { + e.Stack().Frames()(func(f trace.StackFrame) bool { file := strings.TrimPrefix(f.File, runtime.GOROOT()) fmt.Fprintf(&buf, "%s\n\t%s:%d\n", f.Func, file, f.Line) return true -- 2.48.1