Closed Bug 912321 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla27

People

(Reporter: jimb, Assigned: jimb)

Details

Attachments

(1 file, 1 obsolete file)

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.
Revised, with simple tests.
Assignee: general → jimb
Attachment #799220 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Comment 1 is private: false
Attachment #799768 - Attachment is private: false
Attachment #799768 - Flags: review?(bhackett1024)
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.
Attachment #799768 - Flags: review?(bhackett1024) → review+
(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.
Cancelled previous try push; folded in test from bug 912719, and did a fresh try push:
https://tbpl.mozilla.org/?tree=Try&rev=fea5f3d5fd46
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.
On a related note: it would be good for the mashup-fuzzer to have several unit tests for that use these functions.
(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>
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
Once again, with code to skip those tests in non-threadsafe builds:

https://tbpl.mozilla.org/?tree=Try&rev=6783df24af88
https://hg.mozilla.org/integration/mozilla-inbound/rev/b3c5a1f9193b
Flags: in-testsuite+
Target Milestone: --- → mozilla27
https://hg.mozilla.org/mozilla-central/rev/b3c5a1f9193b
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.