GNOME Bugzilla – Bug 725099
Automatically schedule maybe_gc() cycles
Last modified: 2014-04-10 21:12:16 UTC
See discussion at https://mail.gnome.org/archives/gtk-devel-list/2014-February/msg00027.html We now schedule a maybe_gc() cycle in a low-priority idle whenever we return back from JS execution.
Created attachment 270188 [details] [review] util: add private methods to schedule/unschedule a maybe_gc These will be called right after leaving a JS execution context - implementation in a separate commit.
Created attachment 270189 [details] [review] Schedule a maybe_gc pass after executing JS code After we return from JS::Call* or JS::Evaluate*, schedule a maybe_gc() for the next idle iteration, to prevent memory consumption skyrocketing.
Review of attachment 270188 [details] [review]: So...we of course used to do this in the shell: https://bugzilla.gnome.org/show_bug.cgi?id=679832 We never picked up after that though. If you've tested this, I'm ok with exposing it as an internal API, and then we can discuss external heuristics.
Review of attachment 270189 [details] [review]: ::: gjs/context.cpp @@ +643,2 @@ ret = TRUE; + gjs_schedule_maybe_gc(js_context->context); Looking at this again...it seems weird that gjs_schedule_maybe_gc() takes a context argument, but _unschedule() doesn't. We're inconsistent on this front because we really do only support one context per process. But it'd still be nice to centralize most of the statics in context.cpp, and access them via gjs_context_get_current().
Created attachment 270512 [details] [review] jsapi-util: split a gjs_gc_if_needed() function We're going to use this later.
Created attachment 270513 [details] [review] context: add a private method to schedule a GC if needed There's a bit of a roundtrip between jsapi-util.cpp and context.cpp here, but we need to save the source ID on GjsContext.
Created attachment 270514 [details] [review] all: schedule a full GC check after executing JS code After we return from JS::Call* or JS::Evaluate*, schedule a GC check for the next idle iteration, to prevent memory consumption skyrocketing.
Thanks for the reviews, Colin. I now found the time to test these patches with a full jhbuild session from git master and I found some problems with them. Essentially it seems to me it's not safe to defer the JS_MaybeGC() call to an idle, it will cause a crash when the idle is reached in gnome-shell. So I left gjs_maybe_gc() alone, split the logic to GC if needed in a separate function and only schedule that when the idle is reached. I also attempted to address your concern about multiple contexts; implementation is a bit more twisted but I do think the layering is improved here. These are now tested in a full session and they seem to work well.
(In reply to comment #8) > Thanks for the reviews, Colin. > I now found the time to test these patches with a full jhbuild session from git > master and I found some problems with them. > Essentially it seems to me it's not safe to defer the JS_MaybeGC() call to an > idle, it will cause a crash when the idle is reached in gnome-shell. Is this about needing a compartment and a request around JS_MaybeGC?
(In reply to comment #9) > Is this about needing a compartment and a request around JS_MaybeGC? JS_MaybeGC documentation doesn't mention that as a requirement, so I don't think so. It does mention though that it's safe to call the function immediately after returning from JS_Call* or JS_Evaluate*, so that's what my patches do.
(In reply to comment #10) > (In reply to comment #9) > > > Is this about needing a compartment and a request around JS_MaybeGC? > > JS_MaybeGC documentation doesn't mention that as a requirement, so I don't > think so. It does mention though that it's safe to call the function > immediately after returning from JS_Call* or JS_Evaluate*, so that's what my > patches do. JS documentation is terribly outdated, the only reliable source is the mozjs code. Or a mozjs debug build. I'm pretty sure the compartment is needed for JS_MaybeGC and JS_GC, not sure about the request, but probably that too.
You should be using a debug build of libmozjs always. Always. No exceptions.
I am testing these with a debug build of mozjs now and I don't see any errors.
Created attachment 274041 [details] [review] jsapi-util: split a gjs_gc_if_needed() function We're going to use this later.
Created attachment 274042 [details] [review] context: add a private method to schedule a GC if needed There's a bit of a roundtrip between jsapi-util.cpp and context.cpp here, but we need to save the source ID on GjsContext.
Created attachment 274043 [details] [review] all: schedule a full GC check after executing JS code After we return from JS::Call* or JS::Evaluate*, schedule a GC check for the next idle iteration, to prevent memory consumption skyrocketing.
Created attachment 274044 [details] [review] test: avoid immediate GC on the Test object We need to keep this variable alive, otherwise it will be immediately garbage collected now that we trigger GC when we exit JS.
Attachment 274041 [details] pushed as 4e6ae06 - jsapi-util: split a gjs_gc_if_needed() function Attachment 274042 [details] pushed as b9741e9 - context: add a private method to schedule a GC if needed Attachment 274043 [details] pushed as 5719e78 - all: schedule a full GC check after executing JS code Attachment 274044 [details] pushed as 6de94ed - test: avoid immediate GC on the Test object Reviewed by Giovanni at the SF hackfest.