GNOME Bugzilla – Bug 724797
Enable incremental GCs
Last modified: 2017-04-02 03:28:45 UTC
A new feature in js24 that should yield better performance and less memory usage. Not sure we're doing all that's required though. Been running debug mozjs for a couple days now, let's see...
Created attachment 269798 [details] [review] Add support for write barriers to classes with a custom trace To support write barriers (which are a prerequisite for incremental GC), all manually traced pointers to GC things need to be converted to JS::Heap<T>, and their holders made into C++ objects with constructors and destructors.
Created attachment 269799 [details] [review] Mark that all our JSClasses support write barriers JSClasses without a trace hook trivially support them, and the others are using JS::Heap<>, which takes care of calling the GC before mutating the pointer.
Created attachment 269800 [details] [review] runtime: enable incremental GC Incremental GC mean that we no longer need long GC pauses that block the application, and that were especially visible in long-running apps like polari.
Created attachment 269801 [details] [review] runtime: sync default GC parameters with Firefox defaults Defaults taken from Firefox 27, but they are probably good for Firefox 24 too. gnome-shell might want to have different defaults, because 10ms of incremental GC slice is not good for a compositor, while 100MB of JS heap to trigger high-frequency GC is not ok for a background service.
Created attachment 270071 [details] [review] closure: run a MaybeGC at the end of closures MaybeGC is safe to call frequently, and this follows Firefox's idea of running the GC at the end of "scripts" (which in our case are JS closures and ffi calls). In practice, this means we run a MaybeGC before returning to C code, and in particular before returning to the mainloop from sources, which is good.
By the way, I rebased those patches on gjs from git and tried it. It didn't do anything. I think JS_MaybeGC(context); isn't really knowing it should do something or not (hence the maybe?). So I changed the call to JS_GC(JS_GetRuntime(context)); I'm not sure if that was the right thing to do. Every time I clicked Activities, both CPU cores would go to 60% for a few seconds and gnome-shell would eat that much CPU. Then it cooled off. But only 2MB increase in 12 hours. So it was doing garbage collection but freezing the desktop while doing so.
I went back to stock gjs from git since it wasn't worth the freezes.
Freezes are to be expected if you switch for JS_MaybeGC to JS_GC, as that forces an unconditional full GC every time. Obviously you don't want to run an unconditional full GC for every JS callback. These patches are for incremental GC, which is only triggered by JS_MaybeGC. Sorry if this code is not helping you.
Could you please rebase the patches on gjs trunk? They don't apply anymore. Thank you.
Created attachment 313688 [details] [review] Add support for write barriers to classes with a custom trace To support write barriers (which are a prerequisite for incremental GC), all manually traced pointers to GC things need to be converted to JS::Heap<T>, and their holders made into C++ objects with constructors and destructors.
Created attachment 313689 [details] [review] Mark that all our JSClasses support write barriers JSClasses without a trace hook trivially support them, and the others are using JS::Heap<>, which takes care of calling the GC before mutating the pointer.
Created attachment 313690 [details] [review] runtime: enable incremental GC Incremental GC mean that we no longer need long GC pauses that block the application, and that were especially visible in long-running apps like polari.
Created attachment 313691 [details] [review] runtime: sync default GC parameters with Firefox defaults Defaults taken from Firefox 27, but they are probably good for Firefox 24 too. gnome-shell might want to have different defaults, because 10ms of incremental GC slice is not good for a compositor, while 100MB of JS heap to trigger high-frequency GC is not ok for a background service.
Created attachment 313692 [details] [review] closure: run a MaybeGC at the end of closures MaybeGC is safe to call frequently, and this follows Firefox's idea of running the GC at the end of "scripts" (which in our case are JS closures and ffi calls). In practice, this means we run a MaybeGC before returning to C code, and in particular before returning to the mainloop from sources, which is good.
Hello. Would you consider committing those patches in git? I don't get to keep my computer on 24 hours a day but after weeks of using those patches I think the memory increase is a bit slower. Thank you.
Review of attachment 313688 [details] [review]: Looks correct -- though I don't totally understand why the Closure struct had to have a C++ destructor. Is it so that the JS::Heap destructor gets called? In any case, this ties into stuff that will need to be done for SpiderMonkey 38.
Review of attachment 313689 [details] [review]: Yep, I implemented the same thing in bug 776966, but I prefer this one because it adds the flag to all the classes.
Review of attachment 313691 [details] [review]: ::: gjs/runtime.cpp @@ +238,3 @@ JS_SetRuntimePrivate(runtime, data); + // commented are defaults in moz-24 I wonder if JS_SetGCParametersBasedOnAvailableMemory() would be a good fit here... I'll have to dig into the source code and see what it does.
Attachment 313688 [details] pushed as ad5e159 - Add support for write barriers to classes with a custom trace Attachment 313689 [details] pushed as fdb7fbd - Mark that all our JSClasses support write barriers
(In reply to Philip Chimento from comment #18) > Review of attachment 313691 [details] [review] [review]: > > ::: gjs/runtime.cpp > @@ +238,3 @@ > JS_SetRuntimePrivate(runtime, data); > > + // commented are defaults in moz-24 > > I wonder if JS_SetGCParametersBasedOnAvailableMemory() would be a good fit > here... I'll have to dig into the source code and see what it does. It seems JS_SetGCParametersBasedOnAvailableMemory() is more intended for simulating the environment of an embedded device. So that's a dead end. For future reference, here are the places where Firefox's defaults can be found: - For Firefox itself: https://dxr.mozilla.org/mozilla-esr52/source/modules/libpref/init/all.js#1256 - For SpiderMonkey's default JS shell: https://dxr.mozilla.org/mozilla-esr52/source/js/src/shell/js.cpp#7923
All the tests pass, even when running with pre- and post write barrier verification. We'll commit this early in the cycle, and fix problems if and when they come up. Attachment 313690 [details] pushed as 3e1bc71 - runtime: enable incremental GC Attachment 313691 [details] pushed as 867c1bb - runtime: sync default GC parameters with Firefox defaults Attachment 313692 [details] pushed as 0e41484 - closure: run a MaybeGC at the end of closures