Bug 103120 – Web Inspector: Calculate "idle" time for CPU profiles.

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Eugene Klyuchnikov]

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.

[Eugene Klyuchnikov]

Created attachment 175774 [details]
Patch

[Andrey Kosyakov]

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?

[Eugene Klyuchnikov]

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

[Eugene Klyuchnikov]

Created attachment 176866 [details]
Patch

[Pavel Feldman]

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?

[Yury Semikhatsky]

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.

[Eugene Klyuchnikov]

Created attachment 178724 [details]
Patch

[Yury Semikhatsky]

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.

[Andrey Kosyakov]

Committed r138004: <http://trac.webkit.org/changeset/138004>