Queue a GC when a toggle reference goes from >1 to 1
This is my hacky approach to the problem. This runs the GC much less than expected but more than what would be acceptable (@chergert's idea make it runs much less than my original approach). But it definitely holds the GNOME Shell memory at the same plateu for a long, long time, helping gnome-shell#64 not happen.
Some ideas that are worth considering:
Use a timeout instead of an idle add (it appearently didn't have the same effectiveness though...)- Don't force GC but instead improve the current heuristics
- What else?
Issue: #140 (closed)
-
changed the description
Toggle commit list -
-
resolved all discussions
Toggle commit list -
-
resolved all discussions
Toggle commit list -
changed the description
Toggle commit list -
-
-
added 14 commits
-
c6738473...e9f4c12e - 12 commits from branch
master
- 80b3b831 - context: Add API to force GC schedule
- aafe8fd6 - object: Queue a forced GC when toggling down
Toggle commit list -
c6738473...e9f4c12e - 12 commits from branch
-
changed the description
Toggle commit list -
-
Tested in Ubuntu 18.04 and this fix works well for me. I no longer see sustained growth in gnome-shell's memory usage when repeatedly entering and leaving the overview.
-
-
-
Master
As we've discussed on IRC, I'm not opposed to this but I would like some more confirmation of how it does or doesn't affect performance, especially on machines that are not programmer-caliber fast.
It is a big hammer, perhaps necessary, but I'd like to keep exploring other options such as !50 (merged) in addition.
-
Developer
FWIW, I've been playing with an even bigger hammer for more than a week now (unconditional GC in gnome-shell leisure handler), and I have barely seen any stalls all this time.
I just did that to reduce noise in heaptrack and never thought of it as a viable option, but it raises my confidence on this patch. The GC would be just as effective with less grinding involved.
This was on an i5-6200U, I guess on the mediocre side of "programmer-grade". Anyhow, I'll try to get this patch up on the baytrail tablet and report back.
-
resolved all discussions
Toggle commit list -
Developer
FTR testing came out good! This is not an specially snappy machine, but framerates stood similar, the usual short stalls (eg. opening Activities) didn't get noticeably longer, and none was observed out of the blue.
-
I would say "ship it", but also:
"There are merge conflicts. Resolve these conflicts or ask someone with write access to this repository to merge it locally"
-
Developer
Sure, let me update it. Thanks for testing.
Ideally, though, we'd have a more structured way to measure the impact of these changes. None comes to my mind now, but I'm certain that even a quick research would yield meaningful results.
-
resolved all discussions
Toggle commit list -
added 25 commits
-
aafe8fd6...4a28c6b2 - 23 commits from branch
master
- 33cbbeb1 - context: Add API to force GC schedule
- 3f94b11a - object: Queue a forced GC when toggling down
Toggle commit list -
aafe8fd6...4a28c6b2 - 23 commits from branch
-
unmarked as a Work In Progress
Toggle commit list -
Developer
One thought is that we can add "marks" to the Sysprof profile format, and then use them at begin/end of a GC operation. (We'll also need a visualizer in Sysprof for marks).
Then you would just enable the profiler to compare GJS versions.
-
Developer
That would be ideal :). But I don't think the numbers would be really meaningful this time, given the GC strategy we are switching from is basically nonexistent.
Of course would be nice to ensure the median run of a GC doesn't take longer than say half a frame, but I don't think we have such fine grained control. Still the patch seems favorable in that it won't usually let many elements accumulate.
-
-
Master
It shouldn't be necessary to rebase. There's no merge conflict.
-
resolved all discussions
Toggle commit list -
added 19 commits
-
3f94b11a...a6b6fc13 - 17 commits from branch
master
- 09029851 - context: Add API to force GC schedule
- e9e96955 - object: Queue a forced GC when toggling down
Toggle commit list -
3f94b11a...a6b6fc13 - 17 commits from branch
-
Developer
@iainl @vanvugt The most important and interesting information is how much this patch impacts the performance of GJS/GNOME Shell on lower-end hardware. It is somewhat a aggressive approach, since it forces a GC every time an object is destroyed. It might be not as bad as we think because each GC now has a much smaller number of objects to collect in average.
I'd feel most secure with this patchset if this kind of information is provided. I myself will test on a lower hardware of mine, but the more data points we have, the better.
-
I think the smaller memory footprint is going to benefit low-end hardware more than the slightly increased execution time will hurt (if at all - I can't see any increase right now).
While I'm a big advocate of making everything perform silky-smooth on low-end hardware, I haven't got to doing that with gnome-shell yet. Unfortunately the reason is that we don't have great performance on even high-end hardware yet. So that's my focus at the moment. I'm presently using an i7-7700 desktop, but had previously done most of my gnome-shell work on a Xeon E3-1246 v3.
-
Ubuntu 18.04 now contains this patch, so we're about to find out how it goes on a large scale: https://launchpad.net/ubuntu/+source/gjs/1.52.1-1ubuntu1
-
Master
Seems fine. If we do notice stalls during animations, then we could consider rate-limiting the forced GCs to once per 5 frames, as is done with the RSS checks in
gjs_gc_if_needed()
. But I think the idle function at low priority is probably going to mean that there won't be any forced GCs during animations.Ubuntu 18.04 now contains this patch, so we're about to find out how it goes on a large scale
That's up to you, but I don't intend to backport this to 1.52 until we run it for a while on 1.53.x. I guess Ubuntu is that testing ground now...
-
merged
Toggle commit list