Bug 197440 - Web Inspector: Timelines: CPU/memory timeline bars sometimes don't draw correctly and jump around on scrolling
Summary: Web Inspector: Timelines: CPU/memory timeline bars sometimes don't draw corre...
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: Devin Rousso
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-04-30 15:10 PDT by Devin Rousso
Modified: 2019-05-17 22:27 PDT (History)
6 users (show)

See Also:


Attachments
Patch (18.91 KB, patch)
2019-04-30 15:19 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (22.46 KB, patch)
2019-05-03 17:14 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (23.86 KB, patch)
2019-05-03 17:48 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews214 for win-future (13.65 MB, application/zip)
2019-05-03 22:24 PDT, Build Bot
no flags Details
Patch (23.93 KB, patch)
2019-05-17 21:33 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (23.93 KB, patch)
2019-05-17 21:48 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Devin Rousso 2019-04-30 15:10:09 PDT
# STEPS TO REPRODUCE:
1. inspect any page with some activity
2. enable the CPU/memory timeline
3. start a timeline recording
4. scroll/interact with the page to capture activity
5. stop the timeline recording
6. try zooming in/our or scrolling left/right
 => CPU "columns" may appear/disappear if the record is mostly offscreen
 => the memory graph may suddenly update when the next record becomes visible (e.g. the slope of the graph changes)
Comment 1 Devin Rousso 2019-04-30 15:10:23 PDT
<rdar://problem/46886315>
Comment 2 Devin Rousso 2019-04-30 15:19:50 PDT
Created attachment 368617 [details]
Patch
Comment 3 Joseph Pecoraro 2019-05-02 20:05:07 PDT
Comment on attachment 368617 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Models/Timeline.js:122
> +        return this._records.slice(constrain(lowerIndex - 1), constrain(upperIndex + 1));

It sounds like records overlapping time range now include one record before and after both ends?

That is not what I'd expect in some cases. Sometimes we use this to get an exact count of the records in the range. If we are including the surrounding records that are not overlapping then that is bad.

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:1270
> -        let networkRecords = networkTimeline ? networkTimeline.recordsInTimeRange(startTime, endTime) : [];
> +        let networkRecords = networkTimeline ? networkTimeline.recordsOverlappingTimeRange(startTime, endTime) : [];

Does this now include records outside the range? We use this for a count.
Comment 4 Devin Rousso 2019-05-03 17:14:05 PDT
Created attachment 369026 [details]
Patch
Comment 5 Devin Rousso 2019-05-03 17:48:22 PDT
Created attachment 369033 [details]
Patch
Comment 6 Build Bot 2019-05-03 22:24:28 PDT Comment hidden (obsolete)
Comment 7 Build Bot 2019-05-03 22:24:30 PDT Comment hidden (obsolete)
Comment 8 Joseph Pecoraro 2019-05-17 16:28:36 PDT
Comment on attachment 369033 [details]
Patch

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

r=me

> Source/WebInspectorUI/UserInterface/Models/CPUTimeline.js:37
> +        super.addRecord(record);
> +
> +        if (lastRecord)
> +            record.adjustStartTimeToLastRecord(lastRecord);

I don't think I like this approach of modifying the CPUTimelineRecord like this. That said, it seems like an elegant solution. This means we are treating records not as a Sample time but as a Range from the last timestamp to the sample time. This changes the meaning of the model class slightly from my mental model. I suppose it is fine because CPUTimelineRecord has `timestamp` for the sample time and `start/end` may be used for other purposes.

Either way, I would expect the modification of the record to happen before `super.addRecord`, thus before any observers would have seen the pre-modified `startTime`.

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:450
> -        // Don't include the record before the graph start if the graph start is within a gap.
> -        let includeRecordBeforeStart = !discontinuities.length || discontinuities[0].startTime > graphStartTime;
> -        let visibleRecords = this.representedObject.recordsInTimeRange(graphStartTime, visibleEndTime, includeRecordBeforeStart);
> +        let visibleRecords = this.representedObject.recordsInTimeRange(graphStartTime, visibleEndTime, {
> +            includeRecordBeforeStart: !discontinuities.length || discontinuities[0].startTime > graphStartTime,
> +        });

Maybe this could include AfterEnd, and we would see the graph slope up/down to the next entry that is just beyond the selection range. I don't think we do that properly right now.

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:456
> +

Oops.

> Source/WebInspectorUI/UserInterface/Views/TimelineOverview.js:787
> +            if (startTime < this.selectionStartTime || endTime > this.selectionStartTime + this.selectionDuration || firstRecord instanceof WI.CPUTimelineRecord) {

Wrap the `endTime > (...)` in parens to improve readability.
Comment 9 Devin Rousso 2019-05-17 21:33:13 PDT
Created attachment 370184 [details]
Patch
Comment 10 Devin Rousso 2019-05-17 21:48:42 PDT
Created attachment 370186 [details]
Patch
Comment 11 WebKit Commit Bot 2019-05-17 22:27:12 PDT
Comment on attachment 370186 [details]
Patch

Clearing flags on attachment: 370186

Committed r245498: <https://trac.webkit.org/changeset/245498>
Comment 12 WebKit Commit Bot 2019-05-17 22:27:14 PDT
All reviewed patches have been landed.  Closing bug.