Simplify and clean up off-main thread script compilation
Categories
(Core :: DOM: Core & HTML, task)
Tracking
()
Tracking | Status | |
---|---|---|
firefox111 | --- | fixed |
People
(Reporter: jonco, Assigned: jonco)
References
Details
Attachments
(3 files)
ScriptLoader's code for off-thread compilation is complex, and while I can't point to any actual bugs it could be simplified and improved to make it more obviously correct.
Assignee | ||
Comment 1•1 year ago
|
||
The main change in this patch is to prevent access to main-thread objects
while off-thread. This is done by using nsMainThreadPtrHandle to wrap main
thread pointers in the runnable. This prevents access to their targets when
off thread and ensure they are only released on the main thread.
This means that mRunnable is now only accessed on the main thread and so it
doesn't need to be atomic and can be a normal RefPtr. We also don't need to
leak a reference to it in AttemptOffThreadScriptCompile.
This also requires that timing data is moved to the runnable.
Cancellation should always have happened by unlink or destruction of
ScriptLoadContext so handling for that is removed.
Assignee | ||
Comment 2•1 year ago
|
||
This renames the following in an attempt to give consistent names to off-thread
compliation related things:
- AttemptAsyncScriptCompile -> AttemptOffThreadScriptCompile
- NotifyOffThreadScriptLoadCompletedRunnable -> OffThreadJobCompleteRunnable
- OffThreadScriptLoaderCallback -> OffThreadJobCompleteCallback
Depends on D166667
Assignee | ||
Comment 3•1 year ago
|
||
This just moves the method definitions so that they appear in roughly in the
order they are executed, to make reading and understanding this easier.
Depends on D166668
Pushed by jcoppeard@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/15b4b4f318a7 Part 1: Simplify and clean up off-main thread script compilation r=smaug https://hg.mozilla.org/integration/autoland/rev/fa4190d9f97e Part 2: Rename off-thread compilation methods to improve consistency r=smaug https://hg.mozilla.org/integration/autoland/rev/d8b7cb85d87f Part 3: Reorder off-thread compilation methods r=smaug
Comment 5•1 year ago
|
||
Backed out for causing bustages in ScriptLoader.cpp.
- Backout link
- Push with failures
- Failure Log
- Failure line: /builds/worker/checkouts/gecko/dom/script/ScriptLoader.cpp:1462:64: error: no matching function for call to 'nsMainThreadPtrHolder()'
Assignee | ||
Updated•1 year ago
|
Pushed by jcoppeard@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/2c008799d503 Part 1: Simplify and clean up off-main thread script compilation r=smaug https://hg.mozilla.org/integration/autoland/rev/065993b95f2a Part 2: Rename off-thread compilation methods to improve consistency r=smaug https://hg.mozilla.org/integration/autoland/rev/81ee5a55bc47 Part 3: Reorder off-thread compilation methods r=smaug
Comment 7•1 year ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/2c008799d503
https://hg.mozilla.org/mozilla-central/rev/065993b95f2a
https://hg.mozilla.org/mozilla-central/rev/81ee5a55bc47
Comment 8•1 year ago
•
|
||
Did this improve many Jetstream2 and Speedometer2 benchmarks on AWFY?
E.g. AWFY-Jetstream2 - prepack-wtb-First etc. (https://treeherder.mozilla.org/perfherder/graphs?highlightAlerts=1&highlightChangelogData=1&highlightCommonAlerts=0&series=mozilla-central,3738062,1,13&series=mozilla-central,3738062,1,13&series=autoland,3911449,1,13&timerange=5184000&zoom=1673728708017,1674125353979,14.078280209522609,24.06757496624014)
Assignee | ||
Comment 9•1 year ago
|
||
These patches shouldn't have changed script load timing at all so I don't think it's likely that this is related.
Comment 10•1 year ago
|
||
Would it make sense to uplift this into today's b3 build so we can get more data on its impact on stability?
Assignee | ||
Comment 11•1 year ago
|
||
(In reply to Ryan VanderMeulen [:RyanVM] from comment #10)
This is quite a complex change and I'm a bit wary about uplifting this with barely 24 hours on central. It would definitely be a medium/high risk at the present time.
Comment 12•1 year ago
•
|
||
(In reply to Jon Coppeard (:jonco) from comment #9)
These patches shouldn't have changed script load timing at all so I don't think it's likely that this is related.
https://bugzilla.mozilla.org/show_bug.cgi?id=1799024#c7 seems to suggest that this bug improved quite a few Jetstream2 tests on AWFY (basically any test with wtb in its name got improved)
Description
•