Bug 127184 - Web Inspector: Implement basic versions of the TimelineOverview graphs
Summary: Web Inspector: Implement basic versions of the TimelineOverview graphs
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: Timothy Hatcher
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2014-01-17 12:59 PST by Timothy Hatcher
Modified: 2014-01-20 19:05 PST (History)
4 users (show)

See Also:


Attachments
Patch (43.32 KB, patch)
2014-01-17 13:13 PST, Timothy Hatcher
no flags Details | Formatted Diff | Diff
Screenshot (527.47 KB, image/png)
2014-01-17 13:13 PST, Timothy Hatcher
no flags Details
Patch (49.78 KB, patch)
2014-01-17 18:53 PST, Timothy Hatcher
no flags Details | Formatted Diff | Diff
Patch (56.46 KB, patch)
2014-01-17 19:28 PST, Timothy Hatcher
no flags Details | Formatted Diff | Diff
Screenshot (476.38 KB, image/png)
2014-01-17 19:29 PST, Timothy Hatcher
no flags Details
Patch (64.90 KB, patch)
2014-01-19 11:49 PST, Timothy Hatcher
joepeck: review+
timothy: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Timothy Hatcher 2014-01-17 12:59:22 PST
We can implement basic record row versions of these graphs now. Doing the versions detailed in the mock-up will require more data from the backend (frame boundaries and script profiles).
Comment 1 Radar WebKit Bug Importer 2014-01-17 12:59:35 PST
<rdar://problem/15848431>
Comment 2 Timothy Hatcher 2014-01-17 13:13:13 PST
Created attachment 221486 [details]
Patch
Comment 3 Timothy Hatcher 2014-01-17 13:13:41 PST
Created attachment 221487 [details]
Screenshot
Comment 4 Joseph Pecoraro 2014-01-17 17:51:07 PST
Comment on attachment 221486 [details]
Patch

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

Looks good! r=me

> Source/WebInspectorUI/UserInterface/LayoutTimelineOverviewGraph.css:37
> +.timeline-overview-graph.layout > .timeline-record-bar > .segment {
> +    box-shadow: white 0 0px 0px 1px;
> +}
> +
> +.timeline-overview-graph.layout:nth-child(even) > .timeline-record-bar > .segment {
> +    box-shadow: rgb(247, 247, 247) 0 0px 0px 1px;
> +}

Is this what produces the "empty stroke" preceding segments?

> Source/WebInspectorUI/UserInterface/LayoutTimelineOverviewGraph.js:61
> +        for (var record of this._layoutTimeline.records) {
> +            var timelineRecordBar = this._timelineRecordBarMap.get(record);
> +            if (!timelineRecordBar) {

This could be a performance issue when there are a lot of records:

    for each record (simple loop)
        find record in map (lookup)
            ...

Ideally we could just iterate all of the values in the map (assuming it has a guaranteed order)

    for each record in map.values (simple loop)
        ...

Apparently though we may also be creating record bars if they didn't exist. If we can just keep track of the records without bars, we can eliminate that step and just iterate timelineRecordBarMap.values(). I'd be much happier with that.

> Source/WebInspectorUI/UserInterface/NetworkTimelineOverviewGraph.js:81
> +            for (var record of rowRecords) {
> +                var timelineRecordBar = this._timelineRecordBarMap.get(record);
> +                if (!timelineRecordBar) {

Ditto

> Source/WebInspectorUI/UserInterface/NetworkTimelineOverviewGraph.js:112
> +                rowRecords.splice(insertionIndexForObjectInListSortedByFunction(resourceTimelineRecord, rowRecords, compareByStartTime), 0, resourceTimelineRecord);

You could create a helper for this:

    function insertObjectIntoSortedArray(object, array, compareFunction) {
        var index = insertionIndexForObjectInListSortedByFunction(object, array, compareFunction);
        array.splice(index, 0, object);
    }

and usage here would read much better:

    insertObjectIntoSortedArray(resourceTimelineRecord, rowRecords, compareByStartTime);

> Source/WebInspectorUI/UserInterface/NetworkTimelineOverviewGraph.js:123
> +            this._timelineRecordGridRows[this._nextDumpRow++].push(resourceTimelineRecord);

Should this insert be sorted? Or this is specifically not a sorted insert so that the bar for this record will draw in a particular way (on top of earlier items).

> Source/WebInspectorUI/UserInterface/ScriptTimelineOverviewGraph.js:60
> +        for (var record of this._scriptTimeline.records) {
> +            var timelineRecordBar = this._timelineRecordBarMap.get(record);

Ditto

> Source/WebInspectorUI/UserInterface/TimelineOverviewGraph.js:87
> +        this._endTime = x || 0;

Nit: "|| 5" like the default? 0..0 might be weird but might also make an issue easier to diagnose.
Comment 5 Timothy Hatcher 2014-01-17 18:53:30 PST
Created attachment 221522 [details]
Patch
Comment 6 Timothy Hatcher 2014-01-17 19:00:19 PST
Comment on attachment 221486 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/LayoutTimelineOverviewGraph.css:37
>> +}
> 
> Is this what produces the "empty stroke" preceding segments?

Yep!

>> Source/WebInspectorUI/UserInterface/LayoutTimelineOverviewGraph.js:61
>> +            if (!timelineRecordBar) {
> 
> This could be a performance issue when there are a lot of records:
> 
>     for each record (simple loop)
>         find record in map (lookup)
>             ...
> 
> Ideally we could just iterate all of the values in the map (assuming it has a guaranteed order)
> 
>     for each record in map.values (simple loop)
>         ...
> 
> Apparently though we may also be creating record bars if they didn't exist. If we can just keep track of the records without bars, we can eliminate that step and just iterate timelineRecordBarMap.values(). I'd be much happier with that.

I rewrote this patch for performance. The new patch does not use TimelineRecordBar, or the map. The new patch loops over records per row.

>> Source/WebInspectorUI/UserInterface/NetworkTimelineOverviewGraph.js:81
>> +                if (!timelineRecordBar) {
> 
> Ditto

Good point. I'll rework these too, not just Network.

>> Source/WebInspectorUI/UserInterface/NetworkTimelineOverviewGraph.js:112
>> +                rowRecords.splice(insertionIndexForObjectInListSortedByFunction(resourceTimelineRecord, rowRecords, compareByStartTime), 0, resourceTimelineRecord);
> 
> You could create a helper for this:
> 
>     function insertObjectIntoSortedArray(object, array, compareFunction) {
>         var index = insertionIndexForObjectInListSortedByFunction(object, array, compareFunction);
>         array.splice(index, 0, object);
>     }
> 
> and usage here would read much better:
> 
>     insertObjectIntoSortedArray(resourceTimelineRecord, rowRecords, compareByStartTime);

Good idea.

>> Source/WebInspectorUI/UserInterface/NetworkTimelineOverviewGraph.js:123
>> +            this._timelineRecordGridRows[this._nextDumpRow++].push(resourceTimelineRecord);
> 
> Should this insert be sorted? Or this is specifically not a sorted insert so that the bar for this record will draw in a particular way (on top of earlier items).

It should be sorted too. Will fix.

>> Source/WebInspectorUI/UserInterface/TimelineOverviewGraph.js:87
>> +        this._endTime = x || 0;
> 
> Nit: "|| 5" like the default? 0..0 might be weird but might also make an issue easier to diagnose.

Yeah it would be an error to do this, and 0 is likely best.
Comment 7 Timothy Hatcher 2014-01-17 19:28:54 PST
Created attachment 221523 [details]
Patch
Comment 8 Timothy Hatcher 2014-01-17 19:29:40 PST
Created attachment 221524 [details]
Screenshot
Comment 9 Timothy Hatcher 2014-01-17 19:32:02 PST
Comment on attachment 221523 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/LayoutTimelineOverviewGraph.js:59
> +    updateLayout: function()

This code is repeated in ScriptTimelineOverviewGraph (and a more complex version is in NetworkTimelineOverviewGraph). I would normally try to share code this size, however this is just temp code. The final implementations will be different.
Comment 10 Timothy Hatcher 2014-01-19 11:49:14 PST
Created attachment 221594 [details]
Patch
Comment 11 Timothy Hatcher 2014-01-19 11:50:26 PST
Sorry for the patch churn. This version moved some of the bar combining logic into TimelineRecordBar and uses it for TimelineDataGridNode.
Comment 12 Joseph Pecoraro 2014-01-20 12:21:31 PST
Comment on attachment 221594 [details]
Patch

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

r=me!

> Source/WebInspectorUI/UserInterface/LayoutTimelineOverviewGraph.js:91
> +        for (var record of this._layoutTimeline.records) {
> +            // If this bar is completely before the bounds of the graph, skip this record.
> +            if (record.endTime < startTime)
> +                continue;
> +
> +            // If this record is completely after the current time or end time, break out now.
> +            // Records are sorted, so all records after this will be beyond the current or end time too.
> +            if (record.startTime > currentTime || record.startTime > endTime)
> +                break;

Nice! What we can do in the future, if needed because this will change, would be:

  1. Binary search (based on startTime) for the first record to start showing.
  2. Walk through relevant records until this break

Currently for a long recording with the overview graph only showing the last second it looks like this will still walk through all records.

> Source/WebInspectorUI/UserInterface/NetworkTimelineOverviewGraph.js:98
> +            var barElement = barElementCache.shift();
> +            if (!barElement) {
> +                barElement = document.createElement("div");
> +                barElement.classList.add(WebInspector.NetworkTimelineOverviewGraph.BarStyleClassName);
> +            }

Awesome!

> Source/WebInspectorUI/UserInterface/NetworkTimelineOverviewGraph.js:134
> +            var rowElement = rowRecords.__rowElement;
> +            if (!rowElement) {
> +                rowElement = rowRecords.__rowElement = document.createElement("div");
> +                rowElement.className = WebInspector.NetworkTimelineOverviewGraph.GraphRowStyleClassName;
> +                this.element.appendChild(rowElement);
> +            }
> +
> +            if (!rowRecords.length)
> +                continue;

Nit: I wonder if this may be worth doing in reset(). Then we won't have to do do this if check 6 times every updateLayout. Or at least move it past the rowRecords.length check, so you will lazily create rows as needed.

> Source/WebInspectorUI/UserInterface/NetworkTimelineOverviewGraph.js:148
> +                // If this bar is completly before the bounds of the graph, skip this record.

Typo: completly => completely

> Source/WebInspectorUI/UserInterface/NetworkTimelineOverviewGraph.js:152
> +                // If this record is completly after the current time or end time, break out now.

Typo: completly => completely

> Source/WebInspectorUI/UserInterface/NetworkTimelineOverviewGraph.js:222
> +        function insertObjectIntoSortedArray(value, array, compareFunction)
> +        {
> +           var index = insertionIndexForObjectInListSortedByFunction(value, array, compareFunction);
> +           array.splice(index, 0, value);
> +        }

Nit: BinarySearch.js

> Source/WebInspectorUI/UserInterface/ScriptTimelineOverviewGraph.js:86
> +        for (var record of this._scriptTimeline.records) {
> +            // If this bar is completely before the bounds of the graph, skip this record.
> +            if (record.endTime < startTime)
> +                continue;

Ditto RE binary search.

> Source/WebInspectorUI/UserInterface/TimelineDataGridNode.js:262
> +            // Combining multiple record bars is not supported with records that have inactive time.
> +            // ResourceTimelineRecord is the only one right, and it is always a single record handled above.
> +            console.assert(!record.usesActiveStartTime);
> +
> +            if (isNaN(record.startTime))
> +                continue;
> +
> +            // If this bar is completely before the bounds of the graph, skip this record.
> +            if (record.endTime < startTime)
> +                continue;

Ditto. Although here it may be useful because I don't suspect this code will change (it is not part of the overview graphs).

> Source/WebInspectorUI/UserInterface/TimelineRecordBar.js:110
> +        // Inactive time is only supported with one record.
> +        if (this._records.length === 1 && this._records[0].usesActiveStartTime) {
> +            this._inactiveBarElement = document.createElement("div");

If you went from a TimelineRecordBar with 1 record with inactive to 1 record with inactive you could leak the old _inactiveBarElement?

You could just add an: if (!this._inactiveBarElement) inside this section and reuse the existing element.
Comment 13 Timothy Hatcher 2014-01-20 19:05:21 PST
https://trac.webkit.org/changeset/162419