Bug 84763 – GC Activity Callback timer should be based on how much has been allocated since the last collection

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Mark Hahnenberg]

The desired behavior for the GC timer is to collect at some point in the future, regardless of how little we've allocated. A secondary goal, which is almost if not as important, is for the timer to collect sooner if there is the potential to collect a greater amount of memory. Conversely, as we allocate more memory we'd like to reduce the delay to the next collection. If we're allocating quickly enough, the timer should be preempted in favor of a normal allocation-triggered collection. If allocation were to slow or stop, we'd like the timer to be able to opportunistically run a collection without us having to allocate to the hard limit set by the Heap.

This type of policy can be described in terms of the amount of CPU we are willing to dedicate to reclaim a single MB of memory. For example, we might be willing to dedicate 1% of our CPU to reclaim 1 MB. We base our CPU usage off of the length of the last collection, e.g. if our last collection took 1ms, we would want to wait about 100ms before running another collection to reclaim 1 MB. These constants should be tune-able, e.g. 0.1% CPU = 1 MB vs. 1% CPU = 1 MB vs. 10% CPU = 1 MB.

[Mark Hahnenberg]

Created attachment 138638 [details]
Benchmark results for 10% CPU = 1 MB

[Mark Hahnenberg]

Created attachment 138675 [details]
Benchmark results for 0.1%, 1%, and 10%

[Mark Hahnenberg]

Created attachment 138689 [details]
Benchmark results with --measure-gc

[Mark Hahnenberg]

Created attachment 138708 [details]
Patch

[Geoffrey Garen]

Comment on attachment 138708 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=138708&action=review

> Source/JavaScriptCore/runtime/GCActivityCallbackCF.cpp:58
> +const double timerSlop = 2.0; // Fudge factor to avoid performance cost resetting timer.

Typo: missing "of" before "resetting".

> Source/JavaScriptCore/runtime/GCActivityCallbackCF.cpp:119
> +    double gcTimeSlice = std::max((static_cast<double>(bytes) / MB) * gcTimeSlicePerMB, maxGCTimeSlice);

Needs to be min().

[Mark Hahnenberg]

Created attachment 138713 [details]
Patch

[Geoffrey Garen]

Comment on attachment 138713 [details]
Patch

r=me

[Mark Hahnenberg]

Committed r115156: <http://trac.webkit.org/changeset/115156>

[Geoffrey Garen]

<rdar://problem/9725299>

[Benjamin Poulain]

Could this have caused the regression of Dromaeo/jslib-event-prototype?

See http://webkit-perf.appspot.com/graph.html#tests=[[43017,2001,32196]]&sel=1335173610906.9148,1335421909212,1283.893395133256,1793.7427578215527&displayrange=7&datatype=running

The changeset of this regression is http://trac.webkit.org/log/?rev=115156&stop_rev=115149&verbose=on

[Mark Hahnenberg]

(In reply to comment #10)
> Could this have caused the regression of Dromaeo/jslib-event-prototype?
> 
> See http://webkit-perf.appspot.com/graph.html#tests=[[43017,2001,32196]]&sel=1335173610906.9148,1335421909212,1283.893395133256,1793.7427578215527&displayrange=7&datatype=running
> 
> The changeset of this regression is http://trac.webkit.org/log/?rev=115156&stop_rev=115149&verbose=on

I'm confused by the graph on that page. What exactly is the y-axis? It appears to be doing more runs/second, which would be a progression, no? I feel like I'm missing something...

[Benjamin Poulain]

> I'm confused by the graph on that page. What exactly is the y-axis? It appears to be doing more runs/second, which would be a progression, no? I feel like I'm missing something...

You are right. It is a test from Dromaeo.

Most bench are the other way around, in milliseconds. I was going over the regressions and I did not notice that particular one is Dromaeo.

Sorry for the wrong warning. Congrats on the progression! :)

[Mark Hahnenberg]

(In reply to comment #12)
> > I'm confused by the graph on that page. What exactly is the y-axis? It appears to be doing more runs/second, which would be a progression, no? I feel like I'm missing something...
> 
> You are right. It is a test from Dromaeo.
> 
> Most bench are the other way around, in milliseconds. I was going over the regressions and I did not notice that particular one is Dromaeo.
> 
> Sorry for the wrong warning. Congrats on the progression! :)

Thanks! I'm glad somebody is looking at the perf bot :-) Looks like a few of the other Dromaeo benchmarks had a decent progression around that time as well.

[Geoffrey Garen]

Nice! Dromaeo wins on:

http://webkit-perf.appspot.com/graph.html#tests=[[45011,2001,32196]]&sel=none&displayrange=7&datatype=running
http://webkit-perf.appspot.com/graph.html#tests=[[43017,2001,32196]]&sel=none&displayrange=7&datatype=running
http://webkit-perf.appspot.com/graph.html#tests=[[45016,2001,32196]]&sel=none&displayrange=7&datatype=running
http://webkit-perf.appspot.com/graph.html#tests=[[39020,2001,32196]]&sel=none&displayrange=7&datatype=running
http://webkit-perf.appspot.com/graph.html#tests=[[39019,2001,32196]]&sel=none&displayrange=7&datatype=running