Allow speculative evaluation #439

asajeffrey · 2017-08-01T18:40:57Z

Currently, user agents can only invoke worklets with consistent snapshots of the state. This PR allows user agents to invoke worklet scripts with inconsistent state, as long as they discard the result.

The particular instance of this is paint worklets, since the CSS style values are known before the concrete size, so in Servo we speculate that the size hasn't changed (servo/servo#17810).

Fixes #406.

tschneidereit

Some comments based on an IRC conversation with @asajeffrey.

tschneidereit · 2017-08-01T19:01:52Z

worklets/Overview.bs

+any time, and with any arguments, not just ones corresponding to
+consistent states of the user agent. The results of such speculative
+evaluations will be discarded, but may be cached for later use, thus
+increasing the concurrency between the user agent and worklet threads.


I think this paragraph is slightly misleading in two ways:

The methods won't be invoked with inconsistent state so much as with potential future state. The result will be used iff, when the time the invocation was done for has arrived, the actual state of the user agent matches what was passed in speculatively.

The results aren't discarded; they're not used immediately, but saved and hopefully used once the time the invocation was done for has arrived.

Good point, I tried rephrasing it.

tschneidereit · 2017-08-01T19:15:43Z

css-paint-api/Overview.bs

+
+Note: Although the image is discarded, it may still be cached, so subsequent invocations
+    of <<paint()>> may use the cached image.
+


Do we need to be this general about which value the function might be invoked with and when? Being more explicit about when and why a user agent might do this might be useful:

"The draw a paint image function may be speculatively invoked by the user agent with an expected value for |concreteObjectSize|. Once the |concreteObjectSize| is known, the resulting image either used if the speculation succeeded or discarded if the speculation failed."

I added text to the note: "Note: User agents may use whichever heuristics to speculate a possible future value for |concretObjectSize|, for example speculating that the size remains unchanged." I'm not sure that we want to say more about how to speculate in the normative text.

I replaced "discarded" by "not displayed", which is hopefully less confusing.

tschneidereit · 2017-08-01T20:47:56Z

css-paint-api/Overview.bs

    1. Let |paintFunction| be the <<paint()>> function on the |box| which the user agent wants to
        draw.

-    2. If the <a>paint valid flag</a> for the |paintFunction| is <a>paint-valid</a> the user agent
+    2. If the <a>paint valid flag</a> for the |paintFunction| is <a>paint-valid</a>,
+        and the previous invocation had the same |concreteObjectSize|, the user agent


This implies that there can only ever be one speculative evaluation at a time. If we want to allow speculating multiple frames ahead, this should probably say something about checking for results of a speculative evaluation that match the input state.

All the discussion of the paint valid flag is dubious, perhaps we should just delete it? It's very single-threaded. This is probably a separate issue though.

There is a separate section which allows for the caching inside "invoke a paint callback", see step 10.

@bfgeek yes, this is why I was thinking we could just remove all the stuff about the paint valid flag, since we're already allowing caching inside "invoke a paint callback". Shall I submit a separate PR for that?

Yeah that sounds good, we still need to describe how the inputProperties invalidate that image on a box, but we can have that in prose vs. as an algorithm.

OK, I'll file an issue for this.

asajeffrey

Submitted another commit with some edits.

asajeffrey · 2017-08-01T21:15:22Z

css-paint-api/Overview.bs

+
+Note: Although the image is discarded, it may still be cached, so subsequent invocations
+    of <<paint()>> may use the cached image.
+


I added text to the note: "Note: User agents may use whichever heuristics to speculate a possible future value for |concretObjectSize|, for example speculating that the size remains unchanged." I'm not sure that we want to say more about how to speculate in the normative text.

I replaced "discarded" by "not displayed", which is hopefully less confusing.

asajeffrey · 2017-08-01T21:17:22Z

css-paint-api/Overview.bs

    1. Let |paintFunction| be the <<paint()>> function on the |box| which the user agent wants to
        draw.

-    2. If the <a>paint valid flag</a> for the |paintFunction| is <a>paint-valid</a> the user agent
+    2. If the <a>paint valid flag</a> for the |paintFunction| is <a>paint-valid</a>,
+        and the previous invocation had the same |concreteObjectSize|, the user agent


All the discussion of the paint valid flag is dubious, perhaps we should just delete it? It's very single-threaded. This is probably a separate issue though.

asajeffrey · 2017-08-01T21:17:54Z

worklets/Overview.bs

+any time, and with any arguments, not just ones corresponding to
+consistent states of the user agent. The results of such speculative
+evaluations will be discarded, but may be cached for later use, thus
+increasing the concurrency between the user agent and worklet threads.


Good point, I tried rephrasing it.

tschneidereit

Thanks, this looks great to me.

bfgeek

lgtm

bfgeek · 2017-08-09T11:25:33Z

css-paint-api/Overview.bs

    1. Let |paintFunction| be the <<paint()>> function on the |box| which the user agent wants to
        draw.

-    2. If the <a>paint valid flag</a> for the |paintFunction| is <a>paint-valid</a> the user agent
+    2. If the <a>paint valid flag</a> for the |paintFunction| is <a>paint-valid</a>,
+        and the previous invocation had the same |concreteObjectSize|, the user agent


There is a separate section which allows for the caching inside "invoke a paint callback", see step 10.

bfgeek · 2017-08-09T11:26:35Z

worklets/Overview.bs

+Speculative Evaluation {#speculative-evaluation}
+------------------------------------------------
+
+Some specifications which use worklets ([[css-paint-api-1]]) may


please reflow to 100 colwidth.

bfgeek · 2017-08-09T11:27:22Z

css-paint-api/Overview.bs

@@ -450,6 +451,16 @@ Note: In a future version of the spec, the author could have the ability to spec
 The {{PaintSize}} object represents the size of the image that the author should draw. This is
 the <a>concrete object size</a> given by the user agent.

+The <a>draw a paint image</a> function may be speculatively invoked by


please reflow to 100 colwidth.

asajeffrey

Changed fill column.

asajeffrey · 2017-08-11T22:01:30Z

css-paint-api/Overview.bs

@@ -450,6 +451,16 @@ Note: In a future version of the spec, the author could have the ability to spec
 The {{PaintSize}} object represents the size of the image that the author should draw. This is
 the <a>concrete object size</a> given by the user agent.

+The <a>draw a paint image</a> function may be speculatively invoked by


asajeffrey · 2017-08-11T22:02:48Z

worklets/Overview.bs

+Speculative Evaluation {#speculative-evaluation}
+------------------------------------------------
+
+Some specifications which use worklets ([[css-paint-api-1]]) may


bfgeek

Looks great!

asajeffrey · 2017-11-10T00:40:01Z

Thanks!

Added text allowing speculative evaluation.

28084b3

tschneidereit suggested changes Aug 1, 2017

View reviewed changes

Responding to review comment.

5e309c7

asajeffrey commented Aug 1, 2017

View reviewed changes

tschneidereit approved these changes Aug 2, 2017

View reviewed changes

bfgeek requested changes Aug 9, 2017

View reviewed changes

Filled to column 100.

ac505d8

asajeffrey commented Aug 11, 2017

View reviewed changes

asajeffrey mentioned this pull request Aug 11, 2017

[css-paint-api] Unnecessary paint-valid flag? #447

Closed

bfgeek approved these changes Aug 15, 2017

View reviewed changes

bfgeek mentioned this pull request Aug 15, 2017

Custom Paint API to CR w3ctag/design-reviews#140

Closed

bfgeek added 2 commits November 9, 2017 08:47

Merge branch 'master' into speculative-evaluation

a0d3cba

Merge branch 'master' into speculative-evaluation

a2e7e63

bfgeek merged commit 36f9c91 into w3c:master Nov 9, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow speculative evaluation #439

Allow speculative evaluation #439

asajeffrey commented Aug 1, 2017

tschneidereit left a comment

tschneidereit Aug 1, 2017

asajeffrey Aug 1, 2017

tschneidereit Aug 1, 2017

asajeffrey Aug 1, 2017

tschneidereit Aug 1, 2017

asajeffrey Aug 1, 2017

bfgeek Aug 9, 2017

asajeffrey Aug 9, 2017

bfgeek Aug 10, 2017

asajeffrey Aug 11, 2017

asajeffrey left a comment

asajeffrey Aug 1, 2017

asajeffrey Aug 1, 2017

asajeffrey Aug 1, 2017

tschneidereit left a comment

bfgeek left a comment

bfgeek Aug 9, 2017

bfgeek Aug 9, 2017

asajeffrey Aug 11, 2017

bfgeek Aug 9, 2017

asajeffrey Aug 11, 2017

asajeffrey left a comment

asajeffrey Aug 11, 2017

asajeffrey Aug 11, 2017

bfgeek left a comment

asajeffrey commented Nov 10, 2017


		Note: Although the image is discarded, it may still be cached, so subsequent invocations
		of <<paint()>> may use the cached image.

Allow speculative evaluation #439

Allow speculative evaluation #439

Conversation

asajeffrey commented Aug 1, 2017

tschneidereit left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asajeffrey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tschneidereit left a comment

Choose a reason for hiding this comment

bfgeek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asajeffrey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bfgeek left a comment

Choose a reason for hiding this comment

asajeffrey commented Nov 10, 2017