Bug 170134 – WebAssembly: Implement tier up

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Saam Barati]

....

[Radar WebKit Bug Importer]

<rdar://problem/31301163>

[Keith Miller]

Created attachment 308111 [details]
WIP

[Keith Miller]

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

[Filip Pizlo]

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.

[Keith Miller]

Created attachment 308180 [details]
WIP

[Keith Miller]

Created attachment 308299 [details]
Patch

[Build Bot]

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.

[Filip Pizlo]

Comment on attachment 308299 [details]
Patch

File a bug about keeping the list of callsites in zombie BBQ code.

[Keith Miller]

Created attachment 308304 [details]
Patch

[Keith Miller]

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

[Saam Barati]

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

[Saam Barati]

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.

[Keith Miller]

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.

[JF Bastien]

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