Closed Bug 1196094 Opened 4 years ago Closed 3 months ago

Use e10s when creating the PGO profile

Categories

(Firefox Build System :: General, defect)

defect
Not set

Tracking

(e10s+, firefox69 fixed)

RESOLVED FIXED
mozilla69
Tracking Status
e10s + ---
firefox69 --- fixed

People

(Reporter: ehsan, Assigned: Gijs)

References

(Depends on 2 open bugs, Blocks 2 open bugs)

Details

(Whiteboard: [qf:p1:pageload][fxperf:p1])

Attachments

(2 files)

Right now build/pgo/profileserver.py uses the prefs from testing/profiles/prefs_general.js.  That file disables e10s right now, so the profile generation phase of the PGO build happens in non-e10s mode, which will give us less realistic PGO profiles.
Enabling e10s in the profiling run should be as simple as doing:
if build.substs['RELEASE_BUILD'] != '1':
  prefs['browser.tabs.remote.autostart.2'] = True

right here:
https://dxr.mozilla.org/mozilla-central/source/build/pgo/profileserver.py?offset=300#50

Assuming that e10s doesn't break anything the profiling run is doing in-browser this should be fine.
Should we do this for all prefs that have remote.autostart in them to future proof this against the .3 pref and so on?
Yeah, that seems reasonable.
I was going to write a quick patch for this, but then I realized that we only want this behavior where e10s is enabled, which is... well, hard to tell by just looking at a pref given how we've been adding .1, .2 etc prefs, so I think I'll leave this to someone else.  Ideally we should have one master toggle for enabling e10s by default.
I think conditioning it on RELEASE_BUILD like I said in comment 1 should be fine, since that's what we're using in the prefs file.
Yes, but there is nothing to make sure that they will be tied together.
Priority: -- → P2
Is this still valid?
It would be nice to have better e10s performance on the nightlies.
Product: Core → Firefox Build System
See Also: → 1552425

We're still doing PGO based on non-e10s, so updating the summary for that. I expect we're leaving perf gains (at least in IPC code and child process startup paths) up for grabs by not fixing this, so adding some flags to radar this for perf / prioritization.

Priority: P2 → --
Summary: Use e10s when creating PGO profile when e10s is enabled by default → Use e10s when creating the PGO profile
Whiteboard: [qf][fxperf]

I'm investigating. Off-hand it looks like we're not writing a separate file when e10s is turned on, perhaps because of sandboxing, or perhaps llvm is trying to write to the same file for the child process as well, or something.

Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED

I have something working, but it depends on the work in bug 1542746. It does look like there are some perf wins on Windows here ( https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=e5ac79268e81fe143126dccc559de61ab8663fed&newProject=try&newRevision=dab915f464a6d9f493bd1eb58434576efa77df85&framework=1 ) that I think are caused by the e10s switch rather than bug 1542746, though I'm waiting on retriggers (there's a windows raptor/talos backlog) and there's some small issues with the infra pipeline for the work in bug 1542746 for linux64, so a bit early to draw firm conclusions - looks promising though!

Depends on: 1542746
Blocks: 1553850
See Also: → 1553894
Whiteboard: [qf][fxperf] → [qf:p1:pageload][fxperf]
Whiteboard: [qf:p1:pageload][fxperf] → [qf:p1:pageload][fxperf:p1]
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/8b110b9889c3
use e10s when doing PGO profiling, r=froydnj
Status: ASSIGNED → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla69

Backed out changeset 8b110b9889c3 as requested by Gijs.

Backout link: https://hg.mozilla.org/integration/autoland/rev/b047e2089a784e9e5d08e8057ac4bef6411bf47e

Flags: needinfo?(gijskruitbosch+bugs)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla69 → ---
Depends on: 1555311

A few notes as I dig into this:

  1. the current set of files output by the profiler is ambiguous - it's basically meant to use a pool of randomly named files to avoid filename conflicts between multiple runs.
  2. it does not include the pid, so no way to see if a child process wrote anything
  3. good news, we can use an env var to control this filename and make it include the pid
  4. bad news, doing so shows the child process writes nada/nothing/niente.
  5. turning off sandboxing seems like a red herring - I think there was something wrong with my windows setup (or perhaps with windows sandboxing around the time I wrote the patch or whatever). It makes no difference here atm (though it is of course possible that after fixing whatever the issue is we have right now, we'd still run into sandboxing)
  6. profiles are dumped at exit and kept in memory
  7. searching the web finds me http://mysql-qa.blogspot.com/2009/06/kill-exit-exit-and-issues-getting-gcov.html . Our child process uses _exit, as far as I can tell, presumably because there shouldn't be anything to do when it exits?
  8. Annnnd looking for code in our codebase brings me to https://searchfox.org/mozilla-central/rev/7556a400affa9eb99e522d2d17c40689fa23a729/dom/ipc/ContentChild.cpp#2334-2339 which brings me to bug 1460929.

Nathan, does that look right? Do we just want to expand that define so we don't _exit() in the profiling build, just like in coverage builds?

Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(nfroyd)

OK, I have a trypush that suggests the answer to the bottom of comment 15 is "yes", just need to tidy up patches...

Flags: needinfo?(nfroyd)
Attachment #9067066 - Attachment description: Bug 1196094 - use e10s when doing PGO profiling, r?froydnj → Bug 1196094 - use e10s when doing PGO profiling, r=froydnj
Depends on: 1555974
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/4b78f4fb473c
disable _exit in child processes when generating profile information, r=froydnj
https://hg.mozilla.org/integration/autoland/rev/93ae54d47ca4
use e10s when doing PGO profiling, r=froydnj
Status: REOPENED → RESOLVED
Closed: 3 months ago3 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla69
Depends on: 1557762
You need to log in before you can comment on or make changes to this bug.