Closed Bug 1375913 Opened 2 years ago Closed 2 years ago
stylo: re-enable the style thread pool in the parent process
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.
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.
I mean, they *shouldn't* use anything.
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?
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?
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.
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.
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.
Let's enable it after turning on stylo-chrome, so that we can see what's the impact of things there.
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.
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 .  https://hg.mozilla.org/try/rev/22231bda44877a5a5c17b65e1c9b750118eabe5d
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.
Assignee: nobody → xidorn+moz
Priority: P4 → P2
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/autoland/rev/3954a5975f11 Do not disable thread pool in the parent process. r=bholley
You need to log in before you can comment on or make changes to this bug.