From 77dd532de16074de3f85056f1165f3e6a141d9d7 Mon Sep 17 00:00:00 2001 From: Lexer747 Date: Mon, 8 Sep 2025 22:09:12 +0100 Subject: [PATCH] Remove Internal Ticker | Simplify presentation (#32) Removes the framerate ticker, I don't think this is necessary anymore. Cleans up some now removed flags related to this ticker. Removes the mutex around `presentation` and switched to just an `atomic.Of`, since this component doesn't have wider concurrency context and simply just needed a way to not tear this primitive is much cleaner implementation. Fixes some tab_completion tests. --- cmd/subcommands/acci-ping/acci-ping.go | 2 - cmd/subcommands/acci-ping/application.go | 8 +-- cmd/tab_completion/tab_completion_test.go | 57 ++++++++++--------- cmd/tab_completion/tabflags/tabflags.go | 1 + draw/draw.go | 3 - graph/drawing.go | 15 +---- graph/export_test.go | 2 +- graph/graph.go | 68 ++++++++++------------- graph/graphdata/graphdata.go | 7 --- 9 files changed, 69 insertions(+), 94 deletions(-) diff --git a/cmd/subcommands/acci-ping/acci-ping.go b/cmd/subcommands/acci-ping/acci-ping.go index 9747617..146ca61 100644 --- a/cmd/subcommands/acci-ping/acci-ping.go +++ b/cmd/subcommands/acci-ping/acci-ping.go @@ -27,7 +27,6 @@ type Config struct { *application.SharedFlags *tabflags.FlagSet - debugFps *int debuggingTermSize *string filePath *string followingOnStart *bool @@ -69,7 +68,6 @@ func GetFlags(info *application.BuildInfo) *Config { debuggingTermSize: tf.String("debug-term-size", "", "switches the terminal to fixed mode and no iteractivity", tabflags.AutoComplete{Choices: []string{"15x80", "20x85", "HxW"}}), followingOnStart: tf.Bool("follow", false, "if this flag is used the graph will be shown in following mode immediately"), - debugFps: tf.Int("debug-fps", 240, "configures the internal tickrate for the graph re-paint look (in FPS)"), logarithmicOnStart: tf.Bool("logarithmic", false, "if this flag is used the graph will be shown in logarithmic mode immediately"), } *ret.pingBufferingLimit = 10 diff --git a/cmd/subcommands/acci-ping/application.go b/cmd/subcommands/acci-ping/application.go index 6f20e00..a592b8a 100644 --- a/cmd/subcommands/acci-ping/application.go +++ b/cmd/subcommands/acci-ping/application.go @@ -37,10 +37,8 @@ type Application struct { g *graph.Graph term *terminal.Terminal - toUpdate *os.File - config Config - // this doesn't need a mutex because we ensure that no two threads have access to the same byte index (I - // think this is fine when the slice doesn't grow). + toUpdate *os.File + config Config drawBuffer *draw.Buffer errorChannel chan error @@ -108,7 +106,7 @@ func (app *Application) Run( defer close(guiControlChannel) defer close(guiSpeedChange) // Very high FPS is good for responsiveness in the UI (since it's locked) and re-drawing on a re-size. - graph, cleanup, terminalSizeUpdates, err := app.g.Run(ctx, cancelFunc, *app.config.debugFps, app.listeners(), app.fallbacks) + graph, cleanup, terminalSizeUpdates, err := app.g.Run(ctx, cancelFunc, app.listeners(), app.fallbacks) termRecover := func() { _ = app.term.ClearScreen(terminal.UpdateSize) cleanup() diff --git a/cmd/tab_completion/tab_completion_test.go b/cmd/tab_completion/tab_completion_test.go index 0e324fa..0e61f8d 100644 --- a/cmd/tab_completion/tab_completion_test.go +++ b/cmd/tab_completion/tab_completion_test.go @@ -9,7 +9,9 @@ package tabcompletion import ( "flag" + "fmt" "log" + "log/slog" "os" "path/filepath" "slices" @@ -37,14 +39,7 @@ func TestGetChoices(t *testing.T) { actual, err := runGetChoices("acci-ping") assert.NilError(t, err) - expectedFlags := []string{} - accipingFlags.Fs.VisitAll(func(f *flag.Flag) { - if strings.HasPrefix(f.Name, "debug") { - return - } - expectedFlags = append(expectedFlags, "-"+f.Name) - }) - + expectedFlags := acciPingNonDebugFlags() assertEqual(t, actual, slices.Concat([]string{"drawframe", "version"}, expectedFlags)) }) t.Run("start drawframe", func(t *testing.T) { @@ -70,27 +65,15 @@ func TestGetChoices(t *testing.T) { actual, err := runGetChoices("acci-ping", "-") assert.NilError(t, err) - expectedFlags := []string{} - accipingFlags.Fs.VisitAll(func(f *flag.Flag) { - if strings.HasPrefix(f.Name, "debug") { - return - } - expectedFlags = append(expectedFlags, "-"+f.Name) - }) + expectedFlags := acciPingNonDebugFlags() assertEqual(t, actual, expectedFlags) }) - t.Run("start drawframe ", func(t *testing.T) { + t.Run("start drawframe tab", func(t *testing.T) { t.Parallel() actual, err := runGetChoices("acci-ping", "drawframe", "") assert.NilError(t, err) - expectedFlags := []string{} - drawframe.GetFlags(nil).VisitAll(func(f *flag.Flag) { - if strings.HasPrefix(f.Name, "debug") { - return - } - expectedFlags = append(expectedFlags, "-"+f.Name) - }) + expectedFlags := drawframeNonDebugFlags() expectedFlags = slices.Concat(expectedFlags, filesByExt(".go")) assertEqual(t, actual, expectedFlags) }) @@ -112,16 +95,17 @@ func TestGetChoices(t *testing.T) { t.Parallel() starting := []string{} accipingFlags.Fs.VisitAll(func(f *flag.Flag) { - if strings.HasPrefix(f.Name, "debug") { + if strings.HasPrefix(f.Name, "debug") || strings.HasPrefix(f.Name, "file") || strings.HasPrefix(f.Name, "theme") { return } starting = append(starting, "-"+f.Name) }) start := sliceutils.TakeRandom(starting) - expectedFlags := sliceutils.Remove(starting, start) + expectedFlags := sliceutils.Remove(acciPingNonDebugFlags(), start) actual, err := runGetChoices("acci-ping", start, "") assert.NilError(t, err) + slog.Info(fmt.Sprintf("random selection %q %q", start, starting)) assertEqual(t, actual, expectedFlags) }) t.Run("start -file", func(t *testing.T) { @@ -204,7 +188,6 @@ func make_acciping_Flags() Command { _ = tf.String("file", "", "skipped for test", tabflags.AutoComplete{WantsFile: true, FileExt: ".go"}) _ = tf.Bool("hide-help", false, "skipped for test") - _ = tf.Float64("pings-per-minute", 60.0, "skipped for test") _ = tf.Bool("debug-error-creator", false, "skipped for test") _ = tf.String("url", "www.google.com", "skipped for test", tabflags.AutoComplete{}) _ = tf.String("theme", "", "skipped for test", @@ -250,3 +233,25 @@ func assertEqual(t *testing.T, expected, actual []string) { slices.Sort(expected) assert.DeepEqual(t, actual, expected) } + +func acciPingNonDebugFlags() []string { + expectedFlags := []string{} + accipingFlags.Fs.VisitAll(func(f *flag.Flag) { + if strings.HasPrefix(f.Name, "debug") { + return + } + expectedFlags = append(expectedFlags, "-"+f.Name) + }) + return expectedFlags +} + +func drawframeNonDebugFlags() []string { + expectedFlags := []string{} + drawframe.GetFlags(nil).VisitAll(func(f *flag.Flag) { + if strings.HasPrefix(f.Name, "debug") { + return + } + expectedFlags = append(expectedFlags, "-"+f.Name) + }) + return expectedFlags +} diff --git a/cmd/tab_completion/tabflags/tabflags.go b/cmd/tab_completion/tabflags/tabflags.go index b3936d1..8a99e4a 100644 --- a/cmd/tab_completion/tabflags/tabflags.go +++ b/cmd/tab_completion/tabflags/tabflags.go @@ -81,6 +81,7 @@ func (f *FlagSet) GetAutoCompleteFor(flagName string) *AutoComplete { func (f *FlagSet) GetNames(includeDebug bool, toSkip []string) []string { f.syncFlagSet() keys := maps.Keys(f.nameToAc) + // Add the dash first so that we remove items matching [toSkip] withDash := iterutils.Map(keys, func(n string) string { return "-" + n }) diff --git a/draw/draw.go b/draw/draw.go index 22dd834..fa9764b 100644 --- a/draw/draw.go +++ b/draw/draw.go @@ -23,9 +23,6 @@ type Buffer struct { } // TODO paint buffer should be application level and agnostic to the draw buffer itself. -// -// TODO this buffering has race conditions, data can be written to the paint buffer while it's being read for -// painting. A full investigation is required to determine if this is actually a problem. func NewPaintBuffer() *Buffer { return newBuffer(int(indexCount.Load())) } diff --git a/graph/drawing.go b/graph/drawing.go index 9b9981a..13f1065 100644 --- a/graph/drawing.go +++ b/graph/drawing.go @@ -28,10 +28,9 @@ import ( ) type computeFrameConfig struct { - timeBetweenFrames time.Duration - followLatestSpan bool - drawSpinner bool - yAxisScale YAxisScale + followLatestSpan bool + drawSpinner bool + yAxisScale YAxisScale } func (c computeFrameConfig) Match(cfg computeFrameConfig) bool { @@ -41,14 +40,6 @@ func (c computeFrameConfig) Match(cfg computeFrameConfig) bool { const drawingDebug = false -func getTimeBetweenFrames(fps int, pingsPerMinute ping.PingsPerMinute) time.Duration { - if fps == 0 { - return ping.PingsPerMinuteToDuration(pingsPerMinute) - } else { - return time.Duration(1000/fps) * time.Millisecond - } -} - var noFrame = func(w io.Writer) error { return nil } func (g *Graph) computeFrame(cfg computeFrameConfig) func(io.Writer) error { diff --git a/graph/export_test.go b/graph/export_test.go index e6686bf..740edb3 100644 --- a/graph/export_test.go +++ b/graph/export_test.go @@ -24,7 +24,7 @@ func (g *Graph) ComputeFrame() string { painter := g.computeFrame(computeFrameConfig{ followLatestSpan: false, drawSpinner: false, - yAxisScale: g.presentation.YAxisScale, + yAxisScale: g.presentation.Get().YAxisScale, }) err := painter(&b) check.NoErr(err, "While painting frame to string buffer") diff --git a/graph/graph.go b/graph/graph.go index 8fc3600..c56035f 100644 --- a/graph/graph.go +++ b/graph/graph.go @@ -21,17 +21,19 @@ import ( "github.com/Lexer747/acci-ping/gui" "github.com/Lexer747/acci-ping/ping" "github.com/Lexer747/acci-ping/terminal" + "github.com/Lexer747/acci-ping/utils/atomic" "github.com/Lexer747/acci-ping/utils/check" ) type Graph struct { - ui gui.GUI - Term *terminal.Terminal - dataChannel <-chan ping.PingResults - data *graphdata.GraphData - frameMutex *sync.Mutex - drawingBuffer *draw.Buffer - presentation *controlState + ui gui.GUI + Term *terminal.Terminal + dataChannel <-chan ping.PingResults + data *graphdata.GraphData + frameMutex *sync.Mutex + drawingBuffer *draw.Buffer + + presentation atomic.Of[Presentation] controlChannel <-chan Control lastFrame frame initial ping.PingsPerMinute @@ -104,7 +106,7 @@ func NewGraph(ctx context.Context, cfg GraphConfiguration) *Graph { ui: cfg.Gui, debugStrict: cfg.DebugStrict, controlChannel: cfg.ControlPlane, - presentation: &controlState{Presentation: cfg.Presentation, m: &sync.Mutex{}}, + presentation: atomic.Init(cfg.Presentation), } if ctx != nil { // A nil context is valid: It means that no new data is expected and the input channel isn't active @@ -128,12 +130,9 @@ func NewGraph(ctx context.Context, cfg GraphConfiguration) *Graph { func (g *Graph) Run( ctx context.Context, stop context.CancelCauseFunc, - fps int, // this isn't really an FPS given how the GUI is setup and paint buffers, more like a max re-paint delay timer thing. listeners []terminal.ConditionalListener, fallbacks []terminal.Listener, ) (func() error, func(), <-chan terminal.Size, error) { - timeBetweenFrames := getTimeBetweenFrames(fps, g.initial) - frameRate := time.NewTicker(timeBetweenFrames) cleanup, err := g.Term.StartRaw(ctx, stop, listeners, fallbacks) if err != nil { return nil, cleanup, nil, err @@ -143,12 +142,14 @@ func (g *Graph) Run( size := g.Term.GetSize() defer close(terminalUpdates) slog.Info("running acci-ping") + // The main loop of acci-ping, await on the context (will be fired by the terminal) for { select { case <-ctx.Done(): return context.Cause(ctx) - case <-frameRate.C: - err = g.Term.UpdateSize() + default: + // The main body of acci-ping, get the terminal size and send on the channel if changed + err := g.Term.UpdateSize() if err != nil { return err } @@ -157,14 +158,17 @@ func (g *Graph) Run( terminalUpdates <- size size = g.Term.GetSize() } - g.presentation.m.Lock() + // Lock the presentation layer (this ensures we don't get GUI tearing - as in one component + // has data from last frame and another component has data from this frame), compute a new + // frame. Note that the optimisations to do no drawing are all in this function. + p := g.presentation.Get() toWrite := g.computeFrame(computeFrameConfig{ - timeBetweenFrames: timeBetweenFrames, - followLatestSpan: g.presentation.Following, - drawSpinner: true, - yAxisScale: g.presentation.YAxisScale, + followLatestSpan: p.Following, + drawSpinner: true, + yAxisScale: p.YAxisScale, }) - g.presentation.m.Unlock() + // Now that we have a frame to draw (may just be a spinner update), execute this function + // writing the painted frame to the terminal. err = toWrite(g.Term) if err != nil { return err @@ -188,13 +192,12 @@ func (g *Graph) OneFrame() error { if err != nil { return err } - g.presentation.m.Lock() + p := g.presentation.Get() toWrite := g.computeFrame(computeFrameConfig{ - followLatestSpan: g.presentation.Following, + followLatestSpan: p.Following, drawSpinner: false, - yAxisScale: g.presentation.YAxisScale, + yAxisScale: p.YAxisScale, }) - g.presentation.m.Unlock() return toWrite(g.Term) } @@ -216,8 +219,6 @@ func (g *Graph) Summarise() string { } func (g *Graph) ClearForPerfTest() { - g.presentation.m.Lock() - defer g.presentation.m.Unlock() g.lastFrame = frame{spinnerData: spinner{timestampLastDrawn: time.Now()}} g.drawingBuffer = draw.NewPaintBuffer() } @@ -249,22 +250,18 @@ func (g *Graph) handleControl(ctx context.Context) { if !ok { return } - // Note we don't need the mutex while reading the values. guiChanged := p.FollowLatestSpan.DidChange || p.YAxisScale.DidChange - if guiChanged { - // But we do need it while writing - g.presentation.m.Lock() - } + pr := Presentation{} if p.FollowLatestSpan.DidChange { - g.presentation.Following = p.FollowLatestSpan.Value + pr.Following = p.FollowLatestSpan.Value slog.Info("switching to:", "FollowLatestSpan", p.FollowLatestSpan.Value) } if p.YAxisScale.DidChange { - g.presentation.YAxisScale = p.YAxisScale.Value + pr.YAxisScale = p.YAxisScale.Value slog.Info("switching to:", "YAxisScale", p.YAxisScale.Value) } if guiChanged { - g.presentation.m.Unlock() + g.presentation.Set(pr) } } } @@ -296,8 +293,3 @@ func (f frame) Match(s terminal.Size, cfg computeFrameConfig) bool { func (f frame) Size() terminal.Size { return terminal.Size{Height: f.xAxis.size, Width: f.yAxis.size} } - -type controlState struct { - m *sync.Mutex - Presentation -} diff --git a/graph/graphdata/graphdata.go b/graph/graphdata/graphdata.go index 7e0b5e9..842e1af 100644 --- a/graph/graphdata/graphdata.go +++ b/graph/graphdata/graphdata.go @@ -8,7 +8,6 @@ package graphdata import ( "fmt" - "io" "sync" "time" @@ -67,12 +66,6 @@ func (gd *GraphData) Summary() string { return gd.data.Summary() } -func (gd *GraphData) AsCompact(w io.Writer) error { - gd.m.Lock() - defer gd.m.Unlock() - return gd.data.AsCompact(w) -} - func (gd *GraphData) Lock() { gd.m.Lock() }