Bug 158052 – Web Inspector: discontinuous recordings should have discontinuities in the timeline memory graph

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Blaze Burg]

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.

[Blaze Burg]

Created attachment 279737 [details]
Screenshot

[Matt Baker]

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.

[Blaze Burg]

(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.

[Matt Baker]

(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.

[Blaze Burg]

Ping for import.

[Radar WebKit Bug Importer]

<rdar://problem/26516695>

[Matt Baker]

Created attachment 280048 [details]
[Image] New UI

[Matt Baker]

Created attachment 280049 [details]
[Patch] Proposed Fix

[Blaze Burg]

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).

[Devin Rousso]

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);

[Matt Baker]

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.

[Matt Baker]

(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.

[Matt Baker]

Created attachment 280066 [details]
[Patch] Proposed Fix

[Matt Baker]

(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.

[Build Bot]

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.

[Build Bot]

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

[Timothy Hatcher]

Does this work from all views or just Memory? It would be weird to only do this for Memory.

[Joseph Pecoraro]

(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.

[Joseph Pecoraro]

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!

[Matt Baker]

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.

[Matt Baker]

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.

[Matt Baker]

Created attachment 280519 [details]
[Patch] Proposed Fix

[Matt Baker]

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.

[Joseph Pecoraro]

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!

[WebKit Commit Bot]

Comment on attachment 280519 [details]
[Patch] Proposed Fix

Clearing flags on attachment: 280519

Committed r201686: <http://trac.webkit.org/changeset/201686>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.