Bug 157535 - Web Inspector: Avoid unnecessary timeout identifier churn in TimelineManager
Summary: Web Inspector: Avoid unnecessary timeout identifier churn in TimelineManager
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: Joseph Pecoraro
URL:
Keywords: DoNotImportToRadar
Depends on:
Blocks:
 
Reported: 2016-05-10 14:31 PDT by Joseph Pecoraro
Modified: 2016-05-10 18:23 PDT (History)
8 users (show)

See Also:


Attachments
[PATCH] Proposed Fix (2.58 KB, patch)
2016-05-10 14:32 PDT, Joseph Pecoraro
timothy: review+
Details | Formatted Diff | Diff
[PATCH] For Landing (2.62 KB, patch)
2016-05-10 17:26 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Pecoraro 2016-05-10 14:31:47 PDT
* SUMMARY
Avoid unnecessary timeout identifier churn in TimelineManager.

The _deadTimeTimeout can get churned thousands of times (4000+ on theverge.com) because a bunch of records are added at once.

Reduce the churn on the timeout identifier (<100), which also slightly reduces the amount of time spent in _resetAutoRecordingDeadTimeTimeout.
Comment 1 Joseph Pecoraro 2016-05-10 14:32:53 PDT
Created attachment 278531 [details]
[PATCH] Proposed Fix
Comment 2 Joseph Pecoraro 2016-05-10 14:35:09 PDT
Comment on attachment 278531 [details]
[PATCH] Proposed Fix

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

> Source/WebInspectorUI/UserInterface/Controllers/TimelineManager.js:744
> +        let now = Date.now();
> +        if (now <= this._lastDeadTimeTickle + 10)
> +            return;
> +
> +        this._lastDeadTimeTickle = now;

We could make the this._lastDeadTimeTickle = now + 10. And eliminate the "+ 10", thereby eliminating the add every check. But it felt less clear. Still the time spent in this was only ~5ms on a long recording on theverge, so not very important.
Comment 3 Matt Baker 2016-05-10 15:01:33 PDT
Comment on attachment 278531 [details]
[PATCH] Proposed Fix

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

> Source/WebInspectorUI/UserInterface/Controllers/TimelineManager.js:741
> +        if (now <= this._lastDeadTimeTickle + 10)

Magic number: a comment at the end of the line or a const on the line above would help clarify the 10.
Comment 4 Timothy Hatcher 2016-05-10 15:51:36 PDT
Comment on attachment 278531 [details]
[PATCH] Proposed Fix

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

> Source/WebInspectorUI/UserInterface/Controllers/TimelineManager.js:740
> +        let now = Date.now();

performance.now?

>> Source/WebInspectorUI/UserInterface/Controllers/TimelineManager.js:744
>> +        this._lastDeadTimeTickle = now;
> 
> We could make the this._lastDeadTimeTickle = now + 10. And eliminate the "+ 10", thereby eliminating the add every check. But it felt less clear. Still the time spent in this was only ~5ms on a long recording on theverge, so not very important.

I think putting the + 10 here makes more sense.
Comment 5 Joseph Pecoraro 2016-05-10 17:26:33 PDT
Created attachment 278555 [details]
[PATCH] For Landing
Comment 6 WebKit Commit Bot 2016-05-10 17:55:57 PDT
Comment on attachment 278555 [details]
[PATCH] For Landing

Clearing flags on attachment: 278555

Committed r200661: <http://trac.webkit.org/changeset/200661>