Bug 136796 - Web Inspector: subtract elapsed time while debugger is paused from profile nodes
Summary: Web Inspector: subtract elapsed time while debugger is paused from profile nodes
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: 136352
Blocks: WebReplayUI
  Show dependency treegraph
 
Reported: 2014-09-12 17:37 PDT by Brian Burg
Modified: 2014-09-24 17:25 PDT (History)
5 users (show)

See Also:


Attachments
Patch (10.51 KB, patch)
2014-09-14 01:47 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-09-12 17:37:52 PDT
Currently it will zero out any ProfileNode::Call where the debugger paused further down in the call stack. Instead, we should subtract the elapsed paused time.
Comment 1 Radar WebKit Bug Importer 2014-09-12 17:38:01 PDT
<rdar://problem/18327309>
Comment 2 Brian Burg 2014-09-14 01:47:16 PDT
Created attachment 238091 [details]
Patch
Comment 3 WebKit Commit Bot 2014-09-14 01:49:22 PDT
Attachment 238091 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/profiler/ProfileGenerator.cpp:51:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Timothy Hatcher 2014-09-16 22:10:25 PDT
Comment on attachment 238091 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:13
> +        Time for a node may elapse in a non-contiguous fashion depending on the interleaving of
> +        didPause, didContinue, willExecute, and didExecute. A node's start time is set to the
> +        start of the last such interval that accrues elapsed time.

This sounds like it could be confusing in the Timeline data grid. Maybe once TimelineRecords account for pausing it will be fine? Why does start time need to move?

If the profile data was visualized, I'd expect to see start time stay put and end time stay put, but there would be gaps where the debugger paused and resumed. That would require more complex data in ProfileNode::Call.
Comment 5 Timothy Hatcher 2014-09-16 22:13:10 PDT
Comment on attachment 238091 [details]
Patch

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

>> Source/JavaScriptCore/ChangeLog:13
>> +        start of the last such interval that accrues elapsed time.
> 
> This sounds like it could be confusing in the Timeline data grid. Maybe once TimelineRecords account for pausing it will be fine? Why does start time need to move?
> 
> If the profile data was visualized, I'd expect to see start time stay put and end time stay put, but there would be gaps where the debugger paused and resumed. That would require more complex data in ProfileNode::Call.

The "more complex data" likely just needs to be a vector of debugger pause and resume timestamp pairs. It could even be global to the profile and the front end uses it when displaying the data or visuals.
Comment 6 Brian Burg 2014-09-16 22:36:56 PDT
(In reply to comment #4)
> (From update of attachment 238091 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=238091&action=review
> 
> > Source/JavaScriptCore/ChangeLog:13
> > +        Time for a node may elapse in a non-contiguous fashion depending on the interleaving of
> > +        didPause, didContinue, willExecute, and didExecute. A node's start time is set to the
> > +        start of the last such interval that accrues elapsed time.
> 
> This sounds like it could be confusing in the Timeline data grid. Maybe once TimelineRecords account for pausing it will be fine? Why does start time need to move?
> 
> If the profile data was visualized, I'd expect to see start time stay put and end time stay put, but there would be gaps where the debugger paused and resumed. That would require more complex data in ProfileNode::Call.

I think it's worth clarifying: these proposed changes (and others for Timeline records) are moving away from wall-clock time and towards elapsed execution time (of which, time paused in the debugger does not count towards). IMO, the current wall-clock approach---including paused time in profile nodes and timeline records---is almost always going to make the timeline overview unusable. And for replay recordings, a user will probably pause the debugger at different places on different playbacks. It would be nice if the timeline looked more or less the same for all playbacks, otherwise it's not a very good navigation tool.
Comment 7 Brian Burg 2014-09-24 17:25:36 PDT
Committed r173939: <http://trac.webkit.org/changeset/173939>