Skip to content

  • Projects
  • Groups
  • Snippets
  • Help
  • This project
    • Loading...
  • Sign in / Register
gjs
gjs
  • Overview
    • Overview
    • Details
    • Activity
    • Cycle Analytics
  • Repository
    • Repository
    • Files
    • Commits
    • Branches
    • Tags
    • Contributors
    • Graph
    • Compare
    • Charts
  • Issues 88
    • Issues 88
    • List
    • Board
    • Labels
    • Milestones
  • Merge Requests 16
    • Merge Requests 16
  • CI / CD
    • CI / CD
    • Pipelines
    • Jobs
    • Schedules
    • Charts
  • Registry
  • Wiki
    • Wiki
  • Snippets
    • Snippets
  • Members
    • Members
  • Collapse sidebar
  • Activity
  • Graph
  • Charts
  • Create a new issue
  • Jobs
  • Commits
  • Issue Boards
  • GNOME
  • gjsgjs
  • Merge Requests
  • !114

Merged
Opened Mar 31, 2018 by Georges Basile Stavracas Neto@feaneron 
  • Report abuse
Report abuse

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)

Edited Apr 13, 2018 by Georges Basile Stavracas Neto
  • Discussion 20
  • Commits 2
  • Pipelines 7
  • Changes 3
{{ resolvedDiscussionCount }}/{{ discussionCount }} {{ resolvedCountText }} resolved
  • Georges Basile Stavracas Neto @feaneron

    changed the description

    Mar 31, 2018

    changed the description

    changed the description
    Toggle commit list
  • Christian Hergert
    @chergert started a discussion on an old version of the diff Mar 31, 2018
    Resolved by Georges Basile Stavracas Neto Mar 31, 2018
    gjs/context.cpp
    Unable to load the diff.
    • Christian Hergert @chergert commented Mar 31, 2018
      Developer

      If you have many objects toggling from >1 to ==1 toggle ref (say from some related resources being released), couldn't you remove/add the source many times before yielding back to the main context? Removing/creating/adding sources isn't free, and in this case you'll acquire the GMainContext lock here and upon adding the new source for each time this function is called.

      I think you can probably get away with just setting force_gc without flapping the source when non-zero. That is, unless pushing the callback to the end of the GMainContext queue is doing something useful.

      Additionally, you may want to js_context->force_gc |= force_gc so that you don't lose that bit if a followup call is made with true.

      Edited Mar 31, 2018 by Christian Hergert
      If you have many objects toggling from `>1` to `==1` toggle ref (say from some related resources being released), couldn't you remove/add the source many times before yielding back to the main context? Removing/creating/adding sources isn't free, and in this case you'll acquire the `GMainContext` lock here and upon adding the new source for each time this function is called. I think you can probably get away with just setting `force_gc` without flapping the source when non-zero. That is, unless pushing the callback to the end of the `GMainContext` queue is doing something useful. Additionally, you may want to `js_context->force_gc |= force_gc` so that you don't lose that bit if a followup call is made with `true`.
    • Georges Basile Stavracas Neto @feaneron

      changed this line in version 2 of the diff

      Mar 31, 2018

      changed this line in version 2 of the diff

      changed this line in [version 2 of the diff](https://gitlab.gnome.org/GNOME/gjs/merge_requests/114/diffs?diff_id=5089&start_sha=1a3b0a23225998aba1b153cd8c3b43745b5c5cf7#7b8db8ab8669d31b0be16009f6988120edc7b864_612_613)
      Toggle commit list
    • Georges Basile Stavracas Neto @feaneron commented Mar 31, 2018
      Developer

      Indeed, I thought pushing the source to the end of the queue was the only way to make it work, but not doing that also seems to work - it just doesn't free the memory right away.

      Indeed, I thought pushing the source to the end of the queue was the only way to make it work, but not doing that also seems to work - it just doesn't free the memory *right away*.
    Please register or sign in to reply
  • Georges Basile Stavracas Neto @feaneron

    resolved all discussions

    Mar 31, 2018

    resolved all discussions

    resolved all discussions
    Toggle commit list
  • Georges Basile Stavracas Neto @feaneron

    added 1 commit

    • c48e00e8 - object: Queue a forced GC when toggling down

    Compare with previous version

    Mar 31, 2018

    added 1 commit

    • c48e00e8 - object: Queue a forced GC when toggling down

    Compare with previous version

    added 1 commit <ul><li>c48e00e8 - object: Queue a forced GC when toggling down</li></ul> [Compare with previous version](https://gitlab.gnome.org/GNOME/gjs/merge_requests/114/diffs?diff_id=5089&start_sha=1a3b0a23225998aba1b153cd8c3b43745b5c5cf7)
    Toggle commit list
  • Georges Basile Stavracas Neto @feaneron

    resolved all discussions

    Mar 31, 2018

    resolved all discussions

    resolved all discussions
    Toggle commit list
  • Georges Basile Stavracas Neto @feaneron

    changed the description

    Mar 31, 2018

    changed the description

    changed the description
    Toggle commit list
  • Georges Basile Stavracas Neto @feaneron

    added 1 commit

    • 18cad6f3 - object: Queue a forced GC when toggling down

    Compare with previous version

    Mar 31, 2018

    added 1 commit

    • 18cad6f3 - object: Queue a forced GC when toggling down

    Compare with previous version

    added 1 commit <ul><li>18cad6f3 - object: Queue a forced GC when toggling down</li></ul> [Compare with previous version](https://gitlab.gnome.org/GNOME/gjs/merge_requests/114/diffs?diff_id=5090&start_sha=c48e00e87b45514015747c2cff783ae1146d33f8)
    Toggle commit list
  • Georges Basile Stavracas Neto @feaneron

    added 1 commit

    • c6738473 - object: Queue a forced GC when toggling down

    Compare with previous version

    Mar 31, 2018

    added 1 commit

    • c6738473 - object: Queue a forced GC when toggling down

    Compare with previous version

    added 1 commit <ul><li>c6738473 - object: Queue a forced GC when toggling down</li></ul> [Compare with previous version](https://gitlab.gnome.org/GNOME/gjs/merge_requests/114/diffs?diff_id=5091&start_sha=18cad6f370431792c2ca342d614a35e9a1d22a85)
    Toggle commit list
  • Georges Basile Stavracas Neto @feaneron

    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

    Compare with previous version

    Mar 31, 2018

    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

    Compare with previous version

    added 14 commits <ul><li>c6738473...e9f4c12e - 12 commits from branch <code>master</code></li><li>80b3b831 - context: Add API to force GC schedule</li><li>aafe8fd6 - object: Queue a forced GC when toggling down</li></ul> [Compare with previous version](https://gitlab.gnome.org/GNOME/gjs/merge_requests/114/diffs?diff_id=5092&start_sha=c673847356920247d603820e8724348a8feb0fd3)
    Toggle commit list
  • Georges Basile Stavracas Neto @feaneron

    changed the description

    Mar 31, 2018

    changed the description

    changed the description
    Toggle commit list
  • Daniel van Vugt @vanvugt

    mentioned in issue gnome-shell#160 (closed)

    Apr 05, 2018

    mentioned in issue gnome-shell#160 (closed)

    mentioned in issue gnome-shell#160
    Toggle commit list
  • Daniel van Vugt @vanvugt commented Apr 05, 2018

    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.

    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.
  • Daniel van Vugt
    @vanvugt started a discussion on the diff Apr 05, 2018
    Resolved by Georges Basile Stavracas Neto Apr 12, 2018
    gi/object.cpp
    Unable to load the diff.
    • Daniel van Vugt @vanvugt commented Apr 05, 2018

      Is this comment right? Should ">1" be ">=1" ?

      Edited Apr 06, 2018
      Is this comment right? Should ">1" be ">=1" ?
    • Carlos Garnacho @carlosg commented Apr 05, 2018
      Developer

      AFAIU no. Toggle references do increase the reference count, and let you know that the last reference held is from your own toggle reference. It is then when the object may be garbage collected, and that last reference is dropped.

      AFAIU no. Toggle references do increase the reference count, and let you know that the last reference held is from your own toggle reference. It is then when the object may be garbage collected, and that last reference is dropped.
    Please register or sign in to reply
  • Philip Chimento @ptomato

    changed milestone to GNOME 3.30.0

    Apr 07, 2018

    changed milestone to GNOME 3.30.0

    changed milestone to %4
    Toggle commit list
  • Philip Chimento @ptomato commented Apr 07, 2018
    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.

    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 in addition.
  • Carlos Garnacho @carlosg commented Apr 07, 2018
    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.

    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.
  • Georges Basile Stavracas Neto @feaneron

    resolved all discussions

    Apr 12, 2018

    resolved all discussions

    resolved all discussions
    Toggle commit list
  • Georges Basile Stavracas Neto @feaneron commented Apr 12, 2018
    Developer

    @carlosg please let us know how your tablet testing went with this patch.

    @carlosg please let us know how your tablet testing went with this patch.
  • Carlos Garnacho @carlosg commented Apr 12, 2018
    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.

    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.
  • Daniel van Vugt @vanvugt commented Apr 13, 2018

    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"

    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"
  • Georges Basile Stavracas Neto @feaneron commented Apr 13, 2018
    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.

    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.
  • Georges Basile Stavracas Neto @feaneron

    resolved all discussions

    Apr 13, 2018

    resolved all discussions

    resolved all discussions
    Toggle commit list
  • Georges Basile Stavracas Neto @feaneron

    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

    Compare with previous version

    Apr 13, 2018

    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

    Compare with previous version

    added 25 commits <ul><li>aafe8fd6...4a28c6b2 - 23 commits from branch <code>master</code></li><li>33cbbeb1 - context: Add API to force GC schedule</li><li>3f94b11a - object: Queue a forced GC when toggling down</li></ul> [Compare with previous version](https://gitlab.gnome.org/GNOME/gjs/merge_requests/114/diffs?diff_id=5657&start_sha=aafe8fd6b298cd4093f7c8ae174c06a988723472)
    Toggle commit list
  • Georges Basile Stavracas Neto @feaneron

    unmarked as a Work In Progress

    Apr 13, 2018

    unmarked as a Work In Progress

    unmarked as a **Work In Progress**
    Toggle commit list
  • Christian Hergert @chergert commented Apr 13, 2018
    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.

    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.
  • Carlos Garnacho @carlosg commented Apr 13, 2018
    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.

    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.
  • Carlos Garnacho @carlosg

    mentioned in issue gnome-shell#139

    Apr 15, 2018

    mentioned in issue gnome-shell#139

    mentioned in issue gnome-shell#139
    Toggle commit list
  • Daniel van Vugt @vanvugt commented Apr 16, 2018

    Please rebase again. You're missing critical crash fix 8510bede :)

    Please rebase again. You're missing critical crash fix 8510bede1dd1f8a5fb95a2f594b4d3a68289e5ea :)
  • Philip Chimento @ptomato commented Apr 16, 2018
    Master

    It shouldn't be necessary to rebase. There's no merge conflict.

    It shouldn't be necessary to rebase. There's no merge conflict.
  • Iain Lane @iainl commented Apr 18, 2018
    Developer

    @vanvugt can you state here what testing you've done please / on what hardware? I think upstream guys are hoping for more results. :-)

    @vanvugt can you state here what testing you've done please / on what hardware? I think upstream guys are hoping for more results. :-)
  • Georges Basile Stavracas Neto @feaneron

    resolved all discussions

    Apr 18, 2018

    resolved all discussions

    resolved all discussions
    Toggle commit list
  • Georges Basile Stavracas Neto @feaneron

    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

    Compare with previous version

    Apr 18, 2018

    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

    Compare with previous version

    added 19 commits <ul><li>3f94b11a...a6b6fc13 - 17 commits from branch <code>master</code></li><li>09029851 - context: Add API to force GC schedule</li><li>e9e96955 - object: Queue a forced GC when toggling down</li></ul> [Compare with previous version](https://gitlab.gnome.org/GNOME/gjs/merge_requests/114/diffs?diff_id=5917&start_sha=3f94b11a943c0e4d29c96930ced238580dc18fc7)
    Toggle commit list
  • Georges Basile Stavracas Neto @feaneron commented Apr 18, 2018
    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.

    @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.
  • Daniel van Vugt @vanvugt commented Apr 19, 2018

    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.

    Edited Apr 19, 2018 by Daniel van Vugt
    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.
  • Daniel van Vugt @vanvugt commented Apr 19, 2018

    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

    Edited Apr 19, 2018 by Daniel van Vugt
    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
  • Philip Chimento @ptomato commented Apr 19, 2018
    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...

    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...
  • Philip Chimento @ptomato

    mentioned in commit bf0afbd6

    Apr 19, 2018

    mentioned in commit bf0afbd6

    mentioned in commit bf0afbd64361f32feeeea838f412a12e43df7708
    Toggle commit list
  • Philip Chimento @ptomato

    merged

    Apr 19, 2018

    merged

    merged
    Toggle commit list
  • Write
  • Preview
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or sign in to comment
Assignee
No assignee
Assign to
GNOME 3.30.0
Milestone
GNOME 3.30.0
Assign milestone
Time tracking
0
Labels
None
Assign labels
  • View project labels
Reference: GNOME/gjs!114
×

Revert this merge request

This will create a new commit in order to revert the existing changes.

Switch branch
Cancel
A new branch will be created in your fork and a new merge request will be started.
×

Cherry-pick this merge request

Switch branch
Cancel
A new branch will be created in your fork and a new merge request will be started.