1222086 - Block JavaScript parser thread when source code characters are not available

Status: NEW

(Core :: JavaScript Engine, defect)

Description

[Yury Delendik (:yury)]

So the idea is to block parsing thread when end of available source code stream is found. Currently the Parser logic depends on a JavaScript source code to be a continuous char16 buffer -- we either need to replace the initial buffer or pre-allocate enough characters to fit at the beginning. Problem with the latter is that nsIChannel cannot guarantee or does not know total length (hints like Content-Length do not work since they are reflect the encoded content length -- zipped or charset).

Pros:
- Minimal change in the Parser code base
- Practically no impact on the parsing speed
- No intermediate state to save 

Cons:
- Available single Parser thread will be blocked
- Hence more than one Parsing thread must be created (bad due to thread memory overheads) and use additional heuristic needed to detect which files will be processed using this mode

Comment 1

[Yury Delendik (:yury)]

Attachment: [WIP] 1. Pass JS source as CharsStream object.

Comment 2

[Yury Delendik (:yury)]

Attachment: [WIP] 2. Allows OffThreadCompileCallback handle chars in chunks

Comment 3

[Yury Delendik (:yury)]

Attachment: [WIP] 3. jsshell integration

Comment 4

[Yury Delendik (:yury)]

Attachment: jsshell code example

Comment 5

[Yury Delendik (:yury)]

Attachment: [WIP] 1. Pass JS source as CharsStream object.

Comment 6

[Yury Delendik (:yury)]

Attachment: [WIP] 2. Allows OffThreadCompileCallback handle chars in chunks

Comment 7

[Yury Delendik (:yury)]

Attachment: [WIP] 3. jsshell integration

Comment 8

[Yury Delendik (:yury)]

Attachment: 1. Pass JS source as CharsStream object.

Comment 9

[Yury Delendik (:yury)]

Attachment: 2. Allows OffThreadCompileCallback handle chars in chunks

Comment 10

[Yury Delendik (:yury)]

Attachment: 3. jsshell integration

Comment 11

[Yury Delendik (:yury)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1033608147f9

I run Parsemark to see if there is a side effect from the TokenStream redesign -- there was no regression, "Meh." on all files.

Comment 12

[Kannan Vijayan [:djvj]]

Ah!  Just saw these review requests.  Will do them next week.  Sorry for lateness.

Comment 13

[Kannan Vijayan [:djvj]]

Comment on attachment 8689048 [details] [diff] [review]
1. Pass JS source as CharsStream object.

Review of attachment 8689048 [details] [diff] [review]:
-----------------------------------------------------------------

Nice.  Very clean and easy to understand.

::: js/src/builtin/RegExp.cpp
@@ +172,5 @@
>      }
>  
>      /* Steps 9-10. */
>      CompileOptions options(cx);
> +    frontend::TokenStream dummyTokenStream(cx, options, &frontend::NullCharsStream::null(), nullptr);

It seems slightly more readable here to have the NullCharsStream usage reflect the same syntactic structure as the usage of StaticCharsStream: namely, it gets stack-allocated and a pointer gets passed to the stack-allocated item.

::: js/src/frontend/TokenStream.h
@@ +309,5 @@
> +        *start = *end = nullptr;
> +        *final = true;
> +    }
> +
> +    static NullCharsStream& null() {

I'd remove this method and use the same calling convention for NullCharsStream as users of StaticCharsStream do: stack-allocate-and-pass-pointer, instead of having a single statically allocated instance.

Minor nit, though.  I don't feel strongly about it.

@@ +926,5 @@
> +            if (MOZ_LIKELY(ptr < limit_))
> +                return true;
> +            if (final_)
> +                return false;
> +            requestMoreChars();

|requestMoreChars| is guaranteed to grow the buffer, right?  Might be good to assert that after the call, just to make sure that implementors of |CharsStream| are fulfilling that contract.

Comment 14

[Kannan Vijayan [:djvj]]

Comment on attachment 8689048 [details] [diff] [review]
1. Pass JS source as CharsStream object.

Review of attachment 8689048 [details] [diff] [review]:
-----------------------------------------------------------------

One additional note: The |final| parameter/field might better be named something like |streamComplete|.

Comment 15

[Kannan Vijayan [:djvj]]

Comment on attachment 8689049 [details] [diff] [review]
2. Allows OffThreadCompileCallback handle chars in chunks

Review of attachment 8689049 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jsapi.h
@@ +746,5 @@
>          length_(dataLength),
> +        requestChars_(requestChars),
> +        requestCharsData_(requestCharsData),
> +        ownsChars_(ownership == GiveOwnership),
> +        final_(!requestChars)

Nit: |final_| might be better named |streamComplete_|.

@@ +796,5 @@
>          ownsChars_ = false;
>          return const_cast<char16_t*>(data_);
>      }
>  
> +    bool final() const {

see above comment about naming (streamComplete()).

Comment 16

[Kannan Vijayan [:djvj]]

Comment on attachment 8689052 [details] [diff] [review]
3. jsshell integration

Review of attachment 8689052 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit-test/tests/basic/offThreadCompileScript-03.js
@@ +12,5 @@
> +
> +if (helperThreadCount() === 0)
> +  quit(0);
> +
> +offThreadCompileScript('Math.sin(M', {}, false);

This test case would be clearer if the entire "subscript" being compiled was a single string, and there was an array that listed the indexes at which the script should be split up into pieces, and the repeated calls to |appendOffThreadScript| and |waitSome| were instead a loop over that index array, using substring() to pick off pieces of the script to submit in parts.

::: js/src/shell/js.cpp
@@ +5090,5 @@
>  "  Wait for off-thread compilation to complete. If an error occurred,\n"
>  "  throw the appropriate exception; otherwise, run the script and return\n"
>  "  its value."),
>  
> +JS_FN_HELP("appendOffThreadScript", appendOffThreadScript, 0, 0,

Nice.  This is very clever.

Comment 17

[Yury Delendik (:yury)]

Attachment: Part 1. Pass JS source as CharsStream object

Comment 18

[Yury Delendik (:yury)]

(In reply to Kannan Vijayan [:djvj] from comment #14)
> Comment on attachment 8689048 [details] [diff] [review]
> 1. Pass JS source as CharsStream object.
> 
> Review of attachment 8689048 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> One additional note: The |final| parameter/field might better be named
> something like |streamComplete|.

Fixed.

(In reply to Kannan Vijayan [:djvj] from comment #13)
> Comment on attachment 8689048 [details] [diff] [review]
> 1. Pass JS source as CharsStream object.
> 
> Review of attachment 8689048 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Nice.  Very clean and easy to understand.
> 
> ::: js/src/builtin/RegExp.cpp
> @@ +172,5 @@
> >      }
> >  
> >      /* Steps 9-10. */
> >      CompileOptions options(cx);
> > +    frontend::TokenStream dummyTokenStream(cx, options, &frontend::NullCharsStream::null(), nullptr);
> 
> It seems slightly more readable here to have the NullCharsStream usage
> reflect the same syntactic structure as the usage of StaticCharsStream:
> namely, it gets stack-allocated and a pointer gets passed to the
> stack-allocated item.
> 
> ::: js/src/frontend/TokenStream.h
> @@ +309,5 @@
> > +        *start = *end = nullptr;
> > +        *final = true;
> > +    }
> > +
> > +    static NullCharsStream& null() {
> 
> I'd remove this method and use the same calling convention for
> NullCharsStream as users of StaticCharsStream do:
> stack-allocate-and-pass-pointer, instead of having a single statically
> allocated instance.
> 
> Minor nit, though.  I don't feel strongly about it.

Fixed.

> 
> @@ +926,5 @@
> > +            if (MOZ_LIKELY(ptr < limit_))
> > +                return true;
> > +            if (final_)
> > +                return false;
> > +            requestMoreChars();
> 
> |requestMoreChars| is guaranteed to grow the buffer, right?  Might be good
> to assert that after the call, just to make sure that implementors of
> |CharsStream| are fulfilling that contract.

requestMoreChars is non-virtual, so it's safe to add this check at the requestMoreChars method body.
And the described check is already present there. Is it what you had in mind?

Comment 19

[Yury Delendik (:yury)]

Attachment: Part 2. Allows OffThreadCompileCallback handle chars in chunks

(In reply to Kannan Vijayan [:djvj] from comment #15)
> Comment on attachment 8689049 [details] [diff] [review]
> 2. Allows OffThreadCompileCallback handle chars in chunks
> 
> Review of attachment 8689049 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jsapi.h
> @@ +746,5 @@
> >          length_(dataLength),
> > +        requestChars_(requestChars),
> > +        requestCharsData_(requestCharsData),
> > +        ownsChars_(ownership == GiveOwnership),
> > +        final_(!requestChars)
> 
> Nit: |final_| might be better named |streamComplete_|.
> 
> @@ +796,5 @@
> >          ownsChars_ = false;
> >          return const_cast<char16_t*>(data_);
> >      }
> >  
> > +    bool final() const {
> 
> see above comment about naming (streamComplete()).

Fixed.

Comment 20

[Yury Delendik (:yury)]

Attachment: Part 3. jsshell integration

(In reply to Kannan Vijayan [:djvj] from comment #16)
> Comment on attachment 8689052 [details] [diff] [review]
> 3. jsshell integration
> 
> Review of attachment 8689052 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jit-test/tests/basic/offThreadCompileScript-03.js
> @@ +12,5 @@
> > +
> > +if (helperThreadCount() === 0)
> > +  quit(0);
> > +
> > +offThreadCompileScript('Math.sin(M', {}, false);
> 
> This test case would be clearer if the entire "subscript" being compiled was
> a single string, and there was an array that listed the indexes at which the
> script should be split up into pieces, and the repeated calls to
> |appendOffThreadScript| and |waitSome| were instead a loop over that index
> array, using substring() to pick off pieces of the script to submit in parts.
>

Fixed.

Last try https://treeherder.mozilla.org/#/jobs?repo=try&revision=4228eb759fbd

Comment 21

[Yury Delendik (:yury)]

Attachment: Part 2. Allows OffThreadCompileCallback handle chars in chunks

Fixed "js/src/frontend/BytecodeCompiler.cpp:49:5: error: bad implicit conversion constructor for 'SourceBufferCharsStream'"

(https://treeherder.mozilla.org/#/jobs?repo=try&revision=9d2607159a52)

Comment 22

[Yury Delendik (:yury)]

Attachment: Part 1. Pass JS source as CharsStream object

The hazards test was failing (https://treeherder.mozilla.org/#/jobs?repo=try&revision=4228eb759fbd&selectedJob=14272132):

Function '_ZN2js8frontend6ParserINS0_16FullParseHandlerEEC1EPNS_16ExclusiveContextEPNS_9LifoAllocERKN2JS22ReadOnlyCompileOptionsEPNS0_11CharsStreamEbPNS1_INS0_18SyntaxParseHandlerEEEPNS_10LazyScriptE *INTERNAL* |js::frontend::Parser<ParseHandler>::Parser(js::ExclusiveContext*, js::LifoAlloc*, const JS::ReadOnlyCompileOptions&, js::frontend::CharsStream*, bool, js::frontend::Parser<js::frontend::SyntaxParseHandler>*, js::LazyScript*) [with ParseHandler = js::frontend::FullParseHandler]' has unrooted 'lazyOuterFunction' of type 'js::LazyScript*' live across GC call '_ZN2js8frontend11TokenStreamC1EPNS_16ExclusiveContextERKN2JS22ReadOnlyCompileOptionsEPNS0_11CharsStreamEPNS0_16StrictModeGetterE|void js::frontend::TokenStream::TokenStream(js::ExclusiveContext*, JS::ReadOnlyCompileOptions*, js::frontend::CharsStream*, js::frontend::StrictModeGetter*)' at js/src/frontend/Parser.cpp:662
    ...
    js/src/frontend/Parser.cpp:662: Call(6,7, this*.tokenStream.TokenStream(cx*,options*,chars*,__temp_1*.field:0)) [[GC call]]
    ....
    js/src/frontend/Parser.cpp:662: Call(22,23, this*.handler.FullParseHandler(cx*,alloc*,this*.tokenStream,syntaxParser*,lazyOuterFunction*))

I changed the lazyOuterFunction type from LazyScript* to Handle<LazyScript*> for the ParseHandler

(Please also find requestMoreChars body for contract verification of CharsStream.request implementation)

Comment 23

[Yury Delendik (:yury)]

Attachment: Part 2. Allows OffThreadCompileCallback handle chars in chunks

rebased

Comment 24

[Yury Delendik (:yury)]

Attachment: Part 3. jsshell integration

rebased, new try https://treeherder.mozilla.org/#/jobs?repo=try&revision=39643287908a

Comment 25

[Yury Delendik (:yury)]

Attachment: Part 2. Allows OffThreadCompileCallback handle chars in chunks

Found an issue with lazy function parsing: ScriptSource::setSourceCopy was getting initial/incomplete portion of the source and it was never updated. Fixing that by calling setSourceCopy after entire source is loaded.

Comment 26

[Yury Delendik (:yury)]

Attachment: Part 3. jsshell integration

Updated/added a test cases for the finding above.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a4f9bb47eb0c

Comment 27

[Yury Delendik (:yury)]

Attachment: 1-bug1222086-charsstream.patch

Found yesterday that the similar lazy function parsing issue happens when TokenStream's seek/tell are used. The TokenBuf.setOffsetOfNextRawChar (and requestMoreChars) is modified to sync chars buffer.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4b98db5f463d

Comment 28

[Kannan Vijayan [:djvj]]

Comment on attachment 8702607 [details] [diff] [review]
1-bug1222086-charsstream.patch

Review of attachment 8702607 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/frontend/BytecodeCompiler.cpp
@@ +233,2 @@
>                               /* foldConstants = */ false, (Parser<SyntaxParseHandler>*) nullptr,
> +                             Handle<LazyScript*>(nullptr));

Just a question.  There are many places where you convert lazyscript from being a raw pointer to rooted behaviour (using Rooted and Handle<LazyScript*>).  Why is this necessary now when it wasn't before?