Bug 200406

Summary: [results.webkit.org Timeline] Performance improvements
Product: WebKit Reporter: Zhifei Fang <zhifei_fang>
Component: Tools / TestsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, jbedard, webkit-bug-importer, zhifei_fang
Priority: P2 Keywords: InRadar
Version: Safari Technology Preview   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

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>