Bug 81200 – [V8] V8GCForContextDispose should indicate whether a context is disposed for the main frame or not

• Component: WebCore JavaScript
• Status: RESOLVED FIXED

[Ulan Degenbaev]

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.

[Ulan Degenbaev]

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.

[Ulan Degenbaev]

We are rolling the CL that this patch depends on to Chromium:
https://chromiumcodereview.appspot.com/9701093/

[Ulan Degenbaev]

Created attachment 133020 [details]
Rebase and add a check for m_frame->page()

[Ulan Degenbaev]

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().

[Nate Chapin]

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.

[Ulan Degenbaev]

Created attachment 133053 [details]
Rename constants.

[WebKit Review Bot]

Comment on attachment 133053 [details]
Rename constants.

Attachment 133053 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12035555

[Ulan Degenbaev]

Created attachment 133061 [details]
fix compile error

[Ulan Degenbaev]

Created attachment 133066 [details]
Rename constants.

[Adam Barth]

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.

[Ulan Degenbaev]

Created attachment 133070 [details]
rename m_contextDisposedForMainFrame and fix comma placement

Thanks for reviewing!

[Adam Barth]

Comment on attachment 133070 [details]
rename m_contextDisposedForMainFrame and fix comma placement

Thanks.

[WebKit Review Bot]

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>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.