Summary: | Web Inspector: Calculate "idle" time for CPU profiles. | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Eugene Klyuchnikov <eustas> | ||||||||
Component: | Web Inspector (Deprecated) | Assignee: | Eugene Klyuchnikov <eustas> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | abarth, apavlov, caseq, eustas, haraken, japhet, keishi, levin+threading, loislo, pfeldman, pmuellr, vsevik, web-inspector-bugs, webkit.review.bot, yurys | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | All | ||||||||||
OS: | All | ||||||||||
Bug Depends on: | 103550 | ||||||||||
Bug Blocks: | 88446 | ||||||||||
Attachments: |
|
Description
Eugene Klyuchnikov
2012-11-23 03:48:53 PST
Created attachment 175774 [details]
Patch
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 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 Created attachment 176866 [details]
Patch
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 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. Created attachment 178724 [details]
Patch
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. Committed r138004: <http://trac.webkit.org/changeset/138004> |