Closed Bug 1809861 Opened 1 year ago Closed 1 year ago

Simplify and clean up off-main thread script compilation

Categories

(Core :: DOM: Core & HTML, task)

task

Tracking

()

RESOLVED FIXED
111 Branch
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.

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.

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

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

Backed out for causing bustages in ScriptLoader.cpp.

Flags: needinfo?(jcoppeard)
Flags: needinfo?(jcoppeard)
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

These patches shouldn't have changed script load timing at all so I don't think it's likely that this is related.

Would it make sense to uplift this into today's b3 build so we can get more data on its impact on stability?

Flags: needinfo?(jcoppeard)

(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.

Flags: needinfo?(jcoppeard)

(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)

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

Attachment

General

Created:
Updated:
Size: