Closed Bug 1375913 Opened 2 years ago Closed 2 years ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox-esr52 --- unaffected
firefox56 --- unaffected
firefox57 --- wontfix
firefox58 --- wontfix
firefox59 --- wontfix
firefox60 blocking fixed

People

(Reporter: bholley, Assigned: xidorn)

References

Details

Attachments

(1 file)

41 bytes, text/x-github-pull-request
Details | Review
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.
Priority: -- → P5
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.
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.
Priority: P5 → P4
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 [1].

[1] https://hg.mozilla.org/try/rev/22231bda44877a5a5c17b65e1c9b750118eabe5d
Flags: needinfo?(xidorn+moz)
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
Blocks: 1438775
Priority: P4 → P2
Attached file Servo PR
Flags: needinfo?(xidorn+moz)
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3954a5975f11
Do not disable thread pool in the parent process. r=bholley
https://hg.mozilla.org/mozilla-central/rev/3954a5975f11
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.