Bug 81200 - [V8] V8GCForContextDispose should indicate whether a context is disposed for the main frame or not
Summary: [V8] V8GCForContextDispose should indicate whether a context is disposed for ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ulan Degenbaev
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-03-15 02:17 PDT by Ulan Degenbaev
Modified: 2012-03-21 11:54 PDT (History)
7 users (show)

See Also:

Attachments
patch (4.02 KB, patch)
2012-03-16 06:31 PDT, Ulan Degenbaev 		no flags Details | Formatted Diff | Diff
Rebase and add a check for m_frame->page() (4.04 KB, patch)
2012-03-21 05:25 PDT, Ulan Degenbaev 		japhet: review+
Details | Formatted Diff | Diff
Rename constants. (4.06 KB, patch)
2012-03-21 09:08 PDT, Ulan Degenbaev 		webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
fix compile error (4.04 KB, patch)
2012-03-21 09:37 PDT, Ulan Degenbaev 		no flags Details | Formatted Diff | Diff
Rename constants. (4.00 KB, patch)
2012-03-21 10:09 PDT, Ulan Degenbaev 		abarth: review+
abarth: commit-queue-
Details | Formatted Diff | Diff
rename m_contextDisposedForMainFrame and fix comma placement (3.99 KB, patch)
2012-03-21 10:25 PDT, Ulan Degenbaev 		no flags Details | Formatted Diff | Diff
Show Obsolete (5) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
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.