Closed Bug 916255 Opened 6 years ago Closed 6 years ago

Allow script loaders to have multiple in flight off thread parses

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla28

People

(Reporter: bhackett, Assigned: bhackett)

References

Details

(Whiteboard: [qa-])

Attachments

(2 files)

Attached patch patchSplinter Review
The current design only allows one off thread parse at a time per script loader, which is an unnecessary limitation and a result of how the patch in bug 906371 was written.  This patch relaxes this, and also doesn't unblock onload until after the off thread parsed script has executed, per bug 906371 comment 18.
Attachment #804625 - Flags: review?(bzbarsky)
Comment on attachment 804625 [details] [diff] [review]
patch

>+  // This reference will be consumed by OffThreadScriptLoaderCallback.

Need to NS_RELEASE if the CompileOffThread call fails, right?  Or alternately:

  nsRefPtr<NotifyOffThreadScriptLoadCompletedRunnable> runnable = 
    new NotifyOffThreadScriptLoadCompletedRunnable(aRequest, this);

  if (!JS::CompileOffThread(...)) {
    return NS_ERROR_OUT_OF_MEMORY;
  }

  (void)runnable.forget(); // Will get released by NotifyOffThreadScriptLoadCompletedRunnable

or something.

r=me either way with that dealt with.
Attachment #804625 - Flags: review?(bzbarsky) → review+
This also broke gaia and forced us to close our tree for two days. Please feel free to ask for help on dev-gaia for a test case or how to try this with gaia before trying to land this again.
Attached patch (rebased)Splinter Review
I rebased this patch to trunk (trivial) and re-pushed to try:
https://tbpl.mozilla.org/?tree=Try&rev=d9f9a5cf6497
https://tbpl.mozilla.org/?tree=Try&rev=c7fa75643f45
(The OSX bustage was the parent patch, hence the second push.)
Attachment #8343113 - Attachment description: multiple-parses → (rebased)
...and I passed "-t none" out of habit despite that being the failure in comment 3.  Let's see for real now:
https://tbpl.mozilla.org/?tree=Try&rev=ed4346c131da
Looks green!  zac on irc said that nowadays Gu on tbpl is a good smoketest for Gaia so it seems like the underlying bug has since been fixed.  Anyone objections to re-landing this now?
I say let's do it.
https://hg.mozilla.org/mozilla-central/rev/ec5f9193d178
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Assignee: general → bhackett1024
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.