Bug 100358 – [V8] Add histograms to measure V8 work done during window close and navigation

• Component: WebCore JavaScript
• Status: RESOLVED FIXED

[Ulan Degenbaev]

We need to track V8DOMWindowShell::clearForNavigation time to be able to quickly detect such performance issues as http://code.google.com/p/chromium/issues/detail?id=155270

[Ojan Vafai]

Looking at the callers, I wonder if we actually want to instrument ScriptController::clearForNavigation. That seems to cover more of the code that we call pre navigating. I don't know this code at all though.

[Ojan Vafai]

Digging a bit further, it looks like V8DOMWindowShell::clearForNavigation is only ever called by ScriptController::clearForNavigation, which is only ever called by ScriptController::clearWindowShell. Can we kill ScriptController::clearForNavigation?

[Ojan Vafai]

Oh, I see. There's also a clearForClose. I suppose it's better self-documenting code?

[Ojan Vafai]

Created attachment 170685 [details]
Patch

[Adam Barth]

Comment on attachment 170685 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=170685&action=review

> Source/WebCore/bindings/v8/ScriptController.cpp:119
> +    WebKit::Platform::current()->histogramCustomCounts("ScriptController::~ScriptController", (currentTime() - start) * 1000, 0, 10000, 50);

We have helper functions in HistogramSupport.h so that other ports can add their histogram backends if they desire.

[Adam Barth]

Comment on attachment 170685 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=170685&action=review

> Source/WebCore/bindings/v8/ScriptController.cpp:162
> +    WebKit::Platform::current()->histogramCustomCounts("ScriptController::reset", (currentTime() - start) * 1000, 0, 10000, 50);

Is reset called from places other than clearForClose and clearWindowShell?  It's a bit strange that you'll have several of these timers running at once.  For example, in ~ScriptController, it looks like you'll have three running.

[Ojan Vafai]

Comment on attachment 170685 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=170685&action=review

>> Source/WebCore/bindings/v8/ScriptController.cpp:119
>> +    WebKit::Platform::current()->histogramCustomCounts("ScriptController::~ScriptController", (currentTime() - start) * 1000, 0, 10000, 50);
> 
> We have helper functions in HistogramSupport.h so that other ports can add their histogram backends if they desire.

Oh right. I'm a bit braindead this morning clearly.

>> Source/WebCore/bindings/v8/ScriptController.cpp:162
>> +    WebKit::Platform::current()->histogramCustomCounts("ScriptController::reset", (currentTime() - start) * 1000, 0, 10000, 50);
> 
> Is reset called from places other than clearForClose and clearWindowShell?  It's a bit strange that you'll have several of these timers running at once.  For example, in ~ScriptController, it looks like you'll have three running.

It's true. I could drop the reset one for now as it is a little redundant. Should I drop ~ScriptController too? The only extra thing that really measures is windowShell()->destroyGlobal().

[Ojan Vafai]

(In reply to comment #7)
> (From update of attachment 170685 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=170685&action=review
> 
> >> Source/WebCore/bindings/v8/ScriptController.cpp:162
> >> +    WebKit::Platform::current()->histogramCustomCounts("ScriptController::reset", (currentTime() - start) * 1000, 0, 10000, 50);
> > 
> > Is reset called from places other than clearForClose and clearWindowShell?  It's a bit strange that you'll have several of these timers running at once.  For example, in ~ScriptController, it looks like you'll have three running.
> 
> It's true. I could drop the reset one for now as it is a little redundant. Should I drop ~ScriptController too? The only extra thing that really measures is windowShell()->destroyGlobal().

Now that I look, destroyGlobal literally just clears m_global. I'll remove this histogram too.

[Ojan Vafai]

Created attachment 170713 [details]
Patch

[Adam Barth]

Comment on attachment 170713 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=170713&action=review

> Source/WebCore/bindings/v8/ScriptController.cpp:162
> +    HistogramSupport::histogramCustomCounts("ScriptController::clearForClose", (currentTime() - start) * 1000, 0, 10000, 50);

We usually use names more like WebCore.ScriptController.ClearForClose for histograms.

[Ojan Vafai]

Committed r132517: <http://trac.webkit.org/changeset/132517>