Bug 158052 - Web Inspector: discontinuous recordings should have discontinuities in the timeline memory graph
Summary: Web Inspector: discontinuous recordings should have discontinuities in the ti...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Matt Baker
URL:
Keywords: InRadar
Depends on:
Blocks: 158195
  Show dependency treegraph
 
Reported: 2016-05-24 20:38 PDT by Brian Burg
Modified: 2016-06-04 14:08 PDT (History)
10 users (show)

See Also:


Attachments
Screenshot (248.88 KB, image/png)
2016-05-24 20:39 PDT, Brian Burg
no flags Details
[Image] New UI (392.02 KB, image/png)
2016-05-28 15:11 PDT, Matt Baker
no flags Details
[Patch] Proposed Fix (16.97 KB, patch)
2016-05-28 16:12 PDT, Matt Baker
no flags Details | Formatted Diff | Diff
[Patch] Proposed Fix (16.59 KB, patch)
2016-05-29 13:48 PDT, Matt Baker
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-yosemite (1.45 MB, application/zip)
2016-05-29 14:59 PDT, Build Bot
no flags Details
[Patch] Proposed Fix (19.39 KB, patch)
2016-06-04 10:51 PDT, Matt Baker
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brian Burg 2016-05-24 20:38:57 PDT
STEPS TO REPRODUCE:

1. Start recording on some page that allocates and frees memory predictably
2. Stop the recording, then do something that frees up memory (without navigating the page)
3. Start the recording again.

EXPECTED:

Like the timeline bars, the memory graph shouldn't render anything during the interval that was not recorded.

ACTUAL:

The line segment doesn't skip, so it appears to interpolate the line between two sides of the gap.
Comment 1 Brian Burg 2016-05-24 20:39:34 PDT
Created attachment 279737 [details]
Screenshot
Comment 2 Matt Baker 2016-05-25 07:51:12 PDT
Tim and I were discussing this the other day. We were planning to wait until we had a better way to show gaps in the recording (a jagged "break" or something similar), but that could be a ways out. I like the idea of showing breaks.
Comment 3 Brian Burg 2016-05-25 08:45:07 PDT
(In reply to comment #2)
> Tim and I were discussing this the other day. We were planning to wait until
> we had a better way to show gaps in the recording (a jagged "break" or
> something similar), but that could be a ways out. I like the idea of showing
> breaks.

Since we don't yet support discontinuity in the ruler (i.e., 1-5s, jaggy, then 10-15s) there's no reason to wait for that to happen. This should be a simple fix of adding zero points to the path for recording stop/restart.
Comment 4 Matt Baker 2016-05-25 09:30:58 PDT
(In reply to comment #3)
> (In reply to comment #2)
> > Tim and I were discussing this the other day. We were planning to wait until
> > we had a better way to show gaps in the recording (a jagged "break" or
> > something similar), but that could be a ways out. I like the idea of showing
> > breaks.
> 
> Since we don't yet support discontinuity in the ruler (i.e., 1-5s, jaggy,
> then 10-15s) there's no reason to wait for that to happen. This should be a
> simple fix of adding zero points to the path for recording stop/restart.

I have it mostly working, will clean up and post later this evening.
Comment 5 Brian Burg 2016-05-27 07:18:21 PDT
Ping for import.
Comment 6 Radar WebKit Bug Importer 2016-05-27 07:22:29 PDT
<rdar://problem/26516695>
Comment 7 Matt Baker 2016-05-28 15:11:23 PDT
Created attachment 280048 [details]
[Image] New UI
Comment 8 Matt Baker 2016-05-28 16:12:44 PDT
Created attachment 280049 [details]
[Patch] Proposed Fix
Comment 9 Brian Burg 2016-05-28 18:54:42 PDT
Comment on attachment 280049 [details]
[Patch] Proposed Fix

I am not awake enough to review. Eventually, I think we may want to create discrete 'segments' in the recording, like audio tracks. That will make it possible to graph the data as (segment offset + record start).
Comment 10 Devin Rousso 2016-05-28 20:28:10 PDT
Comment on attachment 280049 [details]
[Patch] Proposed Fix

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

> Source/WebInspectorUI/UserInterface/Models/TimelineRecording.js:297
> +        for (let discontinuity of this._discontinuities) {

Is there a reason why you couldn't perform a filter operation on this._discontinuities?

return this._discontinuities.filter(item => item.startTime < endTime && item.endTime > startTime);
Comment 11 Matt Baker 2016-05-29 11:56:20 PDT
Comment on attachment 280049 [details]
[Patch] Proposed Fix

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

>> Source/WebInspectorUI/UserInterface/Models/TimelineRecording.js:297
>> +        for (let discontinuity of this._discontinuities) {
> 
> Is there a reason why you couldn't perform a filter operation on this._discontinuities?
> 
> return this._discontinuities.filter(item => item.startTime < endTime && item.endTime > startTime);

Nice style change. Filter is much more concise.
Comment 12 Matt Baker 2016-05-29 12:10:14 PDT
(In reply to comment #9)
> Comment on attachment 280049 [details]
> [Patch] Proposed Fix
> 
> I am not awake enough to review. Eventually, I think we may want to create
> discrete 'segments' in the recording, like audio tracks. That will make it
> possible to graph the data as (segment offset + record start).

Agreed. As it is, we're drawing a single SVG for all segments, resulting in more cumbersome code and a noticeable line running along the x-axis connecting all the segments.
Comment 13 Matt Baker 2016-05-29 13:48:20 PDT
Created attachment 280066 [details]
[Patch] Proposed Fix
Comment 14 Matt Baker 2016-05-29 13:55:27 PDT
(In reply to comment #12)
> (In reply to comment #9)
> > Comment on attachment 280049 [details]
> > [Patch] Proposed Fix
> > 
> > I am not awake enough to review. Eventually, I think we may want to create
> > discrete 'segments' in the recording, like audio tracks. That will make it
> > possible to graph the data as (segment offset + record start).
> 
> Agreed. As it is, we're drawing a single SVG for all segments, resulting in
> more cumbersome code and a noticeable line running along the x-axis
> connecting all the segments.

Filed follow-up https://bugs.webkit.org/show_bug.cgi?id=158195.
Comment 15 Build Bot 2016-05-29 14:59:23 PDT
Comment on attachment 280066 [details]
[Patch] Proposed Fix

Attachment 280066 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/1403797

Number of test failures exceeded the failure limit.
Comment 16 Build Bot 2016-05-29 14:59:27 PDT
Created attachment 280067 [details]
Archive of layout-test-results from ews103 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 17 Timothy Hatcher 2016-05-31 16:53:25 PDT
Does this work from all views or just Memory? It would be weird to only do this for Memory.
Comment 18 Joseph Pecoraro 2016-05-31 20:57:37 PDT
(In reply to comment #17)
> Does this work from all views or just Memory? It would be weird to only do
> this for Memory.

I think Memory is the only view that has this issue, since it is the only view that draws content from record to record.

Bubbles (JS + Layout) do not extend beyond their own bounds.

Maybe this could affect networking events with long tails, but network events aren't really discontinuous because we have complete data even if the timeline wasn't recording.
Comment 19 Joseph Pecoraro 2016-05-31 21:11:23 PDT
Comment on attachment 280066 [details]
[Patch] Proposed Fix

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

r=me, nicely done!

> Source/WebInspectorUI/UserInterface/Models/Timeline.js:106
> +    visibleRecords(startTime, endTime, includeRecordBeforeStart)

The word "visible" really doesn't make sense anymore on the Model object. I like "recordsInTimeRange" much better. What do you think?

> Source/WebInspectorUI/UserInterface/Views/MemoryTimelineView.js:224
> +                    console.assert(categoryData.dataPoints.length);
> +                    let previousDataPoint = categoryData.dataPoints.lastValue;

Hmm, I worry that we might encounter something where there is not a previous data point.

What happens in this scenario:

    1. Start at recording with just the Scripts timeline.
    2. Stop the recording.
    3. Add the Memory timeline.
    4. Start recording.
    5. Stop recording.
    6. Select Memory Timeline.
    7. Select time range starting before the first datapoint in the Memory timeline, and after the last datapoint in the memory timeline.

The same goes to the Overview Graph code, but that has and assert and early return.

> Source/WebInspectorUI/UserInterface/Views/MemoryTimelineView.js:232
> +                    categoryData.dataPoints.push({time: discontinuity.startTime, size: previousDataPoint.size});
> +
> +                    categoryData.dataPoints.push({time: discontinuity.startTime, size: 0});
> +                    categoryData.dataPoints.push({time: discontinuity.endTime, size: 0});
> +
> +                    categoryData.dataPoints.push({time: discontinuity.endTime, size: category.size});

Style: I think you can drop the comment an newlines.  The code is very clean and concise!
Comment 20 Matt Baker 2016-06-03 21:14:11 PDT
Comment on attachment 280066 [details]
[Patch] Proposed Fix

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

>> Source/WebInspectorUI/UserInterface/Models/Timeline.js:106
>> +    visibleRecords(startTime, endTime, includeRecordBeforeStart)
> 
> The word "visible" really doesn't make sense anymore on the Model object. I like "recordsInTimeRange" much better. What do you think?

Agree. I really like "recordsInTimeRange". I think visibleRecords is still okay at call sites for the returned value, since "visible" makes sense with the additional context.

>> Source/WebInspectorUI/UserInterface/Views/MemoryTimelineView.js:224
>> +                    let previousDataPoint = categoryData.dataPoints.lastValue;
> 
> Hmm, I worry that we might encounter something where there is not a previous data point.
> 
> What happens in this scenario:
> 
>     1. Start at recording with just the Scripts timeline.
>     2. Stop the recording.
>     3. Add the Memory timeline.
>     4. Start recording.
>     5. Stop recording.
>     6. Select Memory Timeline.
>     7. Select time range starting before the first datapoint in the Memory timeline, and after the last datapoint in the memory timeline.
> 
> The same goes to the Overview Graph code, but that has and assert and early return.

Great catch! TypeError:​ undefined is not an object (evaluating 'previousDataPoint.size')​ (at MemoryTimelineView.js:​227:​105).

>> Source/WebInspectorUI/UserInterface/Views/MemoryTimelineView.js:232
>> +                    categoryData.dataPoints.push({time: discontinuity.endTime, size: category.size});
> 
> Style: I think you can drop the comment an newlines.  The code is very clean and concise!

Always happy to drop newlines.
Comment 21 Matt Baker 2016-06-03 21:21:55 PDT
Comment on attachment 280066 [details]
[Patch] Proposed Fix

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

>>> Source/WebInspectorUI/UserInterface/Views/MemoryTimelineView.js:224
>>> +                    let previousDataPoint = categoryData.dataPoints.lastValue;
>> 
>> Hmm, I worry that we might encounter something where there is not a previous data point.
>> 
>> What happens in this scenario:
>> 
>>     1. Start at recording with just the Scripts timeline.
>>     2. Stop the recording.
>>     3. Add the Memory timeline.
>>     4. Start recording.
>>     5. Stop recording.
>>     6. Select Memory Timeline.
>>     7. Select time range starting before the first datapoint in the Memory timeline, and after the last datapoint in the memory timeline.
>> 
>> The same goes to the Overview Graph code, but that has and assert and early return.
> 
> Great catch! TypeError:​ undefined is not an object (evaluating 'previousDataPoint.size')​ (at MemoryTimelineView.js:​227:​105).

We can also do better in the overview graph. Currently discontinuities before the first data point aren't shown.
Comment 22 Matt Baker 2016-06-04 10:51:17 PDT
Created attachment 280519 [details]
[Patch] Proposed Fix
Comment 23 Matt Baker 2016-06-04 10:54:23 PDT
Fixed an issue with the category charts in MemoryTimelineView, that would cause the chart to extend past gaps at the beginning and end of the graph.
Comment 24 Joseph Pecoraro 2016-06-04 14:04:08 PDT
Comment on attachment 280519 [details]
[Patch] Proposed Fix

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

R=me

> Source/WebInspectorUI/UserInterface/Views/MemoryTimelineView.js:243
> +        // If the graph end time is inside a gap, the last data point should
> +        // only be extended to the start of the discontinuity.
> +        if (discontinuities.length)
> +            visibleEndTime = discontinuities[0].startTime;

Nice!
Comment 25 WebKit Commit Bot 2016-06-04 14:08:32 PDT
Comment on attachment 280519 [details]
[Patch] Proposed Fix

Clearing flags on attachment: 280519

Committed r201686: <http://trac.webkit.org/changeset/201686>
Comment 26 WebKit Commit Bot 2016-06-04 14:08:37 PDT
All reviewed patches have been landed.  Closing bug.