At the moment V8GCForContextDispose is sending idle notifications for each context disposal. If a website creates and destroys a lot of iframes, then this causes a lot of GC in V8 and degrades performance. V8 could make better GC decisions on idle notification if V8GCForContextDispose would provide a hint.
Created attachment 132269 [details] patch This depends on this V8 CL https://chromiumcodereview.appspot.com/9701093/ and should be landed after V8 with the CL rolls into Chromium.
Change looks good to me.
We are rolling the CL that this patch depends on to Chromium: https://chromiumcodereview.appspot.com/9701093/
Created attachment 133020 [details] Rebase and add a check for m_frame->page()
The corresponding V8 CL is in Chromium and is sticking: http://src.chromium.org/viewvc/chrome?view=rev&revision=127940 So this patch can now be committed. Adam, Kentaro, Nate: could someone of you review and commit the patch? Michael from the V8 team reviewed the part where we call V8::IdleNotification().
Comment on attachment 133020 [details] Rebase and add a check for m_frame->page() View in context: https://bugs.webkit.org/attachment.cgi?id=133020&action=review LGTM with a style nit. > Source/WebCore/bindings/v8/V8GCForContextDispose.cpp:72 > + const int kLongIdlePauseInMs = 1000; > + const int kShortIdlePauseInMs = 10; We don't typically prefix constants with k. So just longIdlePauseInMS and shortIdlePauseInMs.
Created attachment 133053 [details] Rename constants.
Comment on attachment 133053 [details] Rename constants. Attachment 133053 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12035555
Created attachment 133061 [details] fix compile error
Created attachment 133066 [details] Rename constants.
Comment on attachment 133066 [details] Rename constants. View in context: https://bugs.webkit.org/attachment.cgi?id=133066&action=review > Source/WebCore/bindings/v8/V8GCForContextDispose.cpp:40 > + : m_pseudoIdleTimer(this, &V8GCForContextDispose::pseudoIdleTimerFired), > + m_contextDisposedForMainFrame(false) The comma goes on line 40, just below the : > Source/WebCore/bindings/v8/V8GCForContextDispose.cpp:46 > + m_contextDisposedForMainFrame = m_contextDisposedForMainFrame || isMainFrame; Sorry to nit-pick you names, but consider a name like m_didDisposeContextForMainFrame. This is an Objective-C style name, which we often use in WebKit because of the strong Mac influence on the project.
Created attachment 133070 [details] rename m_contextDisposedForMainFrame and fix comma placement Thanks for reviewing!
Comment on attachment 133070 [details] rename m_contextDisposedForMainFrame and fix comma placement Thanks.
Comment on attachment 133070 [details] rename m_contextDisposedForMainFrame and fix comma placement Clearing flags on attachment: 133070 Committed r111586: <http://trac.webkit.org/changeset/111586>
All reviewed patches have been landed. Closing bug.