Bug 99830 – Web Inspector: Show aggregated time between corresponding time/timeEnd records.

• Component: Web Inspector (Deprecated)
• Status: RESOLVED INVALID

console.time / console.timeEnd are used to measure how long some process takes.
This patch makes this feature more useful - not aggregated statistics is shown
for measured interval.

Created attachment 169613 [details]
Patch

Created attachment 169844 [details]
Patch

[Pavel Feldman]

Comment on attachment 169844 [details]
Patch

Could you elaborate on "aggregated statistics" and add a test?

Created attachment 169851 [details]
Popup with aggregated statistics.

(In reply to comment #3)
> (From update of attachment 169844 [details])
> Could you elaborate on "aggregated statistics" and add a test?

Done.

Created attachment 169865 [details]
Patch

[Andrey Kosyakov]

Comment on attachment 169865 [details]
Patch

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

> Source/WebCore/inspector/front-end/TimelinePresentationModel.js:622
> +        var record = this;
> +        var parent = record.parent;
> +        while (parent.parent) {
> +            record = parent;
> +            parent = parent.parent;
> +        }
> +        return record;

for (var record = this; record.parent && record.parent.parent; record = record.parent) {}

> Source/WebCore/inspector/front-end/TimelinePresentationModel.js:1026
> +        while (record.type !== WebInspector.TimelineModel.RecordType.Root) {

ditto, use for(;;)

> Source/WebCore/inspector/front-end/TimelinePresentationModel.js:1033
> +                if (child.endTime >= boundryTime)
> +                    continue;

why not break?

Comment on attachment 169865 [details]
Patch

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

>> Source/WebCore/inspector/front-end/TimelinePresentationModel.js:622
>> +        return record;
> 
> for (var record = this; record.parent && record.parent.parent; record = record.parent) {}

Done

>> Source/WebCore/inspector/front-end/TimelinePresentationModel.js:1026
>> +        while (record.type !== WebInspector.TimelineModel.RecordType.Root) {
> 
> ditto, use for(;;)

Done

>> Source/WebCore/inspector/front-end/TimelinePresentationModel.js:1033
>> +                    continue;
> 
> why not break?

because glued records may appear out of order.

Created attachment 170163 [details]
Patch

[Andrey Kosyakov]

LGTM

[Pavel Feldman]

Comment on attachment 170163 [details]
Patch

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

> Source/WebCore/inspector/front-end/TimelinePanel.js:556
> +            this._innerAddRecordToTimeline(records[i]);

Is this change related?

> Source/WebCore/inspector/front-end/TimelinePresentationModel.js:615
> +    get topLevelRecord()

Please use methods (isTopLevelRecord) instead of getters for better compile-ability. Also, this seems to be slow, you might want to reflect it in the name.

> Source/WebCore/inspector/front-end/TimelinePresentationModel.js:775
>              contentHelper._appendTextRow(WebInspector.UIString("Self Time"), Number.secondsToString(this._selfTime, true));

Why is time excluded? Does this belong to this change?

> Source/WebCore/inspector/front-end/TimelinePresentationModel.js:1014
> +WebInspector.TimelinePresentationModel._calculateAggregatedStatsBetweenRecords = function(head, tail)

I thought we already aggregated the time for the CPU summary. Why do we need separate code for that?

Comment on attachment 170163 [details]
Patch

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

>> Source/WebCore/inspector/front-end/TimelinePanel.js:556
>> +            this._innerAddRecordToTimeline(records[i]);
> 
> Is this change related?

No. This change is unrelated.
I've done this change to make clarify - "what is parentRecord parameter of _innerAddRecord method".

>> Source/WebCore/inspector/front-end/TimelinePresentationModel.js:615
>> +    get topLevelRecord()
> 
> Please use methods (isTopLevelRecord) instead of getters for better compile-ability. Also, this seems to be slow, you might want to reflect it in the name.

OK

>> Source/WebCore/inspector/front-end/TimelinePresentationModel.js:775
>>              contentHelper._appendTextRow(WebInspector.UIString("Self Time"), Number.secondsToString(this._selfTime, true));
> 
> Why is time excluded? Does this belong to this change?

"Time" is excluded to avoid doubling "Aggregated Time" field.
Aggregated time for "Time/TimeEnd" records is not computed "online", so it can't be rendered at this line.

>> Source/WebCore/inspector/front-end/TimelinePresentationModel.js:1014
>> +WebInspector.TimelinePresentationModel._calculateAggregatedStatsBetweenRecords = function(head, tail)
> 
> I thought we already aggregated the time for the CPU summary. Why do we need separate code for that?

We need to aggregate time by categories between two specified moments.
The problem is - we can't simply use already calculated aggregated time, because cutting record in the middle changes its "self" time.
We do reuse already calculated aggregated time for all records except records on the "cut".

Created attachment 171429 [details]
Patch

[Pavel Feldman]

Comment on attachment 171429 [details]
Patch

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

> Source/WebCore/inspector/front-end/TimelinePanel.js:556
> +            this._innerAddRecordToTimeline(records[i]);

Please extract this into a separate change.

> Source/WebCore/inspector/front-end/TimelinePresentationModel.js:1025
> +WebInspector.TimelinePresentationModel._calculateAggregatedStatsBetweenRecords = function(head, tail)

I still don't understand why this code is different from computing cpu time. They should be combined.

[Pavel Feldman]

Comment on attachment 171429 [details]
Patch

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

>> Source/WebCore/inspector/front-end/TimelinePresentationModel.js:1025
>> +WebInspector.TimelinePresentationModel._calculateAggregatedStatsBetweenRecords = function(head, tail)
> 
> I still don't understand why this code is different from computing cpu time. They should be combined.

I looked at it again, and the code is still too hard to follow. I don't think the feature is worth the complexity. So I stay by my r-.

[Brian Burg]

Closing as invalid, as this bug pertains to the old inspector UI and/or its tests.
Please file a new bug (https://www.webkit.org/new-inspector-bug) if the bug/feature/issue is still relevant to WebKit trunk.