New issue
Advanced search Search tips

Issue 728595 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Oct 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Feature



Sign in to add a comment

Implement two PaintWorkletGlobalScropes

Project Member Reported by xidac...@chromium.org, Jun 1 2017

Issue description

This will optimize the performance, we should have this before shipping paintWorklet.

 
This would likely require some kind of refactoring: Move the Paint() function in CSSPaintImageGeneratorImpl to PaintWorklet. In that function, it should try to find an available PaintWorkletGlobalScope, get the document paint definition, and call paint.

We follow the same pattern as the animationWorklet and audioWorklet implementation
Cc: flackr@chromium.org
Cc: xidac...@chromium.org
 Issue 738772  has been merged into this issue.
Cc: hongchan@chromium.org
+hongchan for audioWorklet, FYI
Project Member

Comment 5 by bugdroid1@chromium.org, Jul 24 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/bf31cc7574cac89beeef037595e4f56412abdb23

commit bf31cc7574cac89beeef037595e4f56412abdb23
Author: Xida Chen <xidachen@chromium.org>
Date: Mon Jul 24 15:33:30 2017

Implement two PaintWorkletGlobalScopes

In our current implementation, we only have one paint worklet global scope.
When we call paint, we always find the css paint definition on that global
scope.

This CL implements two paint worklet global scopes, in order to balance
the workload on each global scope. We follow the spec here:
https://drafts.css-houdini.org/css-paint-api-1/#paint-class-instances
and added the "DocumentPaintDefinition" class. PaintWorklet maintains
a document paint definition map. Now instead of calling CSSPaintDefinition's
Paint() function, PaintWorklet::Paint() finds an available paint worklet
global scope and calls the Paint() function that is associated with that.

Bug:  728595 
Change-Id: I90de5001f8dd8371d56f3a7d677ba169cec94e01
Reviewed-on: https://chromium-review.googlesource.com/562197
Reviewed-by: meade_UTC10 <meade@chromium.org>
Reviewed-by: Robert Flack <flackr@chromium.org>
Reviewed-by: Ian Kilpatrick <ikilpatrick@chromium.org>
Reviewed-by: Hiroki Nakagawa <nhiroki@chromium.org>
Commit-Queue: Xida Chen <xidachen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#488979}
[modify] https://crrev.com/bf31cc7574cac89beeef037595e4f56412abdb23/third_party/WebKit/LayoutTests/http/tests/inspector/console/console-on-paint-worklet-expected.txt
[modify] https://crrev.com/bf31cc7574cac89beeef037595e4f56412abdb23/third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/worklet-import-blocked-expected.txt
[modify] https://crrev.com/bf31cc7574cac89beeef037595e4f56412abdb23/third_party/WebKit/LayoutTests/http/tests/worklet/webexposed/global-interface-listing-paint-worklet-expected.txt
[modify] https://crrev.com/bf31cc7574cac89beeef037595e4f56412abdb23/third_party/WebKit/LayoutTests/http/tests/worklet/webexposed/global-interface-listing-paint-worklet.html
[modify] https://crrev.com/bf31cc7574cac89beeef037595e4f56412abdb23/third_party/WebKit/LayoutTests/inspector/console/paintworklet-console-selector-expected.txt
[modify] https://crrev.com/bf31cc7574cac89beeef037595e4f56412abdb23/third_party/WebKit/Source/core/css/CSSSyntaxDescriptor.h
[modify] https://crrev.com/bf31cc7574cac89beeef037595e4f56412abdb23/third_party/WebKit/Source/core/workers/Worklet.cpp
[modify] https://crrev.com/bf31cc7574cac89beeef037595e4f56412abdb23/third_party/WebKit/Source/modules/csspaint/BUILD.gn
[modify] https://crrev.com/bf31cc7574cac89beeef037595e4f56412abdb23/third_party/WebKit/Source/modules/csspaint/CSSPaintDefinition.cpp
[modify] https://crrev.com/bf31cc7574cac89beeef037595e4f56412abdb23/third_party/WebKit/Source/modules/csspaint/CSSPaintDefinition.h
[modify] https://crrev.com/bf31cc7574cac89beeef037595e4f56412abdb23/third_party/WebKit/Source/modules/csspaint/CSSPaintImageGeneratorImpl.cpp
[modify] https://crrev.com/bf31cc7574cac89beeef037595e4f56412abdb23/third_party/WebKit/Source/modules/csspaint/CSSPaintImageGeneratorImpl.h
[add] https://crrev.com/bf31cc7574cac89beeef037595e4f56412abdb23/third_party/WebKit/Source/modules/csspaint/DocumentPaintDefinition.cpp
[add] https://crrev.com/bf31cc7574cac89beeef037595e4f56412abdb23/third_party/WebKit/Source/modules/csspaint/DocumentPaintDefinition.h
[modify] https://crrev.com/bf31cc7574cac89beeef037595e4f56412abdb23/third_party/WebKit/Source/modules/csspaint/PaintWorklet.cpp
[modify] https://crrev.com/bf31cc7574cac89beeef037595e4f56412abdb23/third_party/WebKit/Source/modules/csspaint/PaintWorklet.h
[modify] https://crrev.com/bf31cc7574cac89beeef037595e4f56412abdb23/third_party/WebKit/Source/modules/csspaint/PaintWorkletGlobalScope.cpp
[modify] https://crrev.com/bf31cc7574cac89beeef037595e4f56412abdb23/third_party/WebKit/Source/modules/csspaint/PaintWorkletGlobalScope.h
[modify] https://crrev.com/bf31cc7574cac89beeef037595e4f56412abdb23/third_party/WebKit/Source/modules/csspaint/PaintWorkletGlobalScopeProxy.cpp
[modify] https://crrev.com/bf31cc7574cac89beeef037595e4f56412abdb23/third_party/WebKit/Source/modules/csspaint/PaintWorkletGlobalScopeProxy.h
[modify] https://crrev.com/bf31cc7574cac89beeef037595e4f56412abdb23/third_party/WebKit/Source/modules/csspaint/PaintWorkletPendingGeneratorRegistry.cpp
[modify] https://crrev.com/bf31cc7574cac89beeef037595e4f56412abdb23/third_party/WebKit/Source/modules/csspaint/PaintWorkletPendingGeneratorRegistry.h

Project Member

Comment 6 by bugdroid1@chromium.org, Jul 25 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0e0addc1fe039025f43d16d3fd368358a2ae6f02

commit 0e0addc1fe039025f43d16d3fd368358a2ae6f02
Author: Xida Chen <xidachen@chromium.org>
Date: Tue Jul 25 14:41:32 2017

Change Worklet::proxies_ to be a HeapVector

While implementing two paint worklet global scope, I need a way to tell
FindAvailableGlobalScope() which WorkletGlobalScopeProxy that I want to
use. However, currently proxies_ is a HeapHashSet, so I cannot tell which
one I want to use by indexing.

This CL changes the HeapHashSet to be a HeapVector. There should be no
performance implication because all worklets uses 1-2 global scopes.
Inserting a new worklet global scope to HeapVector should be faster than
to HeapHashSet.

Bug:  728595 
Change-Id: Ia8d63585d3f64976ace79144c58866db2a8188e5
Reviewed-on: https://chromium-review.googlesource.com/584023
Commit-Queue: Xida Chen <xidachen@chromium.org>
Reviewed-by: Hiroki Nakagawa <nhiroki@chromium.org>
Cr-Commit-Position: refs/heads/master@{#489293}
[modify] https://crrev.com/0e0addc1fe039025f43d16d3fd368358a2ae6f02/third_party/WebKit/Source/core/workers/Worklet.cpp
[modify] https://crrev.com/0e0addc1fe039025f43d16d3fd368358a2ae6f02/third_party/WebKit/Source/core/workers/Worklet.h

Components: Blink>Paint
Project Member

Comment 8 by bugdroid1@chromium.org, Jul 29 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a20d78ed60790d20b48b69ea59d818ea0b6fd7e7

commit a20d78ed60790d20b48b69ea59d818ea0b6fd7e7
Author: Xida Chen <xidachen@chromium.org>
Date: Sat Jul 29 02:02:46 2017

Add a layout test to css-paint-api

This CL adds a test for the case where we register a paint worklet with
two paint definition with different properties. This should cause an
exception and not draw anything.

What would happen in the test is that one global scope will register the
paint definition correctly. When the second global scope tries to
register the paint definition, it founds that the input properties are
different compared with the already registered one, so it will throw
an exception

Bug:  728595 
Change-Id: I941d04e305b3e8eae96c7ca9004e29de8855ee99
Reviewed-on: https://chromium-review.googlesource.com/589627
Reviewed-by: Robert Flack <flackr@chromium.org>
Commit-Queue: Xida Chen <xidachen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#490579}
[add] https://crrev.com/a20d78ed60790d20b48b69ea59d818ea0b6fd7e7/third_party/WebKit/LayoutTests/external/wpt/css-paint-api/parse-input-arguments-018-ref.html
[add] https://crrev.com/a20d78ed60790d20b48b69ea59d818ea0b6fd7e7/third_party/WebKit/LayoutTests/external/wpt/css-paint-api/parse-input-arguments-018.html

Project Member

Comment 9 by bugdroid1@chromium.org, Aug 3 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1ab1fd870ccc6b3f020afdf7f4a049ba47ca6667

commit 1ab1fd870ccc6b3f020afdf7f4a049ba47ca6667
Author: Xida Chen <xidachen@chromium.org>
Date: Thu Aug 03 15:39:26 2017

Implement switching global scope in paint worklet

In this previous CL: https://chromium-review.googlesource.com/c/562197/,
we introduced the concept of document paint definition, such that we
can have two paint worklet global scopes. However, we didn't Implement
the logic to switch between these two global scopes.

This CL implements the logic. In particular, after every 120 frames that
has called the paint function, we switch to use another global scope.

Bug:  728595 
Change-Id: Ie48a7e597deb10342ede64c42e595c0b11d779f1
Reviewed-on: https://chromium-review.googlesource.com/593633
Commit-Queue: Xida Chen <xidachen@chromium.org>
Reviewed-by: Ian Kilpatrick <ikilpatrick@chromium.org>
Reviewed-by: Robert Flack <flackr@chromium.org>
Reviewed-by: Hiroki Nakagawa <nhiroki@chromium.org>
Reviewed-by: David Bokan <bokan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#491747}
[modify] https://crrev.com/1ab1fd870ccc6b3f020afdf7f4a049ba47ca6667/third_party/WebKit/Source/core/frame/LocalFrameView.cpp
[modify] https://crrev.com/1ab1fd870ccc6b3f020afdf7f4a049ba47ca6667/third_party/WebKit/Source/core/frame/LocalFrameView.h
[modify] https://crrev.com/1ab1fd870ccc6b3f020afdf7f4a049ba47ca6667/third_party/WebKit/Source/core/workers/Worklet.cpp
[modify] https://crrev.com/1ab1fd870ccc6b3f020afdf7f4a049ba47ca6667/third_party/WebKit/Source/core/workers/Worklet.h
[modify] https://crrev.com/1ab1fd870ccc6b3f020afdf7f4a049ba47ca6667/third_party/WebKit/Source/modules/csspaint/PaintWorklet.cpp
[modify] https://crrev.com/1ab1fd870ccc6b3f020afdf7f4a049ba47ca6667/third_party/WebKit/Source/modules/csspaint/PaintWorklet.h
[modify] https://crrev.com/1ab1fd870ccc6b3f020afdf7f4a049ba47ca6667/third_party/WebKit/Source/modules/csspaint/PaintWorkletTest.cpp

Status: Fixed (was: Assigned)
Status: Assigned (was: Fixed)
Re-open for more discussion on how to switch between the global scopes.
Project Member

Comment 13 by bugdroid1@chromium.org, Oct 24 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/44d8b0b0c55551eb15631973392d921edaf83b95

commit 44d8b0b0c55551eb15631973392d921edaf83b95
Author: Xida Chen <xidachen@chromium.org>
Date: Tue Oct 24 10:44:48 2017

[PaintWorklet] Change global scope switching logic

Our current switching logic is to switch to the other global scope every
120 paint frames.

This CL changes the logic so that the selection of global scope is
non-determinstic at any time. The idea is that we always choose a random
global scope when a new frame starts. Then within this frame, after a
certain amount of paint calls (rand(30)), we switch to the other global
scope. This CL also added a test to make sure that there is a maximum of
one switching within a frame.

Bug:  728595 
Change-Id: I3571217a47230ee8e8663df93a2ca11a299db3e3
Reviewed-on: https://chromium-review.googlesource.com/692176
Commit-Queue: Xida Chen <xidachen@chromium.org>
Reviewed-by: Hiroki Nakagawa <nhiroki@chromium.org>
Reviewed-by: Robert Flack <flackr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#511079}
[modify] https://crrev.com/44d8b0b0c55551eb15631973392d921edaf83b95/third_party/WebKit/Source/core/workers/Worklet.cpp
[modify] https://crrev.com/44d8b0b0c55551eb15631973392d921edaf83b95/third_party/WebKit/Source/core/workers/Worklet.h
[modify] https://crrev.com/44d8b0b0c55551eb15631973392d921edaf83b95/third_party/WebKit/Source/modules/csspaint/PaintWorklet.cpp
[modify] https://crrev.com/44d8b0b0c55551eb15631973392d921edaf83b95/third_party/WebKit/Source/modules/csspaint/PaintWorklet.h
[modify] https://crrev.com/44d8b0b0c55551eb15631973392d921edaf83b95/third_party/WebKit/Source/modules/csspaint/PaintWorkletTest.cpp

Status: Fixed (was: Assigned)

Sign in to add a comment