Adding a SetRAILMode API.

Adding a SetRAILMode API.

BUG=chromium:613518
LOG=n

Committed: https://crrev.com/ba8ecfd58f62fa0ce762861758391038bf48e684
Cr-Commit-Position: refs/heads/master@{#36411}

[Hannes Payer (out of office), 2016-05-20 10:22:34]

hpayer@chromium.org changed reviewers:
+ jochen@chromium.org, rmcilroy@chromium.org, ulan@chromium.org

[rmcilroy, 2016-05-20 10:29:01]

API LGTM with a comment.

https://codereview.chromium.org/1999743002/diff/1/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/1999743002/diff/1/include/v8.h#newcode5363
include/v8.h:5363: PERFORMANCE_DEFAULT,
Could you add a comment to each of these describing the expectation of when it
would be set.

[Sami, 2016-05-20 10:37:32]

skyostil@chromium.org changed reviewers:
+ skyostil@chromium.org

[Sami, 2016-05-20 10:37:32]

https://codereview.chromium.org/1999743002/diff/1/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/1999743002/diff/1/include/v8.h#newcode5363
include/v8.h:5363: PERFORMANCE_DEFAULT,
On 2016/05/20 10:29:00, rmcilroy wrote:
> Could you add a comment to each of these describing the expectation of when it
> would be set.

+1, a short snippet per state would be useful -- especially if there are
downsides to sticking in a particular mode for too long.

[Hannes Payer (out of office), 2016-05-20 11:02:41]

https://codereview.chromium.org/1999743002/diff/1/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/1999743002/diff/1/include/v8.h#newcode5363
include/v8.h:5363: PERFORMANCE_DEFAULT,
On 2016/05/20 at 10:37:32, Sami wrote:
> On 2016/05/20 10:29:00, rmcilroy wrote:
> > Could you add a comment to each of these describing the expectation of when
it
> > would be set.
> 
> +1, a short snippet per state would be useful -- especially if there are
downsides to sticking in a particular mode for too long.

Done.

[Sami, 2016-05-20 11:08:03]

https://codereview.chromium.org/1999743002/diff/40001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/1999743002/diff/40001/include/v8.h#newcode5370
include/v8.h:5370: // Animation performance mode: In this mode low virtual
machine latency is
In my mind RESPONSE is the one where we'd want the absolute lowest latency and
ANIMATION is where we can tolerate the occasional missed frame. Are my wires
crossed?

In the scheduler we'd generally be in RESPONSE for very short durations.

[rmcilroy, 2016-05-20 11:12:52]

https://codereview.chromium.org/1999743002/diff/40001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/1999743002/diff/40001/include/v8.h#newcode5370
include/v8.h:5370: // Animation performance mode: In this mode low virtual
machine latency is
On 2016/05/20 11:08:03, Sami wrote:
> In my mind RESPONSE is the one where we'd want the absolute lowest latency and
> ANIMATION is where we can tolerate the occasional missed frame. Are my wires
> crossed?
> 
> In the scheduler we'd generally be in RESPONSE for very short durations.

+1 - response would be for something where the animations are following user
input (e.g., scrolling) so latency would be super important, whereas animation
is where there are non-user-input related animations, where occasional dropped
frames and slight delay in doing the animation doesn't matter quite as much.

[ulan, 2016-05-20 11:15:57]

https://codereview.chromium.org/1999743002/diff/40001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/1999743002/diff/40001/include/v8.h#newcode5360
include/v8.h:5360: * Option flags passed to the SetRAILMode function.
Link
https://developers.google.com/web/tools/chrome-devtools/profile/evaluate-perf...
?

[Hannes Payer (out of office), 2016-05-20 11:30:13]

On 2016/05/20 at 11:15:57, ulan wrote:
> https://codereview.chromium.org/1999743002/diff/40001/include/v8.h
> File include/v8.h (right):
> 
> https://codereview.chromium.org/1999743002/diff/40001/include/v8.h#newcode5360
> include/v8.h:5360: * Option flags passed to the SetRAILMode function.
> Link
https://developers.google.com/web/tools/chrome-devtools/profile/evaluate-perf...
?

Response: respond in under 100ms
Animation: render frames every 16ms

Interesting. I guess it depends on the point of view. If response is just one
event followed by many animation events then I agree, this mode should be super
low latency.

[Hannes Payer (out of office), 2016-05-20 12:07:15]

After discussing online, I changed the performance requirements of R.

[Hannes Payer (out of office), 2016-05-20 12:11:24]

hpayer@chromium.org changed reviewers:
+ ulan@chromium.or
- ulan@chromium.org

[Hannes Payer (out of office), 2016-05-20 12:11:24]

Added the url

[Sami, 2016-05-20 12:28:11]

lgtm, thanks for making sure we're talking about the same letters :)

[ulan, 2016-05-20 12:52:11]

ulan@chromium.org changed reviewers:
+ ulan@chromium.org

[ulan, 2016-05-20 12:52:12]

lgtm

https://codereview.chromium.org/1999743002/diff/80001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/1999743002/diff/80001/include/v8.h#newcode5362
include/v8.h:5362: * profile/evaluate-performance/rail
Maybe mention that the API is experimental and can change?

[Hannes Payer (out of office), 2016-05-20 13:03:00]

The CQ bit was checked by hpayer@chromium.org

[Hannes Payer (out of office), 2016-05-20 13:03:00]

The patchset sent to the CQ was uploaded after l-g-t-m from
rmcilroy@chromium.org, ulan@chromium.org, skyostil@chromium.org
Link to the patchset: https://codereview.chromium.org/1999743002/#ps100001
(title: " ")

[commit-bot: I haz the power, 2016-05-20 13:03:12]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1999743002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1999743002/100001

[Hannes Payer (out of office), 2016-05-20 13:03:38]

hpayer@chromium.org changed reviewers:
- ulan@chromium.or

[commit-bot: I haz the power, 2016-05-20 13:36:11]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-05-20 13:37:58]

Description was changed from

==========
Adding a SetRAILMode API.

BUG=chromium:613518
LOG=n
==========

to

==========
Adding a SetRAILMode API.

BUG=chromium:613518
LOG=n

Committed: https://crrev.com/ba8ecfd58f62fa0ce762861758391038bf48e684
Cr-Commit-Position: refs/heads/master@{#36411}
==========

[commit-bot: I haz the power, 2016-05-20 13:37:59]

Patchset 6 (id:??) landed as
https://crrev.com/ba8ecfd58f62fa0ce762861758391038bf48e684
Cr-Commit-Position: refs/heads/master@{#36411}

[alph, 2016-05-24 21:16:57]

alph@chromium.org changed reviewers:
+ alph@chromium.org

[alph, 2016-05-24 21:16:58]

Just noticed the CL.

https://codereview.chromium.org/1999743002/diff/100001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/1999743002/diff/100001/include/v8.h#newcode5371
include/v8.h:5371: PERFORMANCE_RESPONSE,
The mode names look misleading. They should not be browser driven. How about
make them somewhat embedder agnostic, e.g. OPTIMIZE_LATENCY, OPTIMIZE_THROUGHPUT
etc.

https://codereview.chromium.org/1999743002/diff/100001/include/v8.h#newcode5375
include/v8.h:5375: PERFORMANCE_ANIMATION,
It's not clear what's the difference from RESPONSE.

[alph, 2016-05-24 21:20:56]

+pfeldman

[pfeldman, 2016-05-24 23:36:00]

pfeldman@chromium.org changed reviewers:
+ pfeldman@chromium.org

[pfeldman, 2016-05-24 23:36:01]

https://codereview.chromium.org/1999743002/diff/100001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/1999743002/diff/100001/include/v8.h#newcode5381
include/v8.h:5381: PERFORMANCE_LOAD
+1 to what alph@ is saying. not only v8 should operate v8 terms as opposed to
the embedder terms, but RAIL by itself is more of a product terminology rather
than a technical one. There is no consensus on the load phase characteristics,
it can be substituted by another model, etc.