Bug 200406 - [results.webkit.org Timeline] Performance improvements
Summary: [results.webkit.org Timeline] Performance improvements
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: Safari Technology Preview
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-08-02 14:35 PDT by Zhifei Fang
Modified: 2019-08-06 10:58 PDT (History)
4 users (show)

See Also:


Attachments
Patch (15.84 KB, patch)
2019-08-02 14:36 PDT, Zhifei Fang
no flags Details | Formatted Diff | Diff
Patch (16.73 KB, patch)
2019-08-02 16:18 PDT, Zhifei Fang
no flags Details | Formatted Diff | Diff
Patch (16.31 KB, patch)
2019-08-02 19:11 PDT, Zhifei Fang
no flags Details | Formatted Diff | Diff
Patch (20.60 KB, patch)
2019-08-05 16:43 PDT, Zhifei Fang
no flags Details | Formatted Diff | Diff
Patch (20.88 KB, patch)
2019-08-05 18:43 PDT, Zhifei Fang
no flags Details | Formatted Diff | Diff
Patch (21.20 KB, patch)
2019-08-06 10:31 PDT, Zhifei Fang
no flags Details | Formatted Diff | Diff
Patch (21.22 KB, patch)
2019-08-06 10:33 PDT, Zhifei Fang
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Zhifei Fang 2019-08-02 14:35:37 PDT
Unhook the scroll event when a series/axis have been removed from the container 

This will limit the number of event fired and actually destroy all the series and axis that have been removed from the container.
Comment 1 Zhifei Fang 2019-08-02 14:36:18 PDT
Created attachment 375458 [details]
Patch
Comment 2 Jonathan Bedard 2019-08-02 15:59:53 PDT
Comment on attachment 375458 [details]
Patch

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

> Tools/ChangeLog:8
> +        * resultsdbpy/resultsdbpy/view/static/library/js/Ref.js:

We need some comments here.
Comment 3 Zhifei Fang 2019-08-02 16:18:26 PDT
Created attachment 375465 [details]
Patch
Comment 4 Zhifei Fang 2019-08-02 19:11:08 PDT
Created attachment 375474 [details]
Patch
Comment 5 Jonathan Bedard 2019-08-05 09:48:57 PDT
Comment on attachment 375465 [details]
Patch

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

> Tools/resultsdbpy/resultsdbpy/view/static/library/js/components/TimelineComponents.js:620
> +        console.log(stateDiff);

Do you intend to land this logging statement?
Comment 6 Zhifei Fang 2019-08-05 16:43:51 PDT
Created attachment 375583 [details]
Patch
Comment 7 Jonathan Bedard 2019-08-05 17:21:15 PDT
Comment on attachment 375583 [details]
Patch

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

Along with fixing a few pretty clear correctness bugs, this patch has a HUGE impact on memory usage of timelines, particularly large ones. I've observed about 5x in some cases, probably 2 or 3x on average.

> Tools/resultsdbpy/resultsdbpy/view/static/library/js/Utils.js:69
> +// Check more about Intersection observer API

I would put this in one-line:

// Uses intersection observer: <https://developer.mozilla.org/en-US/docs/Web/API/Intersection_Observer_API>

> Tools/resultsdbpy/resultsdbpy/view/static/library/js/components/TimelineComponents.js:380
> +                // We do nothing for those off screen ones

Comment is duplicating what the code says, just remove it.

How would the element be on screen but the stateDiff not?

> Tools/resultsdbpy/resultsdbpy/view/static/library/js/components/TimelineComponents.js:763
> +                // In case of modification white rendering

Nit: while rendering
Comment 8 Zhifei Fang 2019-08-05 18:15:06 PDT
(In reply to Jonathan Bedard from comment #7)
> Comment on attachment 375583 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=375583&action=review
> 
> Along with fixing a few pretty clear correctness bugs, this patch has a HUGE
> impact on memory usage of timelines, particularly large ones. I've observed
> about 5x in some cases, probably 2 or 3x on average.
> 
> > Tools/resultsdbpy/resultsdbpy/view/static/library/js/Utils.js:69
> > +// Check more about Intersection observer API
> 
> I would put this in one-line:
> 
> // Uses intersection observer:
> <https://developer.mozilla.org/en-US/docs/Web/API/Intersection_Observer_API>
> 
> > Tools/resultsdbpy/resultsdbpy/view/static/library/js/components/TimelineComponents.js:380
> > +                // We do nothing for those off screen ones
> 
> Comment is duplicating what the code says, just remove it.
> 
> How would the element be on screen but the stateDiff not?
stateDiff is a diff, when we already set onScreen to true, it will always be true in the state, unless we want to set it to false. 

> 
> > Tools/resultsdbpy/resultsdbpy/view/static/library/js/components/TimelineComponents.js:763
> > +                // In case of modification white rendering
> 
> Nit: while rendering
Comment 9 Zhifei Fang 2019-08-05 18:19:59 PDT
Just change the title, this patch now include multiple performance improvements for the canvas timeline.

1. Unhook the scroll event when a series/axis have been removed from the container 
2. Fix the axis's cache date structure out of sync.
3. Use position:sticky to reduce the blink when update the transform
4. Use intersection observer to detect if the canvas on screen or not, if a canvas is not on the screen, we do nothing, this will eliminate render requests we send out.
Comment 10 Zhifei Fang 2019-08-05 18:43:19 PDT
Created attachment 375592 [details]
Patch
Comment 11 Jonathan Bedard 2019-08-06 08:12:16 PDT
Comment on attachment 375592 [details]
Patch

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

> Tools/ChangeLog:10
> +        Reviewed by NOBODY (OOPS!).

WebKit does:

<change title>
<bug URL>

Reviewed by <Reviewer>.

<change details>

-----

So can we move the numbered list under the 'Reviewed by' line?
Comment 12 Zhifei Fang 2019-08-06 10:31:50 PDT
Created attachment 375630 [details]
Patch
Comment 13 Zhifei Fang 2019-08-06 10:33:48 PDT
Created attachment 375631 [details]
Patch
Comment 14 WebKit Commit Bot 2019-08-06 10:57:03 PDT
Comment on attachment 375631 [details]
Patch

Clearing flags on attachment: 375631

Committed r248305: <https://trac.webkit.org/changeset/248305>
Comment 15 WebKit Commit Bot 2019-08-06 10:57:05 PDT
All reviewed patches have been landed.  Closing bug.
Comment 16 Radar WebKit Bug Importer 2019-08-06 10:58:15 PDT
<rdar://problem/53993566>