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

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.