912321 - JS shell should be able to synchronize on off-thread compilations

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jim Blandy :jimb]

Attachment: Define the 'finishOffThreadScript' function in the JavaScript shell, for testing off-thread compilation.

The fact that offThreadCompileScript just throws away the resulting script, and has no way to receive the result of the compilation, makes it difficult to test.

The attached patch defines a structure protected by a monitor to receive the results of compilation, and a JS shell function that waits for compilation to complete.

For now, this is just for your interested perusal; if there's something wrong with the way I've done it, please let me know.

In the near term, I want to use this to add some tests for Debugger's interactions with off-thread compilation; I'll place notes in this bug when those are complete, and add some (Debugger-independent) tests for the new function to this patch.

Comment 1

[Jim Blandy :jimb]

Attachment: Define the 'finishOffThreadScript' function in the JavaScript shell, for testing off-thread compilation.

Revised, with simple tests.

Comment 2

[Brian Hackett [Laid off!]]

Comment on attachment 799768 [details] [diff] [review]
Define the 'finishOffThreadScript' function in the JavaScript shell, for testing off-thread compilation.

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

::: js/src/shell/js.cpp
@@ +3313,5 @@
> +    State state;
> +    void *token;
> +    JSString *source;
> +
> +    void transition(State from, State to) {

This method doesn't seem to be called anywhere.

Comment 3

[Jim Blandy :jimb]

(In reply to Brian Hackett (:bhackett) from comment #2)
> > +    void transition(State from, State to) {
> 
> This method doesn't seem to be called anywhere.

Oops. Thanks.

Comment 4

[Jim Blandy :jimb]

https://tbpl.mozilla.org/?tree=Try&rev=97437170c289

Comment 5

[Jim Blandy :jimb]

Cancelled previous try push; folded in test from bug 912719, and did a fresh try push:
https://tbpl.mozilla.org/?tree=Try&rev=fea5f3d5fd46

Comment 6

[Luke Wagner [:luke]]

To make this fuzz-ready, I think you need to test JS::CanCompileOffThread() in OffMainThreadCompileScript so that we don't JS_ASSERT-crash when, e.g., --ion-parallel-compile=off or !defined(JS_ION) or there is an active GC in the atoms zone, etc.

Comment 7

[Luke Wagner [:luke]]

On a related note: it would be good for the mashup-fuzzer to have several unit tests for that use these functions.

Comment 8

[Jim Blandy :jimb]

(In reply to Luke Wagner [:luke] from comment #6)
> To make this fuzz-ready, I think you need to test JS::CanCompileOffThread()
> in OffMainThreadCompileScript so that we don't JS_ASSERT-crash when, e.g.,
> --ion-parallel-compile=off or !defined(JS_ION) or there is an active GC in
> the atoms zone, etc.

Okay --- I've added this check.

sergei:src$ obj~/js --thread-count=0
js> offThreadCompileScript
function offThreadCompileScript() {
    [native code]
}
js> offThreadCompileScript("1+2")
typein:2:0 Error: cannot compile code on worker thread
js>

Comment 9

[Jim Blandy :jimb]

New try (the old one wasn't pushed atop the actual fix, and, encouragingly, failed all over the place):

https://tbpl.mozilla.org/?tree=Try&rev=ad5863e42872

Comment 10

[Jim Blandy :jimb]

Once again, with code to skip those tests in non-threadsafe builds:

https://tbpl.mozilla.org/?tree=Try&rev=6783df24af88

Comment 11

[Jim Blandy :jimb]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b3c5a1f9193b

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b3c5a1f9193b