1399583 - stylo: Investigate failing perf-reftests

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement, P2)

Description

[Bobby Holley (:bholley)]

So we have these tests that heycam wrote: https://github.com/heycam/style-perf-tests

Some of them were passing but started failing recently:
* the stop-cascade tests
* the some-descendants tests

Some of them have never passed because we don't implement the associated optimization:
* nth-index
* same-rule-node

Some of them I'm not sure:
* dep-check
* coalesce-1

We should investigate the first and third category, and see what's going on. We should also consider implementing the missing optimizations if we have any time.

Comment 1

[Bobby Holley (:bholley)]

Attachment: testresults.png

Test results from a local run on nightly.

Comment 2

[Cameron McCormack (:heycam)]

Attachment: test results

These are my test results.  It might be nice to have some sort of automatic calibration of what kTimeout and kEqualsTolerance and kEqualsMin should be.

The some-descendants-1.html failure might need some test adjustments to make the reference file times a fairer comparison.

For dep-check-1.html, this is generating rules like this:

.x[y~=v0] > span { ... }
.x[y~=v1] > span { ... }
...
.x[y~=v99] > span { ... }

In the test file, there are 50 copies of these 100 rules, and only one copy in the reference file.  The test starts with 1000 elements all with class="x v0 v1 v2 ... v99", and then measures how long the restyle takes after clearing all the classes.

I think my intention with the test was to make sure we were correctly skipping selector matching work when there are many dependencies to test.  We can do this, but only if we're at the end of the selector.  So changing the rules to this:

.x[y~=v0] { ... }
.x[y~=v1] { ... }
...
.x[y~=v99] { ... }

makes the test pass.

It's not simple to make the test as it is now pass.  We'd have to do a bunch of tracking / de-duplication of dependencies that we encounter based on what's in their selectors.

Comment 3

[Cameron McCormack (:heycam)]

(In reply to Cameron McCormack (:heycam) from comment #2)
> The some-descendants-1.html failure might need some test adjustments to make
> the reference file times a fairer comparison.

I just reduced the number of elements it's working on, so the timing of the test file is smaller, and under kEqualsMin.  (It may even be better to change it to a "<" test where the reference file is restyling all elements.)

Comment 4

[Cameron McCormack (:heycam)]

(In reply to Cameron McCormack (:heycam) from comment #2)
> It's not simple to make the test as it is now pass.  We'd have to do a bunch
> of tracking / de-duplication of dependencies that we encounter based on
> what's in their selectors.

I'm just going to change this so it tests what we do implement.

Comment 5

[Cameron McCormack (:heycam)]

With that, all the tests pass (on my machine at least) except for the nth-cache ones.  I don't think it's a hugely important optimization, so I think I'll just leave that as is.

Comment 6

[Cameron McCormack (:heycam)]

There's also same-rulenode-1.html which I momentarily forgot about.

That test is accidentally passing.  We don't implement that optimization (check to see if restyling gives the same rule node back, and if so, avoid creating a new CV).  It's passing because we end up utilizing the "if the there is no difference to computed values, consider stopping the traversal" optimization.  I could re-write the test to make sure it doesn't rely on that, but since the same rulenode check also isn't an important optimization, I'll just remove the test.

Comment 7

[Joel Maher ( :jmaher ) (UTC -8)]

I don't see a patch or any documentation of what was fixed, if this was done out of tree, can you provide a link to the work which was done?

Comment 8

[Cameron McCormack (:heycam)]

https://github.com/heycam/style-perf-tests/commit/a8ce1400f3a68d69672ea399a0132f7f4da76ac4
https://github.com/heycam/style-perf-tests/commit/5cdb77c88f69a0b6b3f406d1cef67738f4ee190d
https://github.com/heycam/style-perf-tests/commit/e3d1fe49c0a3fe8e15833a1a9cff3d789396582c
https://github.com/heycam/style-perf-tests/commit/6625b89d9b1417cf4541e78ceb38b18560ce808a

Comment 9

[Boris Zbarsky [:bzbarsky]]

Did Gecko pass dep-check-1.html with the complex selectors?

Comment 10

[Cameron McCormack (:heycam)]

(In reply to Boris Zbarsky [:bz] (still digging out from vacation mail) from comment #9)
> Did Gecko pass dep-check-1.html with the complex selectors?

Good question.  No, it didn't.  I think that is because we can't skip evaluating some selector in the rule hash early when we generate eRestyle_SomeDescendants, like we can with simpler restyle hints like eRestyle_Self or eRestyle_Subtree.

http://searchfox.org/mozilla-central/rev/2c9a5993ac40ec1db8450e3e0a85702fa291b9e2/layout/style/nsCSSRuleProcessor.cpp#2912-2919