Bug 200456 – [results.webkit.org Timeline] Performance improvement - Skip render offscreen canvas

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Zhifei Fang]

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.

[Zhifei Fang]

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.

[Zhifei Fang]

Created attachment 375860 [details]
Patch

[Jonathan Bedard]

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?

[Zhifei Fang]

Created attachment 376100 [details]
Patch

[Zhifei Fang]

(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?

[Jonathan Bedard]

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.

[Zhifei Fang]

Created attachment 376182 [details]
Patch

[Jonathan Bedard]

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)

[Zhifei Fang]

Created attachment 376188 [details]
Patch

[EWS]

Comment on attachment 376188 [details]
Patch

Rejecting attachment 376188 [details] from commit-queue.

[e-mail] 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.

[EWS]

Comment on attachment 376188 [details]
Patch

Rejecting attachment 376188 [details] from commit-queue.

[e-mail] 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.

[EWS]

Comment on attachment 376188 [details]
Patch

Rejecting attachment 376188 [details] from commit-queue.

[e-mail] 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.

[WebKit Commit Bot]

Comment on attachment 376188 [details]
Patch

Clearing flags on attachment: 376188

Committed r248649: <https://trac.webkit.org/changeset/248649>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/54282947>