Skip to content

fix: remove redundant cache load and avoid GSettings lookup on every tick#534

Open
glazari1 wants to merge 1 commit intocorecoding:graph-historyfrom
glazari1:fix/follow-up-optimizations
Open

fix: remove redundant cache load and avoid GSettings lookup on every tick#534
glazari1 wants to merge 1 commit intocorecoding:graph-historyfrom
glazari1:fix/follow-up-optimizations

Conversation

@glazari1
Copy link
Copy Markdown

@glazari1 glazari1 commented Apr 8, 2026

Hey @corecoding, here's the follow-up with the two items from my comment.

Summary

  • Extract _ensureTimeSeriesLoaded() to deduplicate the cache load + merge
    logic (now used on first hover and on disable — prevents overwriting
    persisted history when the user never opened the popout)
  • Remove redundant loadTimeSeries() from _init() — the lazy-load
    handles it with proper merge
  • Cache show-sensor-history-graph in a _recordTimeSeries flag instead
    of calling get_boolean() on every sensor tick (6 calls/sec with
    default settings)
  • Keep _recordTimeSeries and _timeSeriesLoaded in sync when toggling
    the setting on/off

Changes

extension.js:

  • New _ensureTimeSeriesLoaded() method (extracted from _showHistoryPopout)
  • _showHistoryPopout() calls _ensureTimeSeriesLoaded() instead of inline merge
  • disable() calls _ensureTimeSeriesLoaded() before saving
  • Signal handler updates _recordTimeSeries and _timeSeriesLoaded on toggle
  • Remove loadTimeSeries() from _init()

values.js:

  • Add _recordTimeSeries flag initialized from settings in _init()
  • _pushTimePoint() checks cached flag instead of get_boolean()

Test plan

  • Enable graph, collect data, hover sensor — graph shows correctly
  • Enable graph, collect data, disable extension without hovering — re-enable and check history is preserved
  • Toggle graph off/on via settings, verify data recording stops/resumes
  • Monitor with pidstat — no GSettings overhead on every tick

…tick

- Extract _ensureTimeSeriesLoaded() to deduplicate cache load + merge
  logic (used on first hover and on disable)
- Remove redundant loadTimeSeries() from _init() — the lazy-load in
  _ensureTimeSeriesLoaded() handles it with proper merge
- Cache show-sensor-history-graph in a _recordTimeSeries flag instead
  of calling get_boolean() on every sensor tick
- Keep _recordTimeSeries and _timeSeriesLoaded in sync on setting toggle
@corecoding
Copy link
Copy Markdown
Owner

I forgot to push a change before your last PR, now there is a minor conflict. My primary change is below - this gets rid of two for loops. By the way, I ended up submitting an update to Vitals with the History feature removed. Another person submitted a bug report. I would like to get more people verifying this PR fixes the issue. In all honesty, even with 200 instead of 3600 datapoints, I still feel like there will be a stutter, albeit smaller.

        const memSeries = this._values._timeSeries;
        const memFormat = this._values._timeSeriesFormat;

@glazari1
Copy link
Copy Markdown
Author

glazari1 commented Apr 8, 2026

Thanks for the update! I understand the decision to remove the feature in v74 while it's being validated.

For what it's worth, I've been running the graph-history branch locally (Ryzen + GNOME 46, Ubuntu 24.04) and it's been completely smooth, measured ~12% average CPU on gnome-shell with zero GC spikes over multiple 15s samples using pidstat (was 60-70% spikes every ~11s before the fix). The widget reuse in history.js seems to make a big difference alongside the downsample.

Let me know if you'd like me to help with anything else or if more testing data would be useful.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants