Allow performance tests to test full rendering performance

Allow performance tests to test full rendering performance

Before we did paint invalidation after layout and compositing update,
blink performance tests reflected the whole document cycle performance
including almost everything from style recalc to invalidation. A simple
'element.offsetTop' could trigger such a cycle.

After change of compositing update and RACUN, the layout triggered
by 'element.offsetTop' no longer included cost of compositing update
and paint invalidation, causing incomplete performance coverage of the
performance tests.

Add PerfTestRunner.layoutOrFullyRender() which force a layout or
a full rendering (including style recalc, layout, compositing update
and paint invalidation, not including painting) according to global
variable window.fullRenderingMeasurement, and change the original
'element.offsetTop' statements in the performance tests to
PerfTestRunner.layoutOrFullyRender() calls.

Chromium side change https://codereview.chromium.org/681493002/ is
needed to expose internals API for blink_perf tests. With the change,
full rendering performance will be tested in new test suites named
like blink_perf.*_full_rendering.

BUG=426599
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=185484

[Xianzhu, 2014-10-24 22:09:52]

wangxianzhu@chromium.org changed reviewers:
+ esprehn@chromium.org, jchaffraix@chromium.org, leviw@chromium.org,
tonyg@chromium.org

[leviw_travelin_and_unemployed, 2014-10-24 22:48:12]

We weren't forcing layouts in this loop prior. Shouldn't we just replace calls
to offsetTop on tests that definitely want this instead of forcing all tests to
do forced compositing updates?

[Xianzhu, 2014-10-27 16:59:57]

On 2014/10/24 22:48:12, leviw wrote:
> We weren't forcing layouts in this loop prior. Shouldn't we just replace calls
> to offsetTop on tests that definitely want this instead of forcing all tests
to
> do forced compositing updates?

You are right. After examining each test, I found that
- some tests needs multiple forced layouts;
- some tests are not testing layout (e.g. Parser/*);
- some tests miss required forced layouts (e.g. SVG/SvgNestedUse.html).

Done.

[tonyg, 2014-10-27 17:35:34]

I defer to Levi, Elliot, Julien, et al on the review, but would suggest a change
to the description.

s/The performance baselines need to be reset after this change./PERF SHERIFFS:
This is expected to change blink_perf results./

[Xianzhu, 2014-10-27 17:43:46]

On 2014/10/27 17:35:34, tonyg wrote:
> I defer to Levi, Elliot, Julien, et al on the review, but would suggest a
change
> to the description.
> 
> s/The performance baselines need to be reset after this change./PERF SHERIFFS:
> This is expected to change blink_perf results./

Done.

[Julien - ping for review, 2014-10-28 16:37:10]

This change is going in the wrong direction.

Now we will have different numbers if run by DRT vs normal content_shell /
chromium, which is bad.

When we enabled RACUN, we were deeply aware of the impact on the tests and
though it was better to test what we *actually* ship instead of artificially
forcing compositing updates.

I would rather have a subset of the tests run under such artificial conditions
than invalidating our whole test suite for the sake of some performance coverage
for RACUN.

[Xianzhu, 2014-10-28 16:46:52]

On 2014/10/28 16:37:10, Julien Chaffraix - PST wrote:
> This change is going in the wrong direction.
> 
> Now we will have different numbers if run by DRT vs normal content_shell /
> chromium, which is bad.
> 
> When we enabled RACUN, we were deeply aware of the impact on the tests and
> though it was better to test what we *actually* ship instead of artificially
> forcing compositing updates.
> 
> I would rather have a subset of the tests run under such artificial conditions
> than invalidating our whole test suite for the sake of some performance
coverage
> for RACUN.

One good thing is: with this patch I got a list of tests whose cost of
compositing update and paint invalidation is significant.

I can copy these tests to PaintInvalidation directory, using the same method as
your original PaintInvalidation change, and keep the original tests unchanged.

[Julien - ping for review, 2014-10-28 17:26:38]

On 2014/10/28 at 16:46:52, wangxianzhu wrote:
> On 2014/10/28 16:37:10, Julien Chaffraix - PST wrote:
> > This change is going in the wrong direction.
> > 
> > Now we will have different numbers if run by DRT vs normal content_shell /
> > chromium, which is bad.
> > 
> > When we enabled RACUN, we were deeply aware of the impact on the tests and
> > though it was better to test what we *actually* ship instead of artificially
> > forcing compositing updates.
> > 
> > I would rather have a subset of the tests run under such artificial
conditions
> > than invalidating our whole test suite for the sake of some performance
coverage
> > for RACUN.
> 
> One good thing is: with this patch I got a list of tests whose cost of
compositing update and paint invalidation is significant.
> 
> I can copy these tests to PaintInvalidation directory, using the same method
as your original PaintInvalidation change, and keep the original tests
unchanged.

That's one way, an even better one: a virtual performance test suite where we
enable full painting only for some tests that we know give us good data. This
would avoid the duplication of tests that is a bad smell.

[Xianzhu, 2014-10-28 17:44:34]

On 2014/10/28 17:26:38, Julien Chaffraix - PST wrote:
> On 2014/10/28 at 16:46:52, wangxianzhu wrote:
> > On 2014/10/28 16:37:10, Julien Chaffraix - PST wrote:
> > > This change is going in the wrong direction.
> > > 
> > > Now we will have different numbers if run by DRT vs normal content_shell /
> > > chromium, which is bad.
> > > 
> > > When we enabled RACUN, we were deeply aware of the impact on the tests and
> > > though it was better to test what we *actually* ship instead of
artificially
> > > forcing compositing updates.
> > > 
> > > I would rather have a subset of the tests run under such artificial
> conditions
> > > than invalidating our whole test suite for the sake of some performance
> coverage
> > > for RACUN.
> > 
> > One good thing is: with this patch I got a list of tests whose cost of
> compositing update and paint invalidation is significant.
> > 
> > I can copy these tests to PaintInvalidation directory, using the same method
> as your original PaintInvalidation change, and keep the original tests
> unchanged.
> 
> That's one way, an even better one: a virtual performance test suite where we
> enable full painting only for some tests that we know give us good data. This
> would avoid the duplication of tests that is a bad smell.

I also thought of that way which would be based on Patch Set 4. Thought of two
ways to control the virtual test suite:
1. Run virtual test suite when content_shell --expose-internals-for-testing;
2. Let run_benchmark blink_perf set a JavaScript variable to control whether to
measure the full cycle.
2 seems better. Will explore more details.

[leviw_travelin_and_unemployed, 2014-10-28 17:45:04]

On 2014/10/28 at 17:26:38, jchaffraix wrote:
> That's one way, an even better one: a virtual performance test suite where we
enable full painting only for some tests that we know give us good data. This
would avoid the duplication of tests that is a bad smell.

+1. I like the notion of a virtual perf suite that ran just the perf test
directories we want to test this on.

[Xianzhu, 2014-10-28 17:52:57]

https://codereview.chromium.org/681493002 is the chromium side change for
virtual suite.

[Xianzhu, 2014-10-28 17:58:54]

PTAL. Patch Set 5 is the blink side change for virtual suite. There is just a
small change over Patch Set 4 to support window.fullLayoutMeasurement variable.

[Xianzhu, 2014-10-29 22:19:04]

ping...

[leviw_travelin_and_unemployed, 2014-10-30 20:33:53]

https://codereview.chromium.org/675983004/diff/100001/PerformanceTests/resour...
File PerformanceTests/resources/runner.js (right):

https://codereview.chromium.org/675983004/diff/100001/PerformanceTests/resour...
PerformanceTests/resources/runner.js:142: // Forcing full layout is only fully
feasible when window.internals is available.
I don't think this naming is right. Simply calling offsetTop *does* force a
layout. You can have many layouts in a frame without a compositing update, and
for better or worse that happens with some frequency in the wild. You're calling
this forceLayout, but you're really forcing a layout, a compositing update, and
an invalidation. This means running the state machine *most* of the way through
a frame, but not through rasterization (which is what differentiates this from a
rAF, along with synchronization).

[Xianzhu, 2014-10-30 21:29:12]

https://codereview.chromium.org/675983004/diff/100001/PerformanceTests/resour...
File PerformanceTests/resources/runner.js (right):

https://codereview.chromium.org/675983004/diff/100001/PerformanceTests/resour...
PerformanceTests/resources/runner.js:142: // Forcing full layout is only fully
feasible when window.internals is available.
On 2014/10/30 20:33:53, leviw wrote:
> I don't think this naming is right. Simply calling offsetTop *does* force a
> layout. You can have many layouts in a frame without a compositing update, and
> for better or worse that happens with some frequency in the wild. You're
calling
> this forceLayout, but you're really forcing a layout, a compositing update,
and
> an invalidation. This means running the state machine *most* of the way
through
> a frame, but not through rasterization (which is what differentiates this from
a
> rAF, along with synchronization).

I'm not sure what the correct name is. I thought styleRecalc, updateRenderTree,
layout, compositing update and invalidation were all parts of 'full-layout'
(before we get a better name, please allow me to use this term temporarily just
for discussion :) ), and separation of them were implementation-specific. In our
code, 'layout' seems to also mean the 'full layout', e.g. WebView::layout().
From the tests' point of view, PerfTestRunner.forceLayout() forces a layout,
which can be Document::updateLayout() or WebView::layout(), depending on the
value of window.fullLayoutMeasurement.

Thought of 'fullDocumentCycle' (corresponding to DocumentLifecycle), but that
seems to be too implementation-specific and not clear to whom are not familiar
with blink implementation.

The problem of rAF for these tests is that most of the tests run less than 10ms
for a single iteration which rAF couldn't measure them accurately. We could use
the method of JsGameBench which increases the complexity of the page until FPS
drops under 30fps, but that needs changes to the existing benchmarks and seems
not suitable for all tests.

Anyway, I'll hold off on this patch until we get a name agreed.

[leviw_travelin_and_unemployed, 2014-10-31 20:42:49]

On 2014/10/30 at 21:29:12, wangxianzhu wrote:
>
https://codereview.chromium.org/675983004/diff/100001/PerformanceTests/resour...
> File PerformanceTests/resources/runner.js (right):
> 
>
https://codereview.chromium.org/675983004/diff/100001/PerformanceTests/resour...
> PerformanceTests/resources/runner.js:142: // Forcing full layout is only fully
feasible when window.internals is available.
> On 2014/10/30 20:33:53, leviw wrote:
> > I don't think this naming is right. Simply calling offsetTop *does* force a
> > layout. You can have many layouts in a frame without a compositing update,
and
> > for better or worse that happens with some frequency in the wild. You're
calling
> > this forceLayout, but you're really forcing a layout, a compositing update,
and
> > an invalidation. This means running the state machine *most* of the way
through
> > a frame, but not through rasterization (which is what differentiates this
from a
> > rAF, along with synchronization).
> 
> I'm not sure what the correct name is. I thought styleRecalc,
updateRenderTree, layout, compositing update and invalidation were all parts of
'full-layout' (before we get a better name, please allow me to use this term
temporarily just for discussion :) ), and separation of them were
implementation-specific. In our code, 'layout' seems to also mean the 'full
layout', e.g. WebView::layout(). From the tests' point of view,
PerfTestRunner.forceLayout() forces a layout, which can be
Document::updateLayout() or WebView::layout(), depending on the value of
window.fullLayoutMeasurement.
> 
> Thought of 'fullDocumentCycle' (corresponding to DocumentLifecycle), but that
seems to be too implementation-specific and not clear to whom are not familiar
with blink implementation.
> 
> The problem of rAF for these tests is that most of the tests run less than
10ms for a single iteration which rAF couldn't measure them accurately. We could
use the method of JsGameBench which increases the complexity of the page until
FPS drops under 30fps, but that needs changes to the existing benchmarks and
seems not suitable for all tests.
> 
> Anyway, I'll hold off on this patch until we get a name agreed.

The notion of layout as running the entire Document Lifecycle is outdated and
something we should be trying to distance ourselves from. It's
implementation-specific of our *old* implementation.

What you effectively want is rAF without the synchronization. Something like
"pushFrame".

[Xianzhu, 2014-10-31 22:12:59]

On 2014/10/31 20:42:49, leviw wrote:
> 
> The notion of layout as running the entire Document Lifecycle is outdated and
> something we should be trying to distance ourselves from. It's
> implementation-specific of our *old* implementation.
> 

Understood.

As we are taking the 'virtual test suite' way, we need to make the name suitable
for both testing configurations: testing layout only, or testing whole document
cycle (not including painting which is not part of current DocumentLifeCycle).

How about the following names:
1. PerfTestRunner.layoutTestStep()
2. PerfTestRunner.layoutOrFullDocumentLifeCycle()
3. PerfTestRunner.layoutTestBarrier()
4. PrefTestRunner.render()  ('render' comes from our separation of rendering and
painting)
5. PerfTestRunner.layoutOrFullyRender()

> What you effectively want is rAF without the synchronization. Something like
> "pushFrame".

I'd not go this far. Just wanted to let performance tests cover paint
invalidation. For now we have coverage for parsing, layout, record, raster etc.,
just not paint invalidation.

[Xianzhu, 2014-11-12 18:59:17]

Any feedback to #18?

[Xianzhu, 2014-11-17 18:23:23]

On 2014/11/12 18:59:17, Xianzhu wrote:
> Any feedback to #18?

Any comments?

[Xianzhu, 2014-11-17 18:37:32]

On 2014/11/17 18:23:23, Xianzhu wrote:
> On 2014/11/12 18:59:17, Xianzhu wrote:
> > Any feedback to #18?
> 
> Any comments?

Uploaded a new patch set using the No. 5 naming proposal in #18, and updated CL
description. PTAL.

[Julien - ping for review, 2014-11-17 19:05:47]

lgtm

https://codereview.chromium.org/675983004/diff/120001/PerformanceTests/Shadow...
File PerformanceTests/ShadowDOM/ChangingSelect.html (left):

https://codereview.chromium.org/675983004/diff/120001/PerformanceTests/Shadow...
PerformanceTests/ShadowDOM/ChangingSelect.html:47: nonSelected.offsetLeft;
It's bit concerning that if people are not careful, they will just miss out our
function. Unfortunately offsetLeft / offsetTop / clientHeight are read-only
attributes so we can't just modify them on body to alleviate the issue.

https://codereview.chromium.org/675983004/diff/120001/PerformanceTests/resour...
File PerformanceTests/resources/runner.js (right):

https://codereview.chromium.org/675983004/diff/120001/PerformanceTests/resour...
PerformanceTests/resources/runner.js:143: PerfTestRunner.layoutOrFullyRender =
function(doc) {
Not a huge fan of this name (render is quite an overridden name in blink / cc).

How about forceLayoutOrFullFrame or something in that vein? </bikeshed>

[Xianzhu, 2014-11-17 20:44:12]

https://codereview.chromium.org/675983004/diff/120001/PerformanceTests/resour...
File PerformanceTests/resources/runner.js (right):

https://codereview.chromium.org/675983004/diff/120001/PerformanceTests/resour...
PerformanceTests/resources/runner.js:143: PerfTestRunner.layoutOrFullyRender =
function(doc) {
On 2014/11/17 19:05:46, Julien Chaffraix - PST wrote:
> Not a huge fan of this name (render is quite an overridden name in blink /
cc).
> 
> How about forceLayoutOrFullFrame or something in that vein? </bikeshed>

Renamed to forcelayoutOrFullFrame.

[Xianzhu, 2014-11-17 20:44:19]

The CQ bit was checked by wangxianzhu@chromium.org

[commit-bot: I haz the power, 2014-11-17 20:45:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/675983004/140001

[commit-bot: I haz the power, 2014-11-17 22:56:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-11-17 22:56:50]

Try jobs failed on following builders:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/36909)

[Xianzhu, 2014-11-17 23:31:24]

The CQ bit was checked by wangxianzhu@chromium.org

[commit-bot: I haz the power, 2014-11-17 23:32:47]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/675983004/140001

[commit-bot: I haz the power, 2014-11-18 01:47:21]

Committed patchset #8 (id:140001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=185484