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•3 years 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•3 years 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•3 years 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
|
||
Comment 5•3 years 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•3 years ago
|
|
||
Comment 7•3 years 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•3 years 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•3 years 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•3 years 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•3 years 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•3 years 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
•