WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
136352
Web Inspector: the profiler should not accrue time to nodes while the debugger is paused
https://bugs.webkit.org/show_bug.cgi?id=136352
Summary
Web Inspector: the profiler should not accrue time to nodes while the debugge...
Brian Burg
Reported
2014-08-28 15:08:52 PDT
It's confusing.
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
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1
2014-08-28 15:09:01 PDT
<
rdar://problem/18167563
>
Brian Burg
Comment 2
2014-08-29 11:20:03 PDT
Created
attachment 237361
[details]
WIP: add hooks
Brian Burg
Comment 3
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.
Timothy Hatcher
Comment 4
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?
Brian Burg
Comment 5
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.
Brian Burg
Comment 6
2014-09-04 10:21:49 PDT
Committed
r173264
: <
http://trac.webkit.org/changeset/173264
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug