Bug 169187 – WebAssembly: Make WebAssembly.instantiate/compile truly asynchronous

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Saam Barati]

...

[Keith Miller]

Created attachment 305148 [details]
WIP

[Keith Miller]

Created attachment 305199 [details]
WIP

[Keith Miller]

Created attachment 305266 [details]
Builds

[Keith Miller]

Created attachment 305377 [details]
works but needs tests

[Saam Barati]

Comment on attachment 305377 [details]
works but needs tests

View in context: https://bugs.webkit.org/attachment.cgi?id=305377&action=review

LGTM so far. Some drive by comments. I'll review more when it applies to ToT and has a changelog and tests.

> Source/JavaScriptCore/runtime/PromiseDeferredTimer.cpp:83
> +void PromiseDeferredTimer::addPendingPromise(JSPromiseDeferred* ticket, Vector<Strong<JSCell>>&& dependancies)

typo: "dependancies" => "dependencies"

> Source/JavaScriptCore/runtime/PromiseDeferredTimer.cpp:130
> +    ASSERT(m_vm->currentThreadIsHoldingAPILock());
> +    auto result = m_blockedTasks.add(blockingTicket, Vector<Task>());
> +    result.iterator->value.append(WTFMove(task));

What's the behavior here if m_tasks doesn't have a task w/ blocking ticket? Maybe worth a debug assertion?

> Source/JavaScriptCore/runtime/PromiseDeferredTimer.h:77
> +
> +

Style: Please remove extra whitespace.

> Source/JavaScriptCore/wasm/JSWebAssembly.cpp:124
> +    // In order to avoid potentially recompiling a module. We first allocate its gather all the import/memory information prior to
> +    // generating any code. If we didn't do this we might choose to compile with Signaling memory but actually get a Bonuds Checking
> +    // memory then have to recompile.

Please rewrite this paragraph. It has some typos and missing words.

> Source/JavaScriptCore/wasm/JSWebAssembly.cpp:141
> +    Vector<Strong<JSCell>> dependancies;

typo

> Source/JavaScriptCore/wasm/JSWebAssembly.cpp:147
> +        vm.promiseDeferredTimer->scheduleBlockedTask(instance->codeBlock()->promise(), [&vm, promise, instance, module, entries] () {

Is the promise parameter different than instance->codeBlock()->promise?

> Source/JavaScriptCore/wasm/JSWebAssembly.cpp:161
> +    // FIXME: This reparses the module header, which shouldn't be necessary.

Please fie a bug and link to it here.

> Source/JavaScriptCore/wasm/WasmPlan.cpp:244
> +            dataLogLn("Getting the lock: fail");

please remove

> Source/JavaScriptCore/wasm/WasmWorklist.cpp:51
> +    typedef AutomaticThread Base;

Style: I think we're starting to use "using" for this type of thing

> Source/JavaScriptCore/wasm/WasmWorklist.cpp:120
> +        Plan* plan = element.plan.get();
> +        ASSERT(plan);
> +        if (!plan->hasBeenPrepared()) {
> +            plan->parseAndValidateModule();
> +            if (!plan->hasWork())
> +                return complete();
> +            
> +            plan->prepare();
> +
> +            LockHolder locker(*worklist.m_lock);
> +            element.nextPriority();
> +            worklist.m_queue.push(element);
> +            worklist.m_planEnqueued->notifyAll(locker);
> +        }
> +
> +        // FIXME: this should check in at some occasionally to see if there are new, higher priority, plans that need to be run.
> +        plan->compileFunctions();
> +        ASSERT(!plan->hasWork());
> +        attemptToRemoveCompilationPlan(holdLock(*worklist.m_lock));
> +        return complete();

If a plan can ever be enqueued twice, the above`if (!plan->hasBeenPrepared()) { ... }` is racy. If it can't be, perhaps you can add an assertion to enqueue(...) to make this explicit.

> Source/JavaScriptCore/wasm/WasmWorklist.cpp:155
> +void Worklist::enqueue(JSPromiseDeferred* promise, RefPtr<Plan> plan)

Can a Plan ever be enqueued twice?

> Source/JavaScriptCore/wasm/WasmWorklist.cpp:175
> +        // FIXME: we should have our own PriorityQueue that doesn't suck.

Please open a bug for this. I agree

> Source/JavaScriptCore/wasm/WasmWorklist.cpp:187
> +            elements.push_back(m_queue.top());

What are the semantics of this?

> Source/JavaScriptCore/wasm/WasmWorklist.cpp:203
> +    unsigned numberOfCompilationThreads = Options::useConcurrentJIT() ? WTF::numberOfProcessorCores() - 1 : 1; // Save a core for the main thread.

This mostly seems good, but it'll be unfortunate if the main thread isn't doing anything, and we could be using an extra core. Can we just have one thread that has super lower priority?

> Source/JavaScriptCore/wasm/WasmWorklist.cpp:233
> +
> +

Style: remove whitespace.

> Source/JavaScriptCore/wasm/WasmWorklist.h:51
> +    JS_EXPORT_PRIVATE void enqueue(JSPromiseDeferred*, RefPtr<Plan>);

Why not Ref<Plan>?

> Source/JavaScriptCore/wasm/WasmWorklist.h:95
> +    // Technically, this could overflow but that's unlikely. Even if it did, we will just compile things of the same
> +    // Priority it the wrong order, which isn't wrong, just suboptimal.

Why not just use uint64_t and make this even more effectively impossible?

> JSTests/wasm/js-api/web-assembly-instantiate.js:1
> +asyncTest(7);

This is the wrong API for something like this. It's going to be really crappy to have to manually count the number of tests. We will definitely get this wrong in the future. Instead, lets make an API like this:

runAsyncTest(function(pass) {
   // do some async work.
   // call pass when done
});
and the test fails if an exception is thrown from that function.

[Keith Miller]

Created attachment 305448 [details]
Patch

[Build Bot]

Attachment 305448 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/wasm/JSWebAssembly.cpp:176:  Extra space before )  [whitespace/parens] [2]
ERROR: Source/JavaScriptCore/wasm/js/JSWebAssemblyInstance.cpp:113:  Extra space before )  [whitespace/parens] [2]
ERROR: Source/JavaScriptCore/wasm/js/JSWebAssemblyInstance.cpp:117:  Extra space before )  [whitespace/parens] [2]
Total errors found: 3 in 51 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Keith Miller]

Created attachment 305449 [details]
Patch

[Build Bot]

Attachment 305449 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/wasm/JSWebAssembly.cpp:176:  Extra space before )  [whitespace/parens] [2]
ERROR: Source/JavaScriptCore/wasm/js/JSWebAssemblyInstance.cpp:113:  Extra space before )  [whitespace/parens] [2]
ERROR: Source/JavaScriptCore/wasm/js/JSWebAssemblyInstance.cpp:117:  Extra space before )  [whitespace/parens] [2]
Total errors found: 3 in 51 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Keith Miller]

Created attachment 305450 [details]
Patch

[Build Bot]

Attachment 305450 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/wasm/JSWebAssembly.cpp:176:  Extra space before )  [whitespace/parens] [2]
ERROR: Source/JavaScriptCore/wasm/js/JSWebAssemblyInstance.cpp:113:  Extra space before )  [whitespace/parens] [2]
ERROR: Source/JavaScriptCore/wasm/js/JSWebAssemblyInstance.cpp:117:  Extra space before )  [whitespace/parens] [2]
Total errors found: 3 in 52 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Keith Miller]

Created attachment 305451 [details]
Patch

[Build Bot]

Attachment 305451 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/wasm/JSWebAssembly.cpp:176:  Extra space before )  [whitespace/parens] [2]
ERROR: Source/JavaScriptCore/wasm/js/JSWebAssemblyInstance.cpp:113:  Extra space before )  [whitespace/parens] [2]
ERROR: Source/JavaScriptCore/wasm/js/JSWebAssemblyInstance.cpp:117:  Extra space before )  [whitespace/parens] [2]
Total errors found: 3 in 52 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 305451 [details]
Patch

Attachment 305451 [details] did not pass jsc-ews (mac):
Output: http://webkit-queues.webkit.org/results/3416753

New failing tests:
profiler-test.yaml/tests/sunspider-1.0/access-nbody.js.profiler-simple
profiler-test.yaml/tests/sunspider-1.0/bitops-bits-in-byte.js.profiler-simple
profiler-test.yaml/tests/sunspider-1.0/string-validate-input.js.profiler-simple
profiler-test.yaml/tests/sunspider-1.0/string-tagcloud.js.profiler-simple
profiler-test.yaml/tests/sunspider-1.0/access-fannkuch.js.profiler-simple
profiler-test.yaml/tests/sunspider-1.0/string-unpack-code.js.profiler-simple
profiler-test.yaml/tests/sunspider-1.0/string-base64.js.profiler-simple
profiler-test.yaml/tests/sunspider-1.0/regexp-dna.js.profiler-simple
profiler-test.yaml/tests/sunspider-1.0/3d-cube.js.profiler-simple
profiler-test.yaml/tests/sunspider-1.0/controlflow-recursive.js.profiler-simple
profiler-test.yaml/tests/sunspider-1.0/bitops-3bit-bits-in-byte.js.profiler-simple
profiler-test.yaml/tests/sunspider-1.0/math-cordic.js.profiler-simple
profiler-test.yaml/tests/sunspider-1.0/string-fasta.js.profiler-simple
stress/tail-call-profiler.js.profiler-simple
profiler-test.yaml/tests/sunspider-1.0/crypto-aes.js.profiler-simple
profiler-test.yaml/tests/sunspider-1.0/3d-morph.js.profiler-simple
profiler-test.yaml/tests/sunspider-1.0/access-binary-trees.js.profiler-simple
profiler-test.yaml/tests/sunspider-1.0/date-format-tofte.js.profiler-simple
profiler-test.yaml/tests/sunspider-1.0/3d-raytrace.js.profiler-simple
profiler-test.yaml/tests/sunspider-1.0/bitops-nsieve-bits.js.profiler-simple
profiler-test.yaml/tests/sunspider-1.0/crypto-sha1.js.profiler-simple
stress/op-push-name-scope-crashes-profiler.js.profiler-simple
profiler-test.yaml/tests/sunspider-1.0/bitops-bitwise-and.js.profiler-simple
profiler-test.yaml/tests/sunspider-1.0/math-partial-sums.js.profiler-simple
profiler-test.yaml/tests/sunspider-1.0/crypto-md5.js.profiler-simple
profiler-test.yaml/tests/sunspider-1.0/date-format-xparb.js.profiler-simple
profiler-test.yaml/tests/sunspider-1.0/math-spectral-norm.js.profiler-simple
profiler-test.yaml/tests/sunspider-1.0/access-nsieve.js.profiler-simple

[Michael Saboff]

Comment on attachment 305451 [details]
Patch

r-
Looks like this patch breaks the profiler.

[Keith Miller]

(In reply to Michael Saboff from comment #15)
> Comment on attachment 305451 [details]
> Patch
> 
> r-
> Looks like this patch breaks the profiler.

Yeah, I'm looking into it. It looks like the GC doesn't like that I'm stopping the run loop when all the async tasks are finished. I'm not sure how to get around that. I suppose we could just not profile anything async but that would be unfortunate...

[Keith Miller]

(In reply to Keith Miller from comment #16)
> (In reply to Michael Saboff from comment #15)
> > Comment on attachment 305451 [details]
> > Patch
> > 
> > r-
> > Looks like this patch breaks the profiler.
> 
> Yeah, I'm looking into it. It looks like the GC doesn't like that I'm
> stopping the run loop when all the async tasks are finished. I'm not sure
> how to get around that. I suppose we could just not profile anything async
> but that would be unfortunate...

It also only seems to happen in release, which is weird.

[Keith Miller]

Created attachment 305519 [details]
Patch

[Build Bot]

Attachment 305519 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/wasm/JSWebAssembly.cpp:174:  Extra space before )  [whitespace/parens] [2]
ERROR: Source/JavaScriptCore/wasm/js/JSWebAssemblyInstance.cpp:113:  Extra space before )  [whitespace/parens] [2]
ERROR: Source/JavaScriptCore/wasm/js/JSWebAssemblyInstance.cpp:117:  Extra space before )  [whitespace/parens] [2]
Total errors found: 3 in 53 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Keith Miller]

Created attachment 305520 [details]
Patch

[Keith Miller]

Created attachment 305524 [details]
Patch

[Keith Miller]

Created attachment 305595 [details]
Patch

[Keith Miller]

Created attachment 305602 [details]
Patch

[Saam Barati]

Comment on attachment 305602 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=305602&action=review

r=me with comments

> Source/JavaScriptCore/ChangeLog:30
> +        WebAssembly.instantiate() then new WebAssembly() on the same

typo: "new WebAssembly()" => "new WebAssembly.Instance()"

> Source/JavaScriptCore/wasm/JSWebAssembly.cpp:155
> +    // FIXME: This reparses the module header, which shouldn't be necessary.

Please file a bug and link it here.

> Source/JavaScriptCore/wasm/WasmPlan.cpp:327
> +

please revert

> Source/JavaScriptCore/wasm/WasmWorklist.cpp:69
> +    void attemptToRemoveCompilationPlan(const AbstractLocker&)
> +    {
> +        auto& queue = worklist.m_queue;
> +
> +        if (queue.empty())
> +            return;
> +
> +        if (queue.top().plan == element.plan)
> +            queue.pop();
> +    }

This function looks error prone. Why not just have something that straight up removes the plan from the queue?
What if something is enqueued w/ higher priority, and element.plan is no longer at the top of the queue?

> Source/JavaScriptCore/wasm/WasmWorklist.cpp:85
> +            if (!queue.top().plan->hasBeenPrepared())
> +                queue.pop();

Style nit: I think this is a tad easier to to read if you just return PollResult::Work after pop().

> Source/JavaScriptCore/wasm/WasmWorklist.cpp:119
> +        // FIXME: this should check in at some occasionally to see if there are new, higher priority e.g. synchronous, plans that need to be run.

Please file a bug and link to it here.

> Source/JavaScriptCore/wasm/WasmWorklist.cpp:133
> +void Worklist::QueueElement::nextPriority()

Style nit: Maybe call this something that indicates it's mutating the priority. Maybe, "setToNextPriority"?

> Source/JavaScriptCore/wasm/WasmWorklist.cpp:240
> +    LockHolder locker(*m_lock);

Why do we need to hold the lock here?

> Source/JavaScriptCore/wasm/WasmWorklist.cpp:268
> +
> +

style: remove extra white space.

> Source/JavaScriptCore/wasm/js/JSWebAssemblyCodeBlock.cpp:49
> +    module()->m_codeBlocks[static_cast<size_t>(mode)].set(vm, this, this);

Shouldn't this be a write barrier with the module as the owner?
Why not have a Module::setCodeBlock(Mode, CodeBlock*) function?

[Saam Barati]

Also, please file a bug about getting the run loop to work in the shell in other platforms.

[Keith Miller]

Comment on attachment 305602 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=305602&action=review

>> Source/JavaScriptCore/wasm/JSWebAssembly.cpp:155
>> +    // FIXME: This reparses the module header, which shouldn't be necessary.
> 
> Please file a bug and link it here.

Fixed.

>> Source/JavaScriptCore/wasm/WasmPlan.cpp:327
>> +
> 
> please revert

Fixed.

>> Source/JavaScriptCore/wasm/WasmWorklist.cpp:69
>> +    }
> 
> This function looks error prone. Why not just have something that straight up removes the plan from the queue?
> What if something is enqueued w/ higher priority, and element.plan is no longer at the top of the queue?

I think it should still work but I switched the code to just remove the plan in the poll case and do this in the work case.

>> Source/JavaScriptCore/wasm/WasmWorklist.cpp:85
>> +                queue.pop();
> 
> Style nit: I think this is a tad easier to to read if you just return PollResult::Work after pop().

Fixed.

>> Source/JavaScriptCore/wasm/WasmWorklist.cpp:119
>> +        // FIXME: this should check in at some occasionally to see if there are new, higher priority e.g. synchronous, plans that need to be run.
> 
> Please file a bug and link to it here.

Fixed.

>> Source/JavaScriptCore/wasm/WasmWorklist.cpp:133
>> +void Worklist::QueueElement::nextPriority()
> 
> Style nit: Maybe call this something that indicates it's mutating the priority. Maybe, "setToNextPriority"?

Fixed.

>> Source/JavaScriptCore/wasm/WasmWorklist.cpp:240
>> +    LockHolder locker(*m_lock);
> 
> Why do we need to hold the lock here?

I'm not 100% sure why that's what the other places that use AutomaticThread do.

>> Source/JavaScriptCore/wasm/WasmWorklist.cpp:268
>> +
> 
> style: remove extra white space.

Fixed.

>> Source/JavaScriptCore/wasm/js/JSWebAssemblyCodeBlock.cpp:49
>> +    module()->m_codeBlocks[static_cast<size_t>(mode)].set(vm, this, this);
> 
> Shouldn't this be a write barrier with the module as the owner?
> Why not have a Module::setCodeBlock(Mode, CodeBlock*) function?

Whoops, your right. Fixed. Also, I added the setCodeBlock function.

[Keith Miller]

Committed r214504: <http://trac.webkit.org/changeset/214504>

[Carlos Garcia Campos]

Does this patch requires any platform specific code in Linux, it seems it caused 4 wasm tests to crash in GTK+ port:

wasm.yaml/wasm/js-api/web-assembly-instantiate-parallel.js.default-wasm: Segmentation fault (core dumped)
                         
wasm.yaml/wasm/js-api/web-assembly-instantiate-parallel.js.default-wasm: ERROR: Unexpected exit code: 139
                                    
wasm.yaml/wasm/js-api/web-assembly-compile-parallel.js.default-wasm: Segmentation fault (core dumped)
                                    
wasm.yaml/wasm/js-api/web-assembly-compile-parallel.js.default-wasm: ERROR: Unexpected exit code: 139
                                    
wasm.yaml/wasm/js-api/web-assembly-instantiate.js.default-wasm: Segmentation fault (core dumped)
                                    
wasm.yaml/wasm/js-api/web-assembly-instantiate.js.default-wasm: ERROR: Unexpected exit code: 139
                                   
wasm.yaml/wasm/js-api/Module-compile.js.default-wasm: Segmentation fault (core dumped)
                                    
wasm.yaml/wasm/js-api/Module-compile.js.default-wasm: ERROR: Unexpected exit code: 139

[Keith Miller]

(In reply to Carlos Garcia Campos from comment #28)
> Does this patch requires any platform specific code in Linux, it seems it
> caused 4 wasm tests to crash in GTK+ port:
> 
> wasm.yaml/wasm/js-api/web-assembly-instantiate-parallel.js.default-wasm:
> Segmentation fault (core dumped)
>                          
> wasm.yaml/wasm/js-api/web-assembly-instantiate-parallel.js.default-wasm:
> ERROR: Unexpected exit code: 139
>                                     
> wasm.yaml/wasm/js-api/web-assembly-compile-parallel.js.default-wasm:
> Segmentation fault (core dumped)
>                                     
> wasm.yaml/wasm/js-api/web-assembly-compile-parallel.js.default-wasm: ERROR:
> Unexpected exit code: 139
>                                     
> wasm.yaml/wasm/js-api/web-assembly-instantiate.js.default-wasm: Segmentation
> fault (core dumped)
>                                     
> wasm.yaml/wasm/js-api/web-assembly-instantiate.js.default-wasm: ERROR:
> Unexpected exit code: 139
>                                    
> wasm.yaml/wasm/js-api/Module-compile.js.default-wasm: Segmentation fault
> (core dumped)
>                                     
> wasm.yaml/wasm/js-api/Module-compile.js.default-wasm: ERROR: Unexpected exit
> code: 139

Yeah, those tests are going to fail because the JSC CLI doesn't know how to run the runloop if you don't have CFRunLoop. Other than that I don't believe anything special should be needed. Do you have any of the backtraces?

[Keith Miller]

(In reply to Keith Miller from comment #29)
> Yeah, those tests are going to fail because the JSC CLI doesn't know how to
> run the runloop if you don't have CFRunLoop. Other than that I don't believe
> anything special should be needed. Do you have any of the backtraces?

I'm actually a little surprised it's crashing. If I had to guess, it's crashing because there isn't a timer set up for the PromiseDeferredTimer so we deref null. Perhaps there is a bug that only shows on Linux, though. Hard to tell without the backtrace.

[Carlos Garcia Campos]

(In reply to Keith Miller from comment #30)
> (In reply to Keith Miller from comment #29)
> > Yeah, those tests are going to fail because the JSC CLI doesn't know how to
> > run the runloop if you don't have CFRunLoop. Other than that I don't believe
> > anything special should be needed. Do you have any of the backtraces?
> 
> I'm actually a little surprised it's crashing. If I had to guess, it's
> crashing because there isn't a timer set up for the PromiseDeferredTimer so
> we deref null. Perhaps there is a bug that only shows on Linux, though. Hard
> to tell without the backtrace.

Right, there are two different problems, one is the crash that happens before the timer is used (see bug #170390) and the other is that promise deferred callback is only implemented by CF. With the crashes fixed, then tests fail instead of crash:

wasm.yaml/wasm/js-api/Module-compile.js.default-wasm: ERROR: Unexpected exit code: 3
wasm.yaml/wasm/js-api/web-assembly-compile-parallel.js.default-wasm: ERROR: Unexpected exit code: 3
wasm.yaml/wasm/js-api/web-assembly-instantiate.js.default-wasm: ERROR: Unexpected exit code: 3
wasm.yaml/wasm/js-api/web-assembly-instantiate-parallel.js.default-wasm: ERROR: Unexpected exit code: 3