1375913 - stylo: re-enable the style thread pool in the parent process

Status: RESOLVED FIXED

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

Description

[Bobby Holley (:bholley)]

In bug 1375911, we're forcing the thread pool to None for the parent process because we don't need it and doing so saves memory. We'll want to undo this hack when we start styling chrome documents, otherwise we won't get any parallelism benefits from doing so.

Comment 1

[Xidorn Quan [:xidorn] UTC+10]

I think we can simply remove the additional gate there, since the thread pool is initialized lazily when it is first requested, which means they should use anything if we don't call into stylo at all.

Comment 2

[Xidorn Quan [:xidorn] UTC+10]

I mean, they *shouldn't* use anything.

Comment 3

[Xidorn Quan [:xidorn] UTC+10]

After creating the patch to remove the disabling code, I actually become wondering whether we should really re-enable it.

Since chrome code is controlled by us, and it seems to be pretty good with a single-thread style system, is it really worth the additional memory usage to have a thread pool? Or is the thread pool usage not really a big deal?

Comment 4

[Xidorn Quan [:xidorn] UTC+10]

I mean, is the memory usage of a thread pool not really a big deal so it doesn't matter whether we enable it or not?

Comment 5

[Bobby Holley (:bholley)]

Back in the spring, Ehsan was very concerned about browser.xul restyles, and was very interested in getting stylo for chrome. Would be worth checking in with him about context there.

Comment 6

[Xidorn Quan [:xidorn] UTC+10]

So I did two try pushes for stylo-chrome with thread pool enabled vs. disabled:
* disabled: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4beec9d49cd731c957c1bb1a9cf8dafffec0161f (base)
* enabled: https://treeherder.mozilla.org/#/jobs?repo=try&revision=285fdf81397d2212a5bb8847d311059fc7e4fae7 (new)

The performance comparison shows both improvement and regression: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=4beec9d49cd731c957c1bb1a9cf8dafffec0161f&newProject=try&newRevision=285fdf81397d2212a5bb8847d311059fc7e4fae7&framework=1

And awsy: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=4beec9d49cd731c957c1bb1a9cf8dafffec0161f&newProject=try&newRevision=285fdf81397d2212a5bb8847d311059fc7e4fae7&framework=4

Doesn't seem to have a big regression on memory though.


Also note that it seems enabling stylo-chrome may lead to significant performance regression as shown in https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&originalRevision=3f627210c55d&newProject=try&newRevision=4beec9d49cd731c957c1bb1a9cf8dafffec0161f&framework=1&showOnlyImportant=1

I don't currently have idea how should we mitigate that.

Comment 7

[Chris Peterson [:cpeterson]]

bz said he was less concerned about the Stylo thread pool's extra memory usage in the parent process than in the content processes *if* we see a performance improvement.

Comment 8

[Xidorn Quan [:xidorn] UTC+10]

Let's enable it after turning on stylo-chrome, so that we can see what's the impact of things there.

Comment 9

[Firefox Bug Husbandry Bot]

https://wiki.mozilla.org/Bug_Triage/Projects/Bug_Handling/Bug_Husbandry#Move_fix-optionals

Comment 10

[Xidorn Quan [:xidorn] UTC+10]

The latest comparison between single-thread and multi-thread stylo-chrome: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=50613c74a44f&newProject=try&newRevision=1265d0e36d7b&framework=1

It seems to improve the result, but most of them don't have a strong confidence.

Comment 11

[Bobby Holley (:bholley)]

I did a bunch of retriggers, and it looks like it's about a 4% win on tpaint and sessionrestore.

Let's get this in. r=bholley on the patch in [1].

[1] https://hg.mozilla.org/try/rev/22231bda44877a5a5c17b65e1c9b750118eabe5d

Comment 12

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

BHolley mentioned plans to flip this pref in order to less the tpaint regression in 60. Tracked as blocking 60 so it gets the necessary attention.

Comment 13

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Servo PR

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3954a5975f11
Do not disable thread pool in the parent process. r=bholley

Comment 15

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/3954a5975f11