1497389 - Slow spacebar/page up/page down scroll on Uniqlo site

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P3)

Description

[Asif Youssuff]

STR:

1. Navigate to: https://www.uniqlo.com/us/en/men/sale
2. Scroll using spacebar or page up/page down

What happens:

Scrolling starts slowly and seems slightly delayed while scrolling. 

Expected result:

Fast scrolling, like other pages. This issue does not affect Chrome, FWIW. 

Performance profile: https://perfht.ml/2NvUXny

Comment 1

[Xidorn Quan [:xidorn] UTC+10]

I did another profile: https://perfht.ml/2CvUznp

Both seem to indicate the same problem that lots of problem are spent in select matching invoked from querySelectorAll. It may worth knowing why is that slow.

Comment 2

[Xidorn Quan [:xidorn] UTC+10]

From the profile it feels like the same queries are invoked multiple times in each scroll? Maybe we can cache the result as far as no change has been done on the page which may affect the result?

This long query doesn't happen only when using space or page up/down. It happens when using wheel as well. But IIRC, we enable the APZ when using wheel, but not when using keyboard?

Comment 3

[Emilio Cobos Álvarez (:emilio)]

The relevant scroll listener (with a bit of formatting) is:

$(window).bind("scroll", function() {
  var a = 0 == $(".primary-content .content-slot").first().height()
    ? $(".primary-content .content-slot").first().find(".contentsection-1").height() + 162 + $(".header-banner").height()
    : $(".primary-content .content-slot").first().height() + 162 + $(".header-banner").height();
  $(window).scrollTop() > a
    ? $(".refinements.producthits, .breadcrumb").addClass("fixed")
    : $(".refinements.producthits, .breadcrumb").removeClass("fixed")
})

It may be worth profiling the body of that function to see if other browsers are faster at that, or if it's just that they asynchronously scroll with the keyboard as well.

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #2)
> This long query doesn't happen only when using space or page up/down. It
> happens when using wheel as well. But IIRC, we enable the APZ when using
> wheel, but not when using keyboard?

I we enable APZ for the keyboard scrolling as well, or at least apz.keyboard.enabled is true here... Ryan, do you know if there's something going wrong with that here?

Comment 4

[Emilio Cobos Álvarez (:emilio)]

I guess that the listener is not passive and thus we cannot use APZ for that?

Comment 5

[Emilio Cobos Álvarez (:emilio)]

So running the following code in the console:

function listener() {
  var a = 0 == $(".primary-content .content-slot").first().height()
    ? $(".primary-content .content-slot").first().find(".contentsection-1").height() + 162 + $(".header-banner").height()
    : $(".primary-content .content-slot").first().height() + 162 + $(".header-banner").height();
  $(window).scrollTop() > a
    ? $(".refinements.producthits, .breadcrumb").addClass("fixed")
    : $(".refinements.producthits, .breadcrumb").removeClass("fixed")
}

var start = performance.now(); for (let i = 0; i < 1000; ++i) listener(); console.log(performance.now() - start);

Gives me ~900ms in Firefox vs. ~700ms in Chrome.

So yeah, there's stuff to improve here, but it's not disproportionate, so I think Chrome ends up scrolling async even for the keyboard case.

Style-system-wise, I think this may just be an artifact of Chrome having a particular optimization for class names in ancestor combinators in querySelector, which does hit in ".primary-content .content-slot"...

But could do some more profiling :)

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Here's a profile of that function over and over: https://perfht.ml/2R6bbGy

Which is not terribly helpful, but makes me wonder why ".primary-content .content-slot" isn't hitting one of the fast paths for querySelector.

I thought we implemented something like that, but apparently for complex selectors we only handle id selectors:

  https://searchfox.org/mozilla-central/rev/4fc7bfa931189e0718dd70b4c59885829f1d761e/servo/components/style/dom_apis.rs#418

I'm pretty sure the fact that we don't hit the fast path for those and Chrome does have a fast-path for this case:

  https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/css/selector_query.cc?l=227&rcl=49fe04d0a39a638f31706cb28f939e6682d2f354

Explains the difference here. I could write that one, should be easy enough.

Comment 7

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #3)
> The relevant scroll listener (with a bit of formatting) is:
> 
> $(window).bind("scroll", function() {
>   var a = 0 == $(".primary-content .content-slot").first().height()
>     ? $(".primary-content
> .content-slot").first().find(".contentsection-1").height() + 162 +
> $(".header-banner").height()
>     : $(".primary-content .content-slot").first().height() + 162 +
> $(".header-banner").height();
>   $(window).scrollTop() > a
>     ? $(".refinements.producthits, .breadcrumb").addClass("fixed")
>     : $(".refinements.producthits, .breadcrumb").removeClass("fixed")
> })

This also seems to be a case that a cache may be helpful since it queries the same selector several times... But probably maintaining such cache may add some complexity and runtime burden for unrelated cases.

(I'm wondering whether it is possible to have JIT optimize this case to invoke the query only once...)

Comment 8

[Emilio Cobos Álvarez (:emilio)]

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #7)
> This also seems to be a case that a cache may be helpful since it queries
> the same selector several times... But probably maintaining such cache may
> add some complexity and runtime burden for unrelated cases.

Yeah, caching the elements that match a query means we need to invalidate it, which means adding complexity around (depending on which selectors we cache).

But at least it needs to invalidate the cached when elements leave and are inserted into the document, which looks like a huge pain and are very-hot codepaths.

> (I'm wondering whether it is possible to have JIT optimize this case to
> invoke the query only once...)

Sounds really unlikely. I think to get this optimized by the jit we should mark the function as [Pure], but the function is indeed not really pure, so it'd be unsound if you mutated the DOM from that function...

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Moved the APZ issue to bug 1500745. Will submit a patch here to improve querySelector perf in this test-case.

Comment 10

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1497389 - Add a fast path for querySelector{,All} when we have classes or tags in the rightmost compound.

Before this patch we were only optimizing the case of a single selector, which
is fine, but not enough to catch ones like .foo .bar or so.

This patch allows us to optimize classes and tags in the rightmost compound,
while keeping the current optimization for #id selectors.

Need to profile this, but code-wise should be ready for review.

Comment 11

[Emilio Cobos Álvarez (:emilio)]

This improves a bit dromaeo-css, and makes us faster than chrome in that test-case.

Comment 12

[Emilio Cobos Álvarez (:emilio)]

https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&newProject=try&newRevision=e198e8ab5552&framework=1&showOnlyImportant=1&selectedTimeRange=172800

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7a6abfe37ce5
Add a fast path for querySelector{,All} when we have classes or tags in the rightmost compound. r=heycam,firefox-style-system-reviewers

Comment 15

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/7a6abfe37ce5

Comment 16

[Ionuț Goldan [:igoldan]]

Noticed some performance improvements! \0/

== Change summary for alert #17207 (as of Mon, 29 Oct 2018 10:26:56 GMT) ==

Improvements:

  6%  dromaeo_css windows7-32 pgo e10s stylo      14,373.66 -> 15,191.24
  5%  dromaeo_css windows7-32 opt e10s stylo      13,920.36 -> 14,604.23
  4%  dromaeo_css osx-10-10 opt e10s stylo        8,642.20 -> 9,009.02
  4%  dromaeo_css windows10-64 opt e10s stylo     13,591.89 -> 14,080.25
  3%  dromaeo_css windows10-64 pgo e10s stylo     14,101.12 -> 14,549.25
  3%  dromaeo_css linux64 pgo e10s stylo          15,883.64 -> 16,335.59

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=17207

Comment 17

[Mayank Bansal]

HEre is a profile on the latest nightly, with the testcase of bug 1468739

http://bit.ly/2RrRoS2

(56% time spent in Servo_SelectorList_QueryAll)

Comment 18

[Emilio Cobos Álvarez (:emilio)]

Yeah, the page is still calling querySelectorAll a lot, I don't think we should do a lot more than that though. This is not really slower than other browsers either.