Closed
Bug 1515201
Opened 6 years ago
Closed 6 years ago
Avoid needlessly hosting about:blank content in the privileged content process
Categories
(Testing :: AWSY, enhancement)
Tracking
(firefox66 fixed)
RESOLVED
FIXED
mozilla66
Tracking | Status | |
---|---|---|
firefox66 | --- | fixed |
People
(Reporter: mconley, Assigned: mconley)
Details
Attachments
(1 file)
Bug 1515201 - Avoid loading about:blank in the privileged content process during AWSY tests. r?erahm
47 bytes,
text/x-phabricator-request
|
Details | Review |
When the privileged content process is enabled, any about:home / about:newtab pages that navigate to about:blank remain in the privileged content process. It's only when they navigate to a non about:blank web address that a process flip to a content process will occur.
The test_base_memory_usage.py AWSY test loads a series of tabs, one for each content process, and sends each to about:blank, in order to get a "base" measurement, where each content process is running, but each also only have an about:blank top-level tab loaded in each. However, because of how AWSY opens tabs (via the new tab button), what we end up with is a bunch of tabs all hosted in the same privileged content process, because of the issue described in the first paragraph.
I tried to address this in bug 1505185 by making it so that an about:blank load triggered by the parent would transition the privileged content process to a normal web content process, but that broke a bunch of stuff. I started trying to fix it, but ended up going down a pretty deep rabbit hole - about:blank is kinda special, and we'll load it in various places for various reasons, and we kinda expect it to keep a tab in the process that was assigned to it.
So I think the simplest solution to make this AWSY test measure what we want is to have it open tabs differently - we should use gBrowser.loadOneTab directly instead of clicking the newtab button, so we bypass loading about:newtab altogether.
Assignee | ||
Comment 1•6 years ago
|
||
Assignee | ||
Comment 2•6 years ago
|
||
Assignee | ||
Comment 3•6 years ago
|
||
Thanks for the review! Sorry about the formatting - I think my Linux VM's vim has a strange concept of whitespacing.
From my read on the try push, this is going to cause a "regression" in a lot of our base content measurements. I suspect this is because we're actually going to be initializing and loading about:blank into several content processes rather than the single privileged content process.
Is that okay, erahm? Do the rest of the numbers seem reasonable to you?
Flags: needinfo?(erahm)
Comment 4•6 years ago
|
||
(In reply to Mike Conley (:mconley) (:⚙️) (Away from Bugmail Dec 24 - Jan 7) from comment #3)
> From my read on the try push, this is going to cause a "regression" in a lot
> of our base content measurements. I suspect this is because we're actually
> going to be initializing and loading about:blank into several content
> processes rather than the single privileged content process.
I think this is most likely actually because we take the median of all process values, and with only one regular content process, that means we average its values with the values for the pre-allocated content process (see bug 1516493), which should in general use significantly less memory than other processes.
> Is that okay, erahm? Do the rest of the numbers seem reasonable to you?
"regression" is OK. It's basically just us fixing our measurements to measure the right things.
Flags: needinfo?(erahm)
Comment 5•6 years ago
|
||
(In reply to Mike Conley (:mconley) (:⚙️) (Away from Bugmail Dec 24 - Jan 7) from comment #3)
> Thanks for the review! Sorry about the formatting - I think my Linux VM's
> vim has a strange concept of whitespacing.
This is because you indented with tabs, and Phabricator has the false and heretical idea that tab stops are 2 spaces. Which, while that may be arguable in some weird circumstances, does not hold any weight when viewing a Python script, where whitespace is significant, and the interpreter parses them as 8 spaces.
Either way, please expand the tabs. And ideally add an appropriate modeline to that file.
Assignee | ||
Comment 6•6 years ago
|
||
(In reply to Kris Maglione [:kmag] from comment #5)
>
> Either way, please expand the tabs. And ideally add an appropriate modeline
> to that file.
Done. I cargo-culted a modeline from a relatively recent Python file.
Pushed by mconley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/653b89c61e49
Avoid loading about:blank in the privileged content process during AWSY tests. r=erahm
Comment 8•6 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox66:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
Comment 9•6 years ago
|
||
Looks like an 800K drop in JS memory usage and a corresponding drop in resident unique.
Comment 10•6 years ago
|
||
Here are the new AWSY baselines:
== Change summary for alert #18620 (as of Mon, 07 Jan 2019 14:07:45 GMT) ==
Improvements:
14% Base Content JS osx-10-10 opt stylo 5,810,292.00 -> 4,999,340.00
14% Base Content JS windows10-64-qr opt stylo 5,864,117.33 -> 5,059,784.00
6% Base Content Explicit windows10-64-qr opt stylo 12,120,405.33 -> 11,390,464.00
3% Base Content Resident Unique Memory windows10-64-qr opt stylo 14,872,917.33 -> 14,484,650.67
For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=18620
Updated•6 years ago
|
Assignee: nobody → mconley
You need to log in
before you can comment on or make changes to this bug.
Description
•