Closed Bug 1316078 Opened 3 years ago Closed 3 years ago

Decode XDR off-main-thread.


(Core :: JavaScript Engine, defect)

Not set



Tracking Status
firefox54 --- fixed


(Reporter: nbp, Assigned: nbp)




(5 files)

+++ This bug was initially created as a clone of Bug #1311020 +++

Currently the parser works out of the main thread.  Preliminary results from Bug 900784 suggests that Decoding can take half the time taken by the parser, which should be enough to miss frames when decoding some encoded scripts.

To improve over this state we should think of moving the decoding phase out of the main thread, as suggested in Bug 1311020 comment 1 and Bug 1311020 comment 2.
See Also: → 1311020
I am currently looking into making use of for ParseTask for decoding the bytecode cache.  To bridge the XDR decoder with the HelperThread ParseTask, I will have to replace the use of JSContext* by an ExclusiveContext* for all XDR functions.
This patch factor StartOffThreadParseScript and StartOffThreadParseModule to
a function which accepts a lambda function which is used to create the
specific parse-task which is needed.

This is needed to avoid duplicating this code in the part 4.
Attachment #8823303 - Flags: review?(bhackett1024)
As of part 4, we add the ability to call the codeScript function from the
helper thread, so we have to switch the to the other logger if we got an
exclusive context as input (after part 3).
Attachment #8823310 - Flags: review?(hv1989)
This is needed in order to use XDR decoder from the helper thread (part 4).

This patch makes XDRState independent of the JSContext, CompileOption, and
its SourceObject reference.

 - JSContext are transformed into ExclusiveContext.

   This has for consequences that we have to replace JSContext exceptions by
   TranscodeResult returned value, and that OOM have to use
   ReportOutOfMemory calls instead.

   The lifoAlloc can no longer be provided exclusively by the JSContext and
   we should expect one as argument if we use an ExclusiveContext, as
   already provided by the HelperThread.

 - CompileOption are now given as argument.

   One of the weirdness that I noticed while adding this, is that XDR has
   always ignored the CompileOption, always considering the encoded content
   as the source of thruth.  This is no longer true for random script
   provided by multiple documents, with different versions for example.

   This is mandatory for moving XDR decoding out of the main thread.

 - The source object receiver is pre-allocated, in order to be traced while
   running under the HelperThread.
Attachment #8823339 - Flags: review?(luke)
This patch add new jsapi functions to be used in Bug 900784.

This reuse the same mechanism as the off-main-thread parsing which is
already in place.  Part 3, changed the XDR decoding to be working fine with
the helper thread constraints.
Attachment #8823341 - Flags: review?(bhackett1024)
This bug needs additional tests, for testing the off-thread parts.  The on-main-thread part should already be tested by the jit-tests/tests/xdr tests which are already part of the test suite.

I will add JS shell tests before landing these patches, at least to get a better fuzzing interface.

My first goal at the moment is to get this integrated in Bug 900784.
Attachment #8823303 - Flags: review?(bhackett1024) → review+
Attachment #8823341 - Flags: review?(bhackett1024) → review+
Comment on attachment 8823339 [details] [diff] [review]
part 3 - Use an ExclusiveContext instead of a JSContext in XDR functions.

Review of attachment 8823339 [details] [diff] [review]:

::: js/src/jsapi.h
@@ +5894,3 @@
> +    // An error, the JSContext has a pending exception, or the ExclusiveContext
> +    // has a pending exception.

ExclusiveContext is not a public jsapi detail, so about just: There is a pending exception.

::: js/src/jsfun.cpp
@@ -551,5 @@
> -        if (!fun->isInterpreted()) {
> -            JSAutoByteString funNameBytes;
> -            if (const char* name = GetFunctionNameBytes(cx, fun, &funNameBytes)) {
> -                JS_ReportErrorNumberLatin1(cx, GetErrorMessage, nullptr,
> -                                           JSMSG_NOT_SCRIPTED_FUNCTION, name);

I think this code can be removed from js.msg now.
Attachment #8823339 - Flags: review?(luke) → review+
Attachment #8823310 - Flags: review?(hv1989) → review+
Assignee: nobody → nicolas.b.pierron
This patch copy the logic of OffThreadCompileScript and runOffThreadScript
to handle bytecode provided in a CacheEntry object which contains some

Basically a minimal test case with the new API would look like:

  var code = CacheEntry(`"some code"`);
  evaluate(code, { saveIncrementalBytecode: true });

Where the evaluate function is used to generate the bytecode, which is then
given as argument to the offThreadDecodeScript function.
Attachment #8826675 - Flags: review?(bhackett1024)
Comment on attachment 8826675 [details] [diff] [review]
part 5 - Add XDR off-thread decoder test cases.

Review of attachment 8826675 [details] [diff] [review]:

Sorry for the delay.
Attachment #8826675 - Flags: review?(bhackett1024) → review+
Pushed by
part 1 - Extract redudant code into StartOffThreadParseTask. r=bhackett
part 2 - Make XDR traceLogger work on a different thread. r=h4writer
part 3 - Use an ExclusiveContext instead of a JSContext in XDR functions. r=luke
part 4 - Add a script decoder as a valid off-main-thread parse-task. r=bhackett
part 5 - Add XDR off-thread decoder test cases. r=bhackett
Depends on: 1351357
Depends on: 1358521
Depends on: 1427860
You need to log in before you can comment on or make changes to this bug.