Created attachment 157800 [details] [review]
shell-global: Add shell_global_maybe_gc

Add a method to call JS_MaybeGC, see
https://developer.mozilla.org/en/SpiderMonkey/JSAPI_Reference/JS_MaybeGC

Created attachment 157801 [details] [review]
Call global.maybe_gc when idle

Call shell_global_maybe_gc when idle to free up memory and improve performance
by preventing the GC to kick in at less convenient times.

See: https://developer.mozilla.org/en/SpiderMonkey/JSAPI_Reference/JS_MaybeGC

Created attachment 157804 [details] [review]
Call global.maybe_gc when idle

Call shell_global_maybe_gc when idle to free up memory and improve performance
by preventing the GC to kick in at less convenient times.

See: https://developer.mozilla.org/en/SpiderMonkey/JSAPI_Reference/JS_MaybeGC

How would you go about measuring if this is doing any good?

(In reply to comment #4)
> How would you go about measuring if this is doing any good?

I did using Colin' systemtap scripts.

When going to the overview and coming back the number of gjs proxies kept growing.

I failed to find any leaks when reading the code (I had the StThemeNode fix applied), so I tried calling global.gc() in overview._hideDone() and this fixed the problem (i.e the JS proxies where gone when leaving the overview) and memory usage did not increase every time when going to the overview.

We talked on IRC about it and we agreed that global.gc() is to expensive to be called in a situation like this. I searched for an alternative and found JS_MaybeGC which seems better suited for this kind of usage.

With this two patches applied the proxies do not go away immediately after leaving the overview but they do not accumulate overtime.

Review of attachment 157800 [details] [review]:

Looks fine

Review of attachment 157804 [details] [review]:

::: js/ui/main.js
@@ +165,3 @@
+     * Call maybe_gc on idle to free up memory and prevent it from being
+    /*
+

I don't like this, it forces mutter to be scheduled even if we're totally idle.

Slightly better would be to say queue a timeout for some time (10s? 30s?) after an X event (if one isn't scheduled already).

(In reply to comment #7)
> Review of attachment 157804 [details] [review]:
> 
> ::: js/ui/main.js
> @@ +165,3 @@
> +     * Call maybe_gc on idle to free up memory and prevent it from being
> +    /*
> +
> 
> I don't like this, it forces mutter to be scheduled even if we're totally idle.
> 
> Slightly better would be to say queue a timeout for some time (10s? 30s?) after
> an X event (if one isn't scheduled already).

Yeah I am not really sure when we should call this without waking up unnecessarily.

Created attachment 161635 [details] [review]
Call shell_global_maybe_gc() when no work is being done

Call shell_global_maybe_gc in shell_global_end_work() when no other work
is ongoing to free up memory and improve performance by preventing the GC
from kicking in at less convenient times.

See: https://developer.mozilla.org/en/SpiderMonkey/JSAPI_Reference/JS_MaybeGC

Review of attachment 161635 [details] [review]:

::: src/shell-global.c
@@ +1554,3 @@
+  if (global->work_count == 0)
+    {
+      shell_global_maybe_gc (global);

You definitely want to do this in the idle instead of right here - the work count going to zero doesn't mean that we are at leisure, it just means that we could be at leisure if there isn't other stuff going on.

Created attachment 161679 [details] [review]
Call shell_global_maybe_gc() when no work is being done

*) Do the gc call in idle rather than directly in 
shell_global_end_work

Review of attachment 161679 [details] [review]:

What I meant was:

===
static gboolean
run_leisure_functions (gpointer data)
{
  ShellGlobal *global = data;
  GSList *closures;
  GSList *iter;

  global->leisure_function_id = 0;

  /* We started more work since we scheduled the idle */
  if (global->work_count > 0)
    return FALSE;

>>> YOUR STUFF GOES HERE

  closures = global->leisure_closures;
====

(In reply to comment #12)
> Review of attachment 161679 [details] [review]:
> 
> What I meant was:
> 
> ===
> static gboolean
> run_leisure_functions (gpointer data)
> {
>   ShellGlobal *global = data;
>   GSList *closures;
>   GSList *iter;
> 
>   global->leisure_function_id = 0;
> 
>   /* We started more work since we scheduled the idle */
>   if (global->work_count > 0)
>     return FALSE;
> 
> >>> YOUR STUFF GOES HERE
> 
>   closures = global->leisure_closures;
> ====

Well this would only be called when leisure functions are registered, no?

This is called by schedule_leisure_functions when global->leisure_closures is not NULL ... so it wouldn't quite do what I intend to do with it (it shouldn't run only when collecting statistics).

(In reply to comment #13)
> (In reply to comment #12)
> > Review of attachment 161679 [details] [review] [details]:
> > 
> > What I meant was:
> > 
> > ===
> > static gboolean
> > run_leisure_functions (gpointer data)
> > {
> >   ShellGlobal *global = data;
> >   GSList *closures;
> >   GSList *iter;
> > 
> >   global->leisure_function_id = 0;
> > 
> >   /* We started more work since we scheduled the idle */
> >   if (global->work_count > 0)
> >     return FALSE;
> > 
> > >>> YOUR STUFF GOES HERE
> > 
> >   closures = global->leisure_closures;
> > ====
> 
> Well this would only be called when leisure functions are registered, no?
> 
> This is called by schedule_leisure_functions when global->leisure_closures is
> not NULL ... so it wouldn't quite do what I intend to do with it (it shouldn't
> run only when collecting statistics).

If you want to do this always, leisure functions or not, then just make the code do that, rather than adding a duplicate idle function.

(In reply to comment #14)
> (In reply to comment #13)
> > (In reply to comment #12)
> [..]
> If you want to do this always, leisure functions or not, then just make the
> code do that, rather than adding a duplicate idle function.

This kind of makes sense ...

Created attachment 161685 [details] [review]
Call shell_global_maybe_gc() when no work is being done

Do the call in run_leisure_functions, and change the code to always
call it when there is no work left.

Review of attachment 161685 [details] [review]:

OK, this looks good ("prevent to run" => "prevent from running" in a comment.)

I'm not completely sure I understand how this fits in to the big picture of garbage collection, and I haven't seen any problem with GC pauses, but let's try it this way and see how it goes.

Attachment 161685 [details] pushed as ce3f003 - Call shell_global_maybe_gc() when no work is being done