Closed Bug 1398993 Opened 7 years ago Closed 7 years ago

stylo: disable stylo in e10s parent process

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox-esr52 --- wontfix
firefox55 --- wontfix
firefox56 --- wontfix
firefox57 --- fixed

People

(Reporter: heycam, Assigned: heycam)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

This wins back around 1% of the AWSY regression versus Gecko.  We'll turn this back on once we support Stylo in chrome documents.
Blocks: 1398996
Comment on attachment 8906881 [details]
Bug 1398993 - Disable stylo in e10s parent process.

https://reviewboard.mozilla.org/r/178614/#review183610
Attachment #8906881 - Flags: review?(xidorn+moz) → review+
Assignee: nobody → cam
Priority: -- → P3
Pushed by cmccormack@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a4bfe93c85a4
Disable stylo in e10s parent process. r=xidorn
https://hg.mozilla.org/mozilla-central/rev/a4bfe93c85a4
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Ryan can explain the details, but this patch, by virtue of turning off Stylo in the GPU process, got read of some leaks we had.  Thanks!
Flags: needinfo?(ryanvm)
Flags: needinfo?(ryanvm)
This changed our perf baseline on AWSY:

== Change summary for alert #9445 (as of September 14 2017 02:57 UTC) ==

Improvements:

 21%  Images summary windows7-32 opt      5,176,929.38 -> 4,071,628.24
 19%  Images summary windows10-64 opt     6,965,582.40 -> 5,613,002.05
 19%  Images summary windows7-32 pgo      5,098,415.24 -> 4,115,874.98
 16%  Images summary linux64 opt          5,779,725.11 -> 4,854,304.15
 16%  Images summary osx-10-10 opt        5,981,148.69 -> 5,047,666.32
 15%  Images summary linux64-stylo-sequential opt stylo-sequential5,805,700.74 -> 4,939,600.63

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=9445
Hmm... we still have so much to optimize :/
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #11)
> Hmm... we still have so much to optimize :/

Well, this landed shortly before our other big memory optimizations. The big ones that would affect this would be bug 1397101 and bug 1362532. So this may not actually be necessary anymore.



> -  InitializeServo();
> +  if (XRE_IsParentProcess() || XRE_IsContentProcess()) {
> +    InitializeServo();
> +  }

Isn't this effectively a no-op? Shouldn't we be using StyloSupportedInCurrentProcess instead?
Flags: needinfo?(cam)
XRE_IsParentProcess() only returns true for a non-e10s parent process.  We can't call StyloSupportedInCurrentProcess, since it calls XRE_IsE10sParentProcess(), which returns an incorrect values during nsLayoutStatics::Initialize since some e10s-disabling prefs haven't been read by that point.  So at this point here, all we can know is whether we're some kind of parent process, not whether e10s is enabled or not.

By checking XRE_IsParentProcess() || XRE_IsContentProcess(), we do unnecessarily initialize Servo in a soon-to-be-decided-e10s parent process, but we still avoid initializing it in other processes, like the GPU compositor processor.
Flags: needinfo?(cam)
Backout of this bug also reflected some sessionrestore improvements:

== Change summary for alert #9437 (as of September 14 2017 02:57 UTC) ==

Improvements:

  3%  sessionrestore windows10-64 pgo e10s     426.75 -> 412.50
  3%  sessionrestore_no_auto_restore windows10-64 pgo e10s504.58 -> 489.33
  1%  sessionrestore windows10-64 opt e10s     509.46 -> 503.67

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=9437
(In reply to Ionuț Goldan [:igoldan], Performance Sheriffing from comment #14)
> Backout of this bug also reflected some sessionrestore improvements:

As far as I know this bug hasn't been backed out.  By this do you mean that if we did back the bug out, it would make sessionrestore faster?
Flags: needinfo?(ionut.goldan)
Sorry, I didn't express myself correctly. I wanted to say that bug 1398993 improved sessionrestore results.
Flags: needinfo?(ionut.goldan)
Thanks, good news. :-)
(In reply to Cameron McCormack (:heycam) from comment #13)
> XRE_IsParentProcess() only returns true for a non-e10s parent process.

That contradicts [1], and my general mental model of things, which is that XRE_IsParentProcess returns true for either the e10s parent process or the main process in non-e10s mode. Though per below, maybe what you wrote isn't what you meant?

> We
> can't call StyloSupportedInCurrentProcess, since it calls
> XRE_IsE10sParentProcess(), which returns an incorrect values during
> nsLayoutStatics::Initialize since some e10s-disabling prefs haven't been
> read by that point.  So at this point here, all we can know is whether we're
> some kind of parent process, not whether e10s is enabled or not.
> 
> By checking XRE_IsParentProcess() || XRE_IsContentProcess(), we do
> unnecessarily initialize Servo in a soon-to-be-decided-e10s parent process,
> but we still avoid initializing it in other processes, like the GPU
> compositor processor.

Ok, so that check isn't really about nochrome so much as it is about avoiding initializing the thread pool overhead in the compositor and GMP processes?

That wasn't really my understanding of what we were doing here, since we'll still have a bunch of unnecessary thread pool arenas in the parent process. But given that memory is at an acceptable level, and given that we'll be changing this back again in 58, it may not matter much.

[1] http://searchfox.org/mozilla-central/rev/1c13d5cf85f904afb8976c02a80daa252b893fca/toolkit/xre/nsAppRunner.cpp#5062
Flags: needinfo?(cam)
(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #18)
> That contradicts [1], and my general mental model of things, which is that
> XRE_IsParentProcess returns true for either the e10s parent process or the
> main process in non-e10s mode. Though per below, maybe what you wrote isn't
> what you meant?

So it does; perhaps I misread.  In any case...

> > can't call StyloSupportedInCurrentProcess, since it calls
> > XRE_IsE10sParentProcess(), which returns an incorrect values during
> > nsLayoutStatics::Initialize since some e10s-disabling prefs haven't been
> > read by that point.  So at this point here, all we can know is whether we're
> > some kind of parent process, not whether e10s is enabled or not.
> > 
> > By checking XRE_IsParentProcess() || XRE_IsContentProcess(), we do
> > unnecessarily initialize Servo in a soon-to-be-decided-e10s parent process,
> > but we still avoid initializing it in other processes, like the GPU
> > compositor processor.
> 
> Ok, so that check isn't really about nochrome so much as it is about
> avoiding initializing the thread pool overhead in the compositor and GMP
> processes?

Yes, that's what I fell back to when I discovered that at this point we can't know whether we're e10s or not.

> That wasn't really my understanding of what we were doing here, since we'll
> still have a bunch of unnecessary thread pool arenas in the parent process.
> But given that memory is at an acceptable level, and given that we'll be
> changing this back again in 58, it may not matter much.

OK.  I think I guessed it not using Stylo for the SVG images in the parent process was more important, although that effect probably diminished with the other savings we made.  Agree that it doesn't matter too much now.
Flags: needinfo?(cam)
You need to log in before you can comment on or make changes to this bug.