I think JS_updateMallocCounter in newer spidermonkeys may be helpful here. Not sure I remember correctly. There's been a fair amount of discussion of this bug, but I'm not finding a link quickly.

What we've been doing for now in the litl shell is just running a GC in an idle. (Well, since we have a highly custom main loop, we GC anytime nothing at all has happened for a short period of time, or something like that.)

Another option is for the gjs process itself to watch its own memory usage via e.g. mallinfo().  GLib defaults to using libc malloc, so on Linux/glibc we'll be using the right thing.

We could wrap this as gjs_context_maybe_gc(), and look at the malloc delta (info.arena + info.hblkhd) between calls.  If it's larger than say 32MB, try a JS GC.

mallinfo sounds pretty good. One way that fails is lots of mallocs that are never owned by JS objects, but that shouldn't be common, and the failure consequences are hopefully not disastrous (just some extra GC).

One thing about SpiderMonkey when I was looking at this last, is that each GC comes only when memory increases by 1/3 since the previous GC. The threshold can thus get pretty high. With JS objects pointing to C objects we were rapidly getting in a state where we'd use an incredible amount of RAM without ever GC'ing.

If your system has a lot of RAM (with swap), conceivably you do want to be reluctant to create a GC pause ... it's better to just use a lot of RAM. But with JS objects pointing to native objects "a lot of RAM" can truly be a lot.

Created attachment 180020 [details] [review]
gjs_context_maybe_gc: New function to hint GC may be necessary

Consumers such as gnome-shell can use this instead of JS_MaybeGC();
it better handles native glibc memory allocations.

We also invoke it internally now after creating a GObject or a GBoxed
derivative.

When I originally wrote this patch I just had gjs_context_maybe_gc() that apps could call, but then I decided the cost of just invoking it on every g_object_new() is relatively small compared to all the other stuff we're doing.

My test script:

#!/usr/bin/env gjs

var Mainloop = imports.mainloop;
var Pango = imports.gi.Pango;

var i;
for (i = 0; i < 1000000; i++) {
    var c = new Pango.Context();
}

Mainloop.run("default");


Test machine: RHEL 6 x86_64, xulrunner-1.9.2.13-3.el6_0.x86_64

Before:
  Idle RSS = 276M
After:
  Idle RSS = 6692

Definitely an improvement =)

Review of attachment 180020 [details] [review]:

looks good to me. the only real danger is that mallinfo() is slow. Looking at the implementation in glibc, I'm hopeful it's fast, though to fully understand it I'd have to read a fair amount of code it looks like.

::: gjs/jsapi-util.c
@@ +1541,3 @@
+#define _GJS_MAYBE_GC_MALLOC_BYTES (16 * 1024 * 1024)
+
+    static int _gjs_last_maybe_gc_malloc_blocks = -1;

making this a global instead of storing per-runtime somehow is sort of wrong, but you're probably right to go this route, strict correctness is probably slow/complex with no actual value

@@ +1549,3 @@
+        struct mallinfo mstats;
+        mstats = mallinfo();
+        if (mstats.uordblks < _gjs_last_maybe_gc_malloc_blocks || _gjs_last_maybe_gc_malloc_blocks == -1) {

it's bugging me a little that mstats.uordblks is a 32-bit signed int but I guess we can't do much about it. wondering what happens if you allocate more than it holds.

the docs say uordblks is a "size" but why is it "blks" instead of "bytes"? are you sure it's bytes?

(In reply to comment #8)
> Review of attachment 180020 [details] [review]:
> 
> looks good to me. the only real danger is that mallinfo() is slow. Looking at
> the implementation in glibc, I'm hopeful it's fast, though to fully understand
> it I'd have to read a fair amount of code it looks like.

It looks to me like most of it is just some loops with bit masking and addition, but yes.  It is a concern.

But our failure mode here is so bad that I'd rather do this for now - we can always change things later.  An obvious thing to do later would be to queue an idle on the thread default main context to run this only once.

> ::: gjs/jsapi-util.c
> @@ +1541,3 @@
> +#define _GJS_MAYBE_GC_MALLOC_BYTES (16 * 1024 * 1024)
> +
> +    static int _gjs_last_maybe_gc_malloc_blocks = -1;
> 
> making this a global instead of storing per-runtime somehow is sort of wrong,
> but you're probably right to go this route, strict correctness is probably
> slow/complex with no actual value

Per runtime?  But malloc() is global, and that's where we're getting data from.  We don't know which JSRuntime caused how much malloc.  And yeah, more complexity.

> it's bugging me a little that mstats.uordblks is a 32-bit signed int but I
> guess we can't do much about it. wondering what happens if you allocate more
> than it holds.

Yes, um...heh so this interface appears to come from SVID, which obviously predates 64 bit computing etc.  In the glibc sources it's INTERNAL_SIZE_T which is really a size_t, i.e. guint64.  Then it does:

  mi.uordblks = av->system_mem - avail;

Without a cast, so we're going guint64 to gint32, which means we'll truncate.  That in turn means past 4G of memory, our usage of mallinfo() is going to become unreliable.  But I don't see an alternative here.

> the docs say uordblks is a "size" but why is it "blks" instead of "bytes"? are
> you sure it's bytes?

Pretty sure, from reading the source.  Also it corresponds with the values I see from my test program.

Attachment 180020 [details] pushed as 45b550d - gjs_context_maybe_gc: New function to hint GC may be necessary