Bug 84763 - GC Activity Callback timer should be based on how much has been allocated since the last collection
Summary: GC Activity Callback timer should be based on how much has been allocated sin...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Hahnenberg
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-04-24 14:06 PDT by Mark Hahnenberg
Modified: 2012-04-26 17:22 PDT (History)
2 users (show)

See Also:


Attachments
Benchmark results for 10% CPU = 1 MB (14.33 KB, text/plain)
2012-04-24 14:08 PDT, Mark Hahnenberg
no flags Details
Benchmark results for 0.1%, 1%, and 10% (21.19 KB, text/plain)
2012-04-24 15:52 PDT, Mark Hahnenberg
no flags Details
Benchmark results with --measure-gc (21.03 KB, text/plain)
2012-04-24 16:52 PDT, Mark Hahnenberg
no flags Details
Patch (13.01 KB, patch)
2012-04-24 18:15 PDT, Mark Hahnenberg
no flags Details | Formatted Diff | Diff
Patch (13.01 KB, patch)
2012-04-24 18:22 PDT, Mark Hahnenberg
ggaren: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Hahnenberg 2012-04-24 14:06:05 PDT
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.
Comment 1 Mark Hahnenberg 2012-04-24 14:08:08 PDT
Created attachment 138638 [details]
Benchmark results for 10% CPU = 1 MB
Comment 2 Mark Hahnenberg 2012-04-24 15:52:08 PDT
Created attachment 138675 [details]
Benchmark results for 0.1%, 1%, and 10%
Comment 3 Mark Hahnenberg 2012-04-24 16:52:55 PDT
Created attachment 138689 [details]
Benchmark results with --measure-gc
Comment 4 Mark Hahnenberg 2012-04-24 18:15:38 PDT
Created attachment 138708 [details]
Patch
Comment 5 Geoffrey Garen 2012-04-24 18:20:17 PDT
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().
Comment 6 Mark Hahnenberg 2012-04-24 18:22:36 PDT
Created attachment 138713 [details]
Patch
Comment 7 Geoffrey Garen 2012-04-24 18:24:01 PDT
Comment on attachment 138713 [details]
Patch

r=me
Comment 8 Mark Hahnenberg 2012-04-24 18:29:05 PDT
Committed r115156: <http://trac.webkit.org/changeset/115156>
Comment 9 Geoffrey Garen 2012-04-24 18:33:25 PDT
<rdar://problem/9725299>
Comment 11 Mark Hahnenberg 2012-04-26 07:35:22 PDT
(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...
Comment 12 Benjamin Poulain 2012-04-26 10:13:10 PDT
> 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! :)
Comment 13 Mark Hahnenberg 2012-04-26 10:16:31 PDT
(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.