notifications@cinnamon.org/applet.js: fixes and performance improvements - #13929
Open
ntwest wants to merge 8 commits into
Open
notifications@cinnamon.org/applet.js: fixes and performance improvements#13929ntwest wants to merge 8 commits into
ntwest wants to merge 8 commits into
Conversation
…tions applet. Covers: - rendered order in both sort directions - banner handoff and handback, including a revision while banner up - urgency picking the panel icon - an arrival while the menu is open - transient notifications - the panel and menu chrome - a source holding several notifications - a clear whose destroy() fails partway - benchmark() times the operations that block the main loop
…t is removed. on_applet_removed_from_panel() left the menu in the ui group, so every reload leaked another one, still drawable and still showing a stale count above buttons whose applet was gone. Test: checkMenuNotLeaked()
critical_blink() threw away the timeout id, so nothing could cancel it. It kept ticking after the applet was removed, against icons already destroyed, and a second chain could start while the first was still queued. Test: checkCriticalBlink()
… when the applet is removed. 'scrolling-changed' and 'destroy' are connected on every notification through raw connect(), so disconnectAllSignals() never reaches them. A notification still listed at removal kept a closure holding the applet alive. Test: checkSignalsDisconnected()
…troyed notification. _notification_added() called actor.unparent() before checking _destroyed, and the tray hands a notification back after hiding its banner even when it was destroyed while shown. The actor is gone by then, so that was a critical on a disposed object. Test: checkDestroyedHandback()
…update. timeify() constructed a Gio.Settings and read the clock format for every notification, so opening a tray of 200 built 200 of them. Test: checkClockSettings()
…d of reparenting them. update_list() rebuilt the whole list on every change, so clearing N notifications reparented every remaining actor N times. Clearing and bursts of arrivals stop being quadratic in the size of the tray. Also fixes two errors. The applet's list and the bin's children are not the same set while the tray has borrowed an actor to show as a banner: _reorderNotifications() rebuilt the bin from the list, calling add_child() on an actor that already had a parent, and _clear_all() removed that actor from a container that was not its parent. Test: checkBorrowedActor()
…ws near the viewport.
Opening a tray of 100+ notifications blocked for hundreds of ms and grew
with the tray. Only the rows near the viewport are parented now, spacers
reserve the height of the rest, and the cap scales with the viewport.
Now opening and reopening a large tray get much cheaper.
Test: checkRowHeights(), checkRenderedGeometry(), checkGrowOnly(),
checkPseudoClasses(), checkOffsetsCurrent()
Best-practices scannerThis is a regex-based check for API usage that can pose security, performance or This check is not perfect and will not replace a normal review.Found 1 potential issue(s):
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Speed up the notifications applet
I was playing with the "peon-ping" app and it left a few hundred notifications in my tray.
Opening it froze the shell for about a second; clearing it froze the shell for seven to ten seconds.
These eight commits fix that, plus a few smaller bugs, and include a test script.
1.
Add Looking Glass checks for the notifications applet.—38eb1ddb9Test only. Ten checks, all passing on master unchanged, plus
benchmark(), which produces everyfigure below. Each commit after this one adds the check for the bug it fixes, so the finished series
has twenty-one. Those later checks detect the bug their own commit fixes and fail before it.
2 to 6. Five lifecycle and allocation fixes. —
718bfb606,54fe40592,7dc878b2c,eaf69f111,8b6d25f2cEach is independent and small. The menu outlived the applet, so every reload leaked another one.
The critical blink re-armed a one second timeout and threw away the source id, so nothing could
cancel it. Handlers connected with a raw
connect()are not covered bydisconnectAllSignals(),so a notification still listed at removal kept the applet alive. The
_destroyedcheck came afteractor.unparent(), so the tray handing back a notification destroyed while its banner was up meanttouching a disposed object. And
timeify()built aGio.Settingsper notification, so opening atray of 200 built 200 of them.
7.
Move notification actors instead of reparenting them.—53c16b201Re-adding a child to a mapped container makes St restyle its whole subtree, and
update_list()rebuilt the list on every change.
All four still walk the list, so they still grow with the tray, just no longer quadratically. Also
closes two Clutter error paths hit while the tray has borrowed a row for a banner.
8.
Parent only the notification rows near the viewport.—55246260bOnly the rows near the viewport are parented, with an idle pass filling outward. Spacers reserve the
height of the rest, so the scrollbar is right without building it. The cap scales with the viewport,
so a rotated monitor is not a special case.
Against commit 7, which this builds on. Opening still grows with the tray, 33.5 ms at 20 against
90.2 ms at 200. Reordering regressed, 2.6 ms to 16.7 ms at 100, since a reorder now rebuilds the
offsets.
Taking only some of this
You can take the first few commits and stop anywhere, without the rest. What you cannot do is take
one on its own: each builds on the ones before it, so no single commit applies to master by itself.
Stopping after commit 6 gives the four lifecycle bugs and the allocation fix. Stopping after commit
7 adds the reparenting fix and the large speedups, with no new architecture, and is the conservative
choice. Commit 8 adds the virtualization.
Checking it
From Looking Glass:
let t = imports.testing.testNotificationsApplet, thent.checkOrder()and soon. Each returns true or false and logs every case.
checkMenuNotLeaked()andcheckSignalsDisconnected()reload the applet, so use a container or a throwaway session.Known limits
Every figure is the median of three runs, taken in one sitting on one machine against Cinnamon
6.6.9. Absolute numbers move a lot with machine load, so the ratios are the durable part. Row
heights were checked on three themes. Scroll behaviour is verified in a container: the scroll
position is set and the geometry read back in a separate D-Bus call, so a relayout completes
between the two.