Bug 136352 - Web Inspector: the profiler should not accrue time to nodes while the debugger is paused
Summary: Web Inspector: the profiler should not accrue time to nodes while the debugge...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Brian Burg
URL:
Keywords: InRadar
Depends on: 136380 136381
Blocks: WebReplayUI 136796
  Show dependency treegraph
 
Reported: 2014-08-28 15:08 PDT by Brian Burg
Modified: 2014-09-12 17:37 PDT (History)
4 users (show)

See Also:


Attachments
WIP: add hooks (11.57 KB, patch)
2014-08-29 11:20 PDT, Brian Burg
no flags Details | Formatted Diff | Diff
Proposed fix (12.68 KB, patch)
2014-09-02 00:15 PDT, Brian Burg
timothy: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brian Burg 2014-08-28 15:08:52 PDT
It's confusing.
Comment 1 Radar WebKit Bug Importer 2014-08-28 15:09:01 PDT
<rdar://problem/18167563>
Comment 2 Brian Burg 2014-08-29 11:20:03 PDT
Created attachment 237361 [details]
WIP: add hooks
Comment 3 Brian Burg 2014-09-02 00:15:09 PDT
Created attachment 237477 [details]
Proposed fix

Patch depends on 136381 and 136380. It fixes profile data, but a similar fix needs to happen for TimelineRecords, which independently record their own start/stop times.
Comment 4 Timothy Hatcher 2014-09-02 11:08:40 PDT
Comment on attachment 237477 [details]
Proposed fix

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

> Source/JavaScriptCore/profiler/LegacyProfiler.cpp:141
> +    callFunctionForProfilesWithGroup(std::bind(&ProfileGenerator::willExecute, std::placeholders::_1, callerCallFrame, callIdentifier), m_currentProfiles, callerCallFrame->lexicalGlobalObject()->profileGroup());

std::bind + std::placeholders === magic

> Source/JavaScriptCore/profiler/ProfileGenerator.cpp:131
> +    last.setTotalTime(m_debuggerPaused ? 0.0 : currentTime() - last.startTime());

Couldn't we subtract out the time between pause and resume?
Comment 5 Brian Burg 2014-09-02 11:16:30 PDT
Comment on attachment 237477 [details]
Proposed fix

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

>> Source/JavaScriptCore/profiler/LegacyProfiler.cpp:141
>> +    callFunctionForProfilesWithGroup(std::bind(&ProfileGenerator::willExecute, std::placeholders::_1, callerCallFrame, callIdentifier), m_currentProfiles, callerCallFrame->lexicalGlobalObject()->profileGroup());
> 
> std::bind + std::placeholders === magic

Yeah, it's almost like JavaScript. It can get you in trouble if you copy big things by accident. In this case (AFAIK), it will copy the arguments (a bunch of pointers and scalars) into a stuct on the stack.

>> Source/JavaScriptCore/profiler/ProfileGenerator.cpp:131
>> +    last.setTotalTime(m_debuggerPaused ? 0.0 : currentTime() - last.startTime());
> 
> Couldn't we subtract out the time between pause and resume?

Yeah, it could do something like that, or reset the start time of all active nodes when didContinue is received.
I would have to examine the order of operations more closely, but when I wrote this patch I never hit a
breakpoint inside didPause- the debugger was already paused by the time the profiler got the willExecute.
Maybe I need to set the breakpoint so it will pause with a bigger call stack.

I have no quick way of testing a smarter approach because the timeline records still have wrong times
and those seem to be used as the timeline row's "overall" time.
Comment 6 Brian Burg 2014-09-04 10:21:49 PDT
Committed r173264: <http://trac.webkit.org/changeset/173264>