Closed Bug 1533569 Opened 1 year ago Closed 7 months ago

freeze UA sheet shared memory and enable the feature

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla70
Fission Milestone M4
Tracking Status
firefox70 --- fixed

People

(Reporter: heycam, Assigned: heycam)

References

(Blocks 1 open bug)

Details

(Keywords: perf-alert)

Attachments

(3 files, 2 obsolete files)

Bug 1474793 will be landed with the pref disabled, since we need to wait until bug 1479960 (shared memory freezing) is available.

Depends on: 1479960
Depends on: 1474793

Now that bug 1479960 is fixed, can we turn this on? :)

Flags: needinfo?(cam)

Hi Jed, I notice in the comments for SharedMemory::Freeze that it will unmap the shared memory in the current process. Is this a fundamental design requirement? Currently, the shared memory UA style sheets benefit from having the same shared memory copy of the sheet data in the parent and child processes. I can easily change this to keep a unique copy in the parent process if needed. Or can I just map it again immediately in the parent process, and I'll have a frozen read only view of it?

Flags: needinfo?(cam) → needinfo?(jld)

(In reply to Cameron McCormack (:heycam) from comment #2)

Hi Jed, I notice in the comments for SharedMemory::Freeze that it will unmap the shared memory in the current process. Is this a fundamental design requirement? Currently, the shared memory UA style sheets benefit from having the same shared memory copy of the sheet data in the parent and child processes. I can easily change this to keep a unique copy in the parent process if needed. Or can I just map it again immediately in the parent process, and I'll have a frozen read only view of it?

Yes, you can map it again after it's frozen.

However, the address space could be claimed by another thread in the meantime. If that's a showstopper, it would be possible to avoid it, more or less: on Unix we can atomically replace the mapping with MAP_FIXED, but on Windows there doesn't seem to be an equivalent, so we'd just have to remove write permission and hope nobody accidentally re-adds it. (This shouldn't be a security problem because it's in a trusted process, but it seems worthwhile to prevent accidental misuse, and some of the backends will require not trying to write that mapping, and I'd like to keep things as consistent across platforms as possible.) And I'd have to write more code to implement all that. So, hopefully it's acceptable to fall back to copying / regenerating the data in rare cases.

Flags: needinfo?(jld)

Thanks, I think it's fine to fall back to using a unique copy in that case, and my guess is that remapping immediately is going to work most of the time, since we already chose an address some distance away from other allocations.

Fission Milestone: --- → M4
Assignee: nobody → cam
Status: NEW → ASSIGNED

(In reply to Cameron McCormack (:heycam) from comment #4)

Thanks, I think it's fine to fall back to using a unique copy in that case, and my guess is that remapping immediately is going to work most of the time, since we already chose an address some distance away from other allocations.

It would be good to have telemetry to confirm/monitor this. Should we have a separate bug for that?

Flags: needinfo?(cam)

Filed bug 1562060. (And also to measure the rate that we fail to map in the content process, too.)

Flags: needinfo?(cam)
See Also: → 1562060

There are some r+ patches which didn't land and no activity in this bug for 2 weeks.
:heycam, could you have a look please?
For more information, please visit auto_nag documentation.

Flags: needinfo?(cam)
Pushed by cmccormack@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/06da212d37f5
Part 1: Freeze UA sheet shared memory. r=jwatt
https://hg.mozilla.org/integration/autoland/rev/9c5be5540874
Part 2: Enable shared UA style sheets. r=jwatt
Regressions: 1574704

400 KiB is just too small for Android ARM64.

Attachment #9086915 - Attachment is obsolete: true
Attachment #9086916 - Attachment is obsolete: true
Pushed by cmccormack@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/43c411ccf445
Part 1: Freeze UA sheet shared memory. r=jwatt
https://hg.mozilla.org/integration/autoland/rev/216edfe6b998
Part 2: Enable shared UA style sheets. r=jwatt
https://hg.mozilla.org/integration/autoland/rev/70e4542f1434
Part 3: Increase shared memory size. r=jwatt
Status: ASSIGNED → RESOLVED
Closed: 7 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla70
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla70 → ---

The failure mode of that test is just like that reported for the intermittents in bug 1574066, but perma-failing.

Pushed by cmccormack@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/39ea7a8fdbea
Part 1: Freeze UA sheet shared memory. r=jwatt
https://hg.mozilla.org/integration/autoland/rev/fcd5bfe0a6a7
Part 2: Enable shared UA style sheets. r=jwatt
https://hg.mozilla.org/integration/autoland/rev/4ef6bd2cf54b
Part 3: Increase shared memory size. r=jwatt
Flags: needinfo?(cam)
Status: REOPENED → RESOLVED
Closed: 7 months ago7 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla70

== Change summary for alert #22859 (as of Tue, 27 Aug 2019 05:32:05 GMT) ==

Improvements:

4% Base Content Heap Unclassified windows10-64-shippable opt 1,630,012.00 -> 1,559,872.00
4% Base Content Heap Unclassified windows10-64-shippable-qr opt 1,622,960.67 -> 1,555,689.33
4% Base Content Explicit linux64-shippable-qr opt 13,450,922.67 -> 12,949,333.33
4% Base Content Explicit linux64-shippable opt 13,406,720.00 -> 12,915,712.00
4% Base Content Explicit windows10-64-shippable opt 10,705,066.67 -> 10,321,920.00
4% Base Content Explicit windows10-64-shippable-qr opt 10,678,613.33 -> 10,297,856.00
3% Base Content Resident Unique Memory windows10-64-shippable opt 14,405,973.33 -> 13,959,168.00
3% Base Content Resident Unique Memory windows10-64-shippable-qr opt 14,263,978.67 -> 13,822,122.67
3% Base Content Resident Unique Memory windows10-64-shippable-qr opt 14,265,600.00 -> 13,827,413.33

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=22859

(In reply to Pulsebot from comment #19)

Pushed by cmccormack@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/43c411ccf445
Part 1: Freeze UA sheet shared memory. r=jwatt
https://hg.mozilla.org/integration/autoland/rev/216edfe6b998
Part 2: Enable shared UA style sheets. r=jwatt
https://hg.mozilla.org/integration/autoland/rev/70e4542f1434
Part 3: Increase shared memory size. r=jwatt

Those pushes caused the regression below:
== Change summary for alert #22840 (as of Sat, 24 Aug 2019 09:44:17 GMT) ==

Regressions:

19% raptor-tp6-google-firefox-cold fcp windows10-64-shippable-qr opt 310.38 -> 370.08
5% raptor-tp6-google-firefox-cold windows10-64-shippable-qr opt 315.56 -> 332.10

Improvements:

3% raptor-ares6-firefox windows10-64-shippable opt 59.49 -> 57.73

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=22840
(In reply to Pulsebot from comment #23)

Pushed by cmccormack@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/39ea7a8fdbea
Part 1: Freeze UA sheet shared memory. r=jwatt
https://hg.mozilla.org/integration/autoland/rev/fcd5bfe0a6a7
Part 2: Enable shared UA style sheets. r=jwatt
https://hg.mozilla.org/integration/autoland/rev/4ef6bd2cf54b
Part 3: Increase shared memory size. r=jwatt

and those pushes caused the improvements below (99% sure, waiting for the backfill to finish):
== Change summary for alert #22854 (as of Mon, 26 Aug 2019 20:52:59 GMT) ==

Improvements:

19% raptor-tp6-google-firefox-cold fcp windows10-64-shippable-qr opt 362.50 -> 293.17
19% raptor-tp6-microsoft-firefox loadtime macosx1014-64-shippable opt 580.50 -> 472.96
16% raptor-tp6-microsoft-firefox macosx1014-64-shippable opt 402.81 -> 336.64
16% raptor-tp6-imdb-firefox loadtime macosx1014-64-shippable opt 1,266.54 -> 1,069.29
16% raptor-tp6-reddit-firefox fcp macosx1014-64-shippable opt 421.12 -> 355.71
15% raptor-tp6-imdb-firefox macosx1014-64-shippable opt 672.39 -> 574.22
14% raptor-tp6-imdb-firefox fcp macosx1014-64-shippable opt 560.83 -> 482.88
12% raptor-tp6-amazon-firefox fcp macosx1014-64-shippable opt 513.73 -> 451.67
12% raptor-tp6-amazon-firefox macosx1014-64-shippable opt 623.84 -> 551.18
6% raptor-tp6-google-firefox-cold windows10-64-shippable-qr opt 330.72 -> 309.89

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=22854

OK, please needinfo me if you determine that there are still lingering regressions, thanks!

You need to log in before you can comment on or make changes to this bug.