Bug 169187

Created attachment 305148 [details] WIP

Created attachment 305199 [details] WIP

Created attachment 305266 [details] Builds

Created attachment 305377 [details] works but needs tests

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.

Created attachment 305448 [details] Patch

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.

Created attachment 305449 [details] Patch

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.

Created attachment 305450 [details] Patch

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.

Created attachment 305451 [details] Patch

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.

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

Comment on attachment 305451 [details] Patch r- Looks like this patch breaks the profiler.

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

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

Created attachment 305519 [details] Patch

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.

Created attachment 305520 [details] Patch

Created attachment 305524 [details] Patch

Created attachment 305595 [details] Patch

Created attachment 305602 [details] Patch

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?

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

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.

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

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

(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?

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

(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