1316078 - Decode XDR off-main-thread.

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicolas B. Pierron [:nbp]]

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

Comment 1

[Nicolas B. Pierron [:nbp]]

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.

Comment 2

[Nicolas B. Pierron [:nbp]]

Attachment: part 1 - Extract redudant code into StartOffThreadParseTask.

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.

Comment 3

[Nicolas B. Pierron [:nbp]]

Attachment: part 2 - Make XDR traceLogger work on a different thread.

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

Comment 4

[Nicolas B. Pierron [:nbp]]

Attachment: part 3 - Use an ExclusiveContext instead of a JSContext in XDR functions.

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.

Comment 5

[Nicolas B. Pierron [:nbp]]

Attachment: part 4 - Add a script decoder as a valid off-main-thread parse-task.

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.

Comment 6

[Nicolas B. Pierron [:nbp]]

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.

Comment 7

[Luke Wagner [:luke]]

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.

Comment 8

[Nicolas B. Pierron [:nbp]]

Attachment: part 5 - Add XDR off-thread decoder test cases.

This patch copy the logic of OffThreadCompileScript and runOffThreadScript
to handle bytecode provided in a CacheEntry object which contains some
bytecode.

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

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

Where the evaluate function is used to generate the bytecode, which is then
given as argument to the offThreadDecodeScript function.

Comment 9

[Brian Hackett [Laid off!]]

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.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6ad62d0a98d7
part 1 - Extract redudant code into StartOffThreadParseTask. r=bhackett
https://hg.mozilla.org/integration/mozilla-inbound/rev/1c3dd1e35823
part 2 - Make XDR traceLogger work on a different thread. r=h4writer
https://hg.mozilla.org/integration/mozilla-inbound/rev/37077abfae06
part 3 - Use an ExclusiveContext instead of a JSContext in XDR functions. r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/2793ad041967
part 4 - Add a script decoder as a valid off-main-thread parse-task. r=bhackett
https://hg.mozilla.org/integration/mozilla-inbound/rev/36aebe4f719d
part 5 - Add XDR off-thread decoder test cases. r=bhackett

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6ad62d0a98d7
https://hg.mozilla.org/mozilla-central/rev/1c3dd1e35823
https://hg.mozilla.org/mozilla-central/rev/37077abfae06
https://hg.mozilla.org/mozilla-central/rev/2793ad041967
https://hg.mozilla.org/mozilla-central/rev/36aebe4f719d