Bug 170134

<rdar://problem/31301163>

Created attachment 308111 [details] WIP

Just rewrote 50% of this so it's super broken still.

Comment on attachment 308111 [details] WIP View in context: https://bugs.webkit.org/attachment.cgi?id=308111&action=review Looks pretty good to me. > Source/JavaScriptCore/jsc.cpp:3194 > + Ref<Wasm::BBQPlan> plan = adoptRef(*new Wasm::BBQPlan(vm, static_cast<uint8_t*>(source->vector()), source->length(), Wasm::BBQPlan::FullCompile, Wasm::Plan::dontFinalize())); This naming scheme is epic. > Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:859 > + BasicBlock* continuation = m_proc.addBlock(); > + > + Value* countDownLocation = constant(pointerType(), reinterpret_cast<uint64_t>(m_tierUp)); > + Value* oldCountDown = entry->appendNew<MemoryValue>(m_proc, Load, Int32, origin(), countDownLocation); > + Value* newCountDown = entry->appendNew<Value>(m_proc, Sub, origin(), oldCountDown, constant(Int32, decrementCount)); > + entry->appendNew<MemoryValue>(m_proc, Store, origin(), newCountDown, countDownLocation); > + > + Value* underFlowed = entry->appendNew<Value>(m_proc, Above, origin(), newCountDown, oldCountDown); > + > + { > + BasicBlock* tierUp = m_proc.addBlock(); > + entry->appendNew<Value>(m_proc, B3::Branch, origin(), underFlowed); > + entry->setSuccessors(FrequentedBlock(tierUp, FrequencyClass::Rare), FrequentedBlock(continuation)); > + > + tierUp->appendNew<CCallValue>(m_proc, B3::Void, origin(), Effects::forCall(), > + constant(pointerType(), reinterpret_cast<uint64_t>(runOMGPlanForIndex)), > + materializeWasmContext(tierUp), > + constant(Int32, m_functionIndex)); > + > + tierUp->appendNewControlValue(m_proc, Jump, origin(), continuation); > + } > + > + return continuation; Interesting! I would have made this a patchpoint to save compile time. Compile time is correlated to basic block count. No need to make this change now. I think it's fine to land it like this. I would put in a FIXME and link to a bug.

Created attachment 308180 [details] WIP

Created attachment 308299 [details] Patch

Attachment 308299 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/jsc.cpp:81: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:837: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/JavaScriptCore/wasm/WasmTierUpCount.h:31: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/JavaScriptCore/wasm/WasmModule.cpp:32: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/JavaScriptCore/wasm/WasmBBQPlan.cpp:188: Wrong number of spaces before statement. (expected: 8) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/wasm/WasmBBQPlan.cpp:309: A case label should not be indented, but line up with its switch statement. [whitespace/indent] [4] ERROR: Source/JavaScriptCore/wasm/WasmFormat.h:48: Code inside a namespace should not be indented. [whitespace/indent] [4] ERROR: Source/JavaScriptCore/wasm/WasmOMGPlan.h:71: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] ERROR: Source/JavaScriptCore/wasm/WasmOMGPlan.cpp:42: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/JavaScriptCore/wasm/WasmOMGPlan.cpp:139: More than one command on the same line [whitespace/newline] [4] ERROR: Source/JavaScriptCore/wasm/WasmCodeBlock.cpp:33: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/JavaScriptCore/wasm/WasmBBQPlanInlines.h:31: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 12 in 46 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 308299 [details] Patch File a bug about keeping the list of callsites in zombie BBQ code.

Created attachment 308304 [details] Patch

Committed r215843: <http://trac.webkit.org/changeset/215843>

Comment on attachment 308304 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=308304&action=review > Source/JavaScriptCore/assembler/AbstractMacroAssembler.h:1024 > + void ensureCacheLineSpace(size_t instructionSize) Is this even called from anywhere? > Source/JavaScriptCore/assembler/AbstractMacroAssembler.h:1027 > + const size_t cacheLineSize = 64; How does this work when we do branch compaction? I would've thought this should live in link buffer. > Source/JavaScriptCore/wasm/WasmOMGPlan.cpp:2 > + * Copyright (C) 2016 Apple Inc. All rights reserved. 2017

Comment on attachment 308304 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=308304&action=review > Source/JavaScriptCore/wasm/WasmBBQPlan.cpp:2 > + * Copyright (C) 2016 Apple Inc. All rights reserved. 2017 > Source/JavaScriptCore/wasm/WasmCodeBlock.h:123 > + Vector<void*> m_wasmEntryPoints; Is this only used for indirect calls? I wonder if we can pick a better name. > Source/JavaScriptCore/wasm/WasmMachineThreads.cpp:40 > + static LazyNeverDestroyed<MachineThreads> threads; Does MachineThreads handle when threads get destroyed? > Source/JavaScriptCore/wasm/WasmOMGPlan.cpp:53 > +OMGPlan::OMGPlan(VM& vm, Ref<Module> module, uint32_t functionIndex, MemoryMode mode, CompletionTask&& task) This naming is glorious > Source/JavaScriptCore/wasm/WasmOMGPlan.cpp:127 > + // the updates to B but still not have reset it's cache of A', which would lead to all kinds of badness. typo: it's => its > Source/JavaScriptCore/wasm/WasmOMGPlan.cpp:149 > + for (unsigned i = 0; i < m_codeBlock->m_wasmToWasmCallsites.size(); ++i) { > + if (i != functionIndexSpace) > + repatchCalls(m_codeBlock->m_wasmToWasmCallsites[i]); > + } I wonder if we should come up w/ a more efficient way to do this. > Source/JavaScriptCore/wasm/WasmTierUpCount.h:41 > + // This class manages the tier up counts for Wasm binaries. The main interesting thing about > + // wasm tiering up counts is that the least significant bit indicates if the tier up has already > + // started. Also, wasm code does not atomically update this count. This is because we > + // don't care too much if the countdown is slightly off. The tier up trigger is atomic, however, > + // so tier up will be triggered exactly once. > + class TierUpCount { Style: None of this should be indented. > Source/JavaScriptCore/wasm/WasmTierUpCount.h:45 > + : m_count(Options::webAssemblyOMGTierUpCount() * 2) Why not just have the option be the real count? > Source/JavaScriptCore/wasm/WasmTierUpCount.h:61 > + bool shouldStartTierUp() > + { > + return !(WTF::atomicExchangeOr(&m_count, 1) & 1); > + } Style nit: this is deceptively effectful. I wonder if we should use a name that indicates this better.

Comment on attachment 308304 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=308304&action=review >> Source/JavaScriptCore/assembler/AbstractMacroAssembler.h:1024 >> + void ensureCacheLineSpace(size_t instructionSize) > > Is this even called from anywhere? Yeah, I got rid of this. >> Source/JavaScriptCore/assembler/AbstractMacroAssembler.h:1027 >> + const size_t cacheLineSize = 64; > > How does this work when we do branch compaction? I would've thought this should live in link buffer. Yeah, this was wrong, I forgot to delete this. >> Source/JavaScriptCore/wasm/WasmBBQPlan.cpp:2 >> + * Copyright (C) 2016 Apple Inc. All rights reserved. > > 2017 Whoops, I'll fix. >> Source/JavaScriptCore/wasm/WasmCodeBlock.h:123 >> + Vector<void*> m_wasmEntryPoints; > > Is this only used for indirect calls? I wonder if we can pick a better name. Yeah, We could call it m_wasmIndirectCallEntrypoints. That kinda makes it sound like it's different than the regular entrypoint though. >> Source/JavaScriptCore/wasm/WasmMachineThreads.cpp:40 >> + static LazyNeverDestroyed<MachineThreads> threads; > > Does MachineThreads handle when threads get destroyed? Yeah, they are destroyed (and removed from the list) when pthreads frees some thread specific data we allocate. >> Source/JavaScriptCore/wasm/WasmOMGPlan.cpp:2 >> + * Copyright (C) 2016 Apple Inc. All rights reserved. > > 2017 Whoops, I'll fix. >> Source/JavaScriptCore/wasm/WasmOMGPlan.cpp:127 >> + // the updates to B but still not have reset it's cache of A', which would lead to all kinds of badness. > > typo: it's => its Ditto. >> Source/JavaScriptCore/wasm/WasmOMGPlan.cpp:149 >> + } > > I wonder if we should come up w/ a more efficient way to do this. Yeah, we could keep another list. It might also be more efficient if we did every thing in batches. >> Source/JavaScriptCore/wasm/WasmTierUpCount.h:41 >> + class TierUpCount { > > Style: None of this should be indented. Whoops, I'll fix. >> Source/JavaScriptCore/wasm/WasmTierUpCount.h:45 >> + : m_count(Options::webAssemblyOMGTierUpCount() * 2) > > Why not just have the option be the real count? Then we would need to be sure that the count was a multiple of 2. Also, it would make the option harder to read since you would always need to know the number should be divided by two. >> Source/JavaScriptCore/wasm/WasmTierUpCount.h:61 >> + } > > Style nit: > this is deceptively effectful. I wonder if we should use a name that indicates this better. Hmm, I'll try to think of a better name.

*** Bug 178165 has been marked as a duplicate of this bug. ***