Summary: | WebAssembly: Implement tier up | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Saam Barati <saam> | ||||||||||
Component: | JavaScriptCore | Assignee: | Keith Miller <keith_miller> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | benjamin, buildbot, fpizlo, gauravdewan007, ggaren, gskachkov, jfbastien, keith_miller, mark.lam, msaboff, ticaiolima, webkit-bug-importer, ysuzuki | ||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||
Version: | WebKit Nightly Build | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Bug Depends on: | |||||||||||||
Bug Blocks: | 170133, 170136 | ||||||||||||
Attachments: |
|
Description
Saam Barati
2017-03-27 13:30:31 PDT
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. *** |