Bug 200456 - [results.webkit.org Timeline] Performance improvement - Skip render offscreen canvas
Summary: [results.webkit.org Timeline] Performance improvement - Skip render offscreen...
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-05 16:32 PDT by Zhifei Fang
Modified: 2019-08-15 00:03 PDT (History)
6 users (show)

See Also:


Attachments
Patch (27.99 KB, patch)
2019-08-08 16:49 PDT, Zhifei Fang
no flags Details | Formatted Diff | Diff
Patch (30.93 KB, patch)
2019-08-12 15:31 PDT, Zhifei Fang
no flags Details | Formatted Diff | Diff
Patch (30.71 KB, patch)
2019-08-13 11:07 PDT, Zhifei Fang
no flags Details | Formatted Diff | Diff
Patch (30.53 KB, patch)
2019-08-13 11:46 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-05 16:32:06 PDT
Using intersection observer to observe if a series on display on viewport, if it is, we do the render, if not, we just skip the render.

This limited the number of frame we requested for rendering, save huge resources for nested expand series as well.
Comment 1 Zhifei Fang 2019-08-08 16:44:33 PDT
Get rid of the offscreen canvas cache, optimize the memory usage, so that webkit won't get into a "low memory" mode, this will loss a lot of performance than the benefit we gain from it.

Create a new way to batch all context state machine command by color, so that it will fill/stoke same color elements all at once, this will improve the render speed.
Comment 2 Zhifei Fang 2019-08-08 16:49:34 PDT
Created attachment 375860 [details]
Patch
Comment 3 Jonathan Bedard 2019-08-08 17:26:24 PDT
Comment on attachment 375860 [details]
Patch

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

> Tools/resultsdbpy/resultsdbpy/view/static/library/js/components/TimelineComponents.js:121
> +class ColorBatchRender {

While you are generally rendering things by color, I don't see this class actually setting colors. It's more accurately a generic render batch map that you happen to be using colors as a key. I also don't get why you need to pass colors in.

With that in mind, I think that you need to pick. Either this class batches together renders by color (and so should set colors, but does not need to pass them to the callbacks) or, this is a generic batch mapping which maps any object to a set of associated render callbacks. In that case, it would seem that the very first function provided would define what sort of context you were defining (ie, is this about filling with a specific color, drawing lines with a specific color, using a specific font, etc)

> Tools/resultsdbpy/resultsdbpy/view/static/library/js/components/TimelineComponents.js:208
>                  context.fillStyle = color;

I'm surprised to see you setting the fillStyle in this function, I would have expected it to be set in the ColorBatchRender class

> Tools/resultsdbpy/resultsdbpy/view/static/library/js/components/TimelineComponents.js:375
> +                // Clean the canvas, free its memory

Do we really need to do this? It feels like a WebKit bug if we do.

> Tools/resultsdbpy/resultsdbpy/view/static/library/js/components/TimelineComponents.js:639
> +            context.strokeStyle = usedLineColor;

Shouldn't this be color instead of usedLineColor?
Comment 4 Zhifei Fang 2019-08-12 15:31:39 PDT
Created attachment 376100 [details]
Patch
Comment 5 Zhifei Fang 2019-08-12 15:40:04 PDT
(In reply to Jonathan Bedard from comment #3)
> Comment on attachment 375860 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=375860&action=review
> 
> > Tools/resultsdbpy/resultsdbpy/view/static/library/js/components/TimelineComponents.js:121
> > +class ColorBatchRender {
> 
> While you are generally rendering things by color, I don't see this class
> actually setting colors. It's more accurately a generic render batch map
> that you happen to be using colors as a key. I also don't get why you need
> to pass colors in.
> 
> With that in mind, I think that you need to pick. Either this class batches
> together renders by color (and so should set colors, but does not need to
> pass them to the callbacks) or, this is a generic batch mapping which maps
> any object to a set of associated render callbacks. In that case, it would
> seem that the very first function provided would define what sort of context
> you were defining (ie, is this about filling with a specific color, drawing
> lines with a specific color, using a specific font, etc)
> 
> >

The operation can be fill color or stroke color or do those together, so the user of this class should define the start operation and end operation, in which they can set strokeStyle, fillStyle or both of them. I don't want to do this for user, because setting all of them together, and what's more, there are more styles should be set like lineWidth/ and beginPath, we should provide a hook for user to define their operation at the beginning and the end of the batch. 
 Tools/resultsdbpy/resultsdbpy/view/static/library/js/components/TimelineComponents.js:208
> >                  context.fillStyle = color;
> 
> I'm surprised to see you setting the fillStyle in this function, I would
> have expected it to be set in the ColorBatchRender class

This is because we have fillStyle and also strokeStyle
> 
> > Tools/resultsdbpy/resultsdbpy/view/static/library/js/components/TimelineComponents.js:375
> > +                // Clean the canvas, free its memory
> 
> Do we really need to do this? It feels like a WebKit bug if we do.
> 
> > Tools/resultsdbpy/resultsdbpy/view/static/library/js/components/TimelineComponents.js:639
> > +            context.strokeStyle = usedLineColor;
> 
> Shouldn't this be color instead of usedLineColor?
Comment 6 Jonathan Bedard 2019-08-13 07:59:24 PDT
Comment on attachment 376100 [details]
Patch

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

> Tools/resultsdbpy/resultsdbpy/view/static/library/js/components/TimelineComponents.js:53
> +* Detact if point right ray have a collision with a line segment

Should be: 'Detect a collision between a right-pointing ray and a line segment

> Tools/resultsdbpy/resultsdbpy/view/static/library/js/components/TimelineComponents.js:60
> +function pointRightRayLineSegmentCollisionDetect(point, lineStart, lineEnd) {

I like the first ray-intersecting point comment, the rest seem unhelpful. Especially if we can reduce this entire function by using point-slope.

> Tools/resultsdbpy/resultsdbpy/view/static/library/js/components/TimelineComponents.js:67
> +        return false;

What if the point y position matches the line y position?

> Tools/resultsdbpy/resultsdbpy/view/static/library/js/components/TimelineComponents.js:102
> +}

Feels like we have more if statements here than we need. I think if you just use the inverse of point-slope, we'll have everything we need.

Equations:
m = (y1 - y0) / (x1 - x0)
y = m * (x - x1) + y1

m^(-1) = (x1 - x0) / (y1 - y0)
x = m^(-1) * (y - y1) + x1

Code:

if (lineEnd.y - lineStart.y == 0)
    return lineEnd.y == point.y && (lineStart.x >= point.x || lineEnd.x >= point.x);
const invertedSlope = (lineEnd.x - lineStart.x) / (lineEnd.y - lineStart.y);
const xForY = invertedSlope * (point.y - lineStart.y) + lineStart.x;
return xForY >= point.x;

That code is totally untested, but I think we're looking for something closer to that.
Comment 7 Zhifei Fang 2019-08-13 11:07:09 PDT
Created attachment 376182 [details]
Patch
Comment 8 Jonathan Bedard 2019-08-13 11:42:11 PDT
Comment on attachment 376182 [details]
Patch

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

> Tools/resultsdbpy/resultsdbpy/view/static/library/js/components/TimelineComponents.js:66
> +    */

Not quite sure what the point you're trying to get across with this comment is.

> Tools/resultsdbpy/resultsdbpy/view/static/library/js/components/TimelineComponents.js:78
> +        return false;

We don't need the else because of early return (it's a WebKit code style policy buried somewhere in https://webkit.org/code-style-guidelines/, I think)
Comment 9 Zhifei Fang 2019-08-13 11:46:06 PDT
Created attachment 376188 [details]
Patch
Comment 10 ews-feeder 2019-08-13 14:38:00 PDT
Comment on attachment 376188 [details]
Patch

Rejecting attachment 376188 [details] from commit-queue.

zhifei_fang@apple.com does not have committer permissions according to https://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/contributors.json.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/contributors.json by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.
Comment 11 ews-feeder 2019-08-13 15:14:55 PDT
Comment on attachment 376188 [details]
Patch

Rejecting attachment 376188 [details] from commit-queue.

zhifei_fang@apple.com does not have committer permissions according to https://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/contributors.json.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/contributors.json by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.
Comment 12 ews-feeder 2019-08-13 15:17:08 PDT
Comment on attachment 376188 [details]
Patch

Rejecting attachment 376188 [details] from commit-queue.

zhifei_fang@apple.com does not have committer permissions according to https://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/contributors.json.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/contributors.json by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.
Comment 13 WebKit Commit Bot 2019-08-13 18:03:15 PDT
Comment on attachment 376188 [details]
Patch

Clearing flags on attachment: 376188

Committed r248649: <https://trac.webkit.org/changeset/248649>
Comment 14 WebKit Commit Bot 2019-08-13 18:03:16 PDT
All reviewed patches have been landed.  Closing bug.
Comment 15 Radar WebKit Bug Importer 2019-08-13 18:04:17 PDT
<rdar://problem/54282947>