Bug 81200

Summary: [V8] V8GCForContextDispose should indicate whether a context is disposed for the main frame or not
Product: WebKit Reporter: Ulan Degenbaev <ulan>
Component: WebCore JavaScriptAssignee: Ulan Degenbaev <ulan>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, dglazkov, erikcorry, haraken, japhet, mstarzinger, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
patch
none
Rebase and add a check for m_frame->page()
japhet: review+
Rename constants.
webkit.review.bot: commit-queue-
fix compile error
none
Rename constants.
abarth: review+, abarth: commit-queue-
rename m_contextDisposedForMainFrame and fix comma placement none

Description Ulan Degenbaev 2012-03-15 02:17:34 PDT
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.
Comment 1 Ulan Degenbaev 2012-03-16 06:31:20 PDT
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.
Comment 2 mstarzinger 2012-03-21 01:54:18 PDT
Change looks good to me.
Comment 3 Ulan Degenbaev 2012-03-21 03:17:02 PDT
We are rolling the CL that this patch depends on to Chromium:
https://chromiumcodereview.appspot.com/9701093/
Comment 4 Ulan Degenbaev 2012-03-21 05:25:54 PDT
Created attachment 133020 [details]
Rebase and add a check for m_frame->page()
Comment 5 Ulan Degenbaev 2012-03-21 08:56:01 PDT
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 6 Nate Chapin 2012-03-21 09:01:48 PDT
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.
Comment 7 Ulan Degenbaev 2012-03-21 09:08:11 PDT
Created attachment 133053 [details]
Rename constants.
Comment 8 WebKit Review Bot 2012-03-21 09:13:57 PDT
Comment on attachment 133053 [details]
Rename constants.

Attachment 133053 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12035555
Comment 9 Ulan Degenbaev 2012-03-21 09:37:19 PDT
Created attachment 133061 [details]
fix compile error
Comment 10 Ulan Degenbaev 2012-03-21 10:09:48 PDT
Created attachment 133066 [details]
Rename constants.
Comment 11 Adam Barth 2012-03-21 10:14:43 PDT
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.
Comment 12 Ulan Degenbaev 2012-03-21 10:25:29 PDT
Created attachment 133070 [details]
rename m_contextDisposedForMainFrame and fix comma placement

Thanks for reviewing!
Comment 13 Adam Barth 2012-03-21 10:51:41 PDT
Comment on attachment 133070 [details]
rename m_contextDisposedForMainFrame and fix comma placement

Thanks.
Comment 14 WebKit Review Bot 2012-03-21 11:54:34 PDT
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>
Comment 15 WebKit Review Bot 2012-03-21 11:54:40 PDT
All reviewed patches have been landed.  Closing bug.