Bug 103120 - Web Inspector: Calculate "idle" time for CPU profiles.
Summary: Web Inspector: Calculate "idle" time for CPU profiles.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Eugene Klyuchnikov
URL:
Keywords:
Depends on: 103550
Blocks: 88446
  Show dependency treegraph
 
Reported: 2012-11-23 03:48 PST by Eugene Klyuchnikov
Modified: 2012-12-20 00:44 PST (History)
15 users (show)

See Also:


Attachments
Patch (21.55 KB, patch)
2012-11-23 04:07 PST, Eugene Klyuchnikov
no flags Details | Formatted Diff | Diff
Patch (12.47 KB, patch)
2012-11-29 17:46 PST, Eugene Klyuchnikov
no flags Details | Formatted Diff | Diff
Patch (16.34 KB, patch)
2012-12-10 22:45 PST, Eugene Klyuchnikov
pfeldman: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eugene Klyuchnikov 2012-11-23 03:48:53 PST
CPU profiles contain a very confusing "(program)" item.
It is time when there is no JS stack.
That could be either idle time, or time when browser is doing something.

To split "(program)" item to idle and really program time we are to
calculate idle time during profiling.
Comment 1 Eugene Klyuchnikov 2012-11-23 04:07:29 PST
Created attachment 175774 [details]
Patch
Comment 2 Andrey Kosyakov 2012-11-28 11:59:52 PST
Comment on attachment 175774 [details]
Patch

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

Mostly good, modulo the fact that we pollute ScriptProfile binding and InspectorProfilerAgent with a logic that is specific to v8's implementation of profiler.

> Source/WebCore/inspector/InspectorController.cpp:121
> +        , inspectorClient));

please revert these 2 lines

> Source/WebCore/inspector/InspectorProfilerAgent.cpp:413
> +        profile->setIdleTime(m_idleTime * 1000);

Is there a particular reason to pass time in milliseconds here?

> Source/WebCore/inspector/InspectorProfilerAgent.h:136
> +    double m_taskEndTime;

m_previousTaskEndTime?
Comment 3 Eugene Klyuchnikov 2012-11-29 17:38:57 PST
Comment on attachment 175774 [details]
Patch

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

>> Source/WebCore/inspector/InspectorController.cpp:121
>> +        , inspectorClient));
> 
> please revert these 2 lines

Done

>> Source/WebCore/inspector/InspectorProfilerAgent.cpp:413
>> +        profile->setIdleTime(m_idleTime * 1000);
> 
> Is there a particular reason to pass time in milliseconds here?

That's because all other values in profile are also measured in ms.

>> Source/WebCore/inspector/InspectorProfilerAgent.h:136
>> +    double m_taskEndTime;
> 
> m_previousTaskEndTime?

Done
Comment 4 Eugene Klyuchnikov 2012-11-29 17:46:56 PST
Created attachment 176866 [details]
Patch
Comment 5 Pavel Feldman 2012-11-29 20:41:09 PST
Comment on attachment 176866 [details]
Patch

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

> Source/WebCore/bindings/js/ScriptProfile.cpp:80
> +    return 0.0;

You should provide the same implementation for JSC as you provide for v8.

> Source/WebCore/inspector/InspectorProfilerAgent.cpp:388
> +    m_idleTime = 0.0;

There is a way to record nested profiles using console.profile / console.profileEnd. So you might need to have an idle time per profile. I know this makes things more complex.

> Source/WebCore/inspector/ScriptProfile.idl:36
> +    readonly attribute double idleTime;

Yury, do we need to change V8ScriptProfile.cpp for it to be exposed on web-facing object in v8?
Comment 6 Yury Semikhatsky 2012-11-30 09:33:22 PST
Comment on attachment 176866 [details]
Patch

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

>> Source/WebCore/inspector/ScriptProfile.idl:36
>> +    readonly attribute double idleTime;
> 
> Yury, do we need to change V8ScriptProfile.cpp for it to be exposed on web-facing object in v8?

No, it is generated from .idl.
Comment 7 Eugene Klyuchnikov 2012-12-10 22:45:05 PST
Created attachment 178724 [details]
Patch
Comment 8 Yury Semikhatsky 2012-12-14 00:51:50 PST
Comment on attachment 178724 [details]
Patch

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

> Source/WebCore/bindings/v8/ScriptProfiler.cpp:264
> +    AtomicallyInitializedStatic(WTF::ThreadSpecific<ProfileNameIdleTimeMap>*, map = new WTF::ThreadSpecific<ProfileNameIdleTimeMap>);

You could make do without ThreadSpecific: in case of PageProfilerAgent the map can be a static variable and WorkerProfilerAgent can own one for the worker context which is currently one per worker thread.
Comment 9 Andrey Kosyakov 2012-12-18 03:45:15 PST
Committed r138004: <http://trac.webkit.org/changeset/138004>