stylo: avoid spinning up the style thread pool in the parent process

RESOLVED FIXED in Firefox 57

Status

()

Core
CSS Parsing and Computation
P2
normal
RESOLVED FIXED
8 months ago
4 months ago

People

(Reporter: bholley, Assigned: bholley)

Tracking

(Blocks: 2 bugs)

unspecified
mozilla57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

8 months ago
In bug 1374824 comment 9, glandium determined that avoiding firing up the rayon thread pool saves significant memory, since we have per-thread jemalloc arenas.

Given that we don't immediately plan to enable stylo for chrome documents, we should avoid creating this thread pool in the parent process. There might be some incidental stylo-driven styling of html documents going on in the parent process (which would currently trigger the thread-pool creation), but that should be fine to run in sequential mode.
(Assignee)

Updated

8 months ago
Blocks: 1375913
(Assignee)

Comment 1

8 months ago
Oh, and the patch itself here should be trivial - we just want to do an FFI call during threadpool creation (around where we check the STYLO_THREADS environmental variable), and force to None if we're in the parent process. Probably worth building in an env-var opt-out while we're at it.
We should make sure not to do this when e10s is disabled, so that our current e10s-disabled tests continue running in parallel mode.
(Assignee)

Updated

7 months ago
Priority: -- → P2
(Assignee)

Updated

6 months ago
Assignee: nobody → bobbyholley
(Assignee)

Comment 3

6 months ago
Created attachment 8898369 [details] [diff] [review]
Avoid creating a stylo thread pool in e10s parent processes. v1

MozReview-Commit-ID: LW92yNDKZf4
Attachment #8898369 - Flags: review?(xidorn+moz)
Attachment #8898369 - Flags: review?(xidorn+moz) → review+

Comment 4

6 months ago
Pushed by bholley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/74df08122c3d
Avoid creating a stylo thread pool in e10s parent processes. r=xidorn
(Assignee)

Comment 5

6 months ago
Servo-side changes: https://github.com/servo/servo/pull/18137

Comment 6

6 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/74df08122c3d
Status: NEW → RESOLVED
Last Resolved: 6 months ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Reading this code again, I actually get confused that, why do we need to do this at all?

STYLE_THREAD_POOL is a lazy static, and it is only dereferenced when traverse_subtree is called. It means that if no document is using stylo, then style threads would never be created. So why do we bother adding another gate at all?
(In reply to Xidorn Quan [:xidorn] UTC+10 (less responsive Nov 5 ~ Dec 16) from comment #7)
> Reading this code again, I actually get confused that, why do we need to do
> this at all?
> 
> STYLE_THREAD_POOL is a lazy static, and it is only dereferenced when
> traverse_subtree is called. It means that if no document is using stylo,
> then style threads would never be created. So why do we bother adding
> another gate at all?

It was done because there were about:blank tabs and what not that were calling into style, IIRC.
(In reply to Emilio Cobos Álvarez [:emilio] from comment #8)
> It was done because there were about:blank tabs and what not that were
> calling into style, IIRC.

OK, that makes sense, then. I guess we can still go ahead removing it anyway because we now have another gate added in bug 1398993 to disable stylo in e10s parent process.
Yup, that makes sense to me.
You need to log in before you can comment on or make changes to this bug.