Bug 162693 – WASM should support faster loads.

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Keith Miller]

...

[Keith Miller]

Created attachment 302873 [details]
WIP

[Keith Miller]

Created attachment 302888 [details]
WIP

[Keith Miller]

Seems to not crash when doing an invalid load. Now to fix some new failing tests and cleanup.

[Keith Miller]

Created attachment 302971 [details]
WIP

[Keith Miller]

I realized that I need to have a code block for Wasm Modules since we might need to have different blocks for different memory types. I've already added a JSWebAssemblyCodeBlock class (no relation no the JS one, maybe I should pick a different name?) but I need to make it play nice with Wasm::Plan.

[Keith Miller]

Created attachment 303026 [details]
Compiles

[Keith Miller]

Created attachment 303107 [details]
WIP

[Keith Miller]

Created attachment 303172 [details]
passes tests

[Keith Miller]

Created attachment 303204 [details]
Patch

[Keith Miller]

I'm going to test on iOS now.

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/wasm/js/JSWebAssemblyModule.cpp:123:  Missing space after ,  [whitespace/comma] [3]
ERROR: Source/JavaScriptCore/wasm/js/JSWebAssemblyModule.h:70:  The parameter name "mode" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/wasm/js/JSWebAssemblyInstance.cpp:47:  Missing space after ,  [whitespace/comma] [3]
Total errors found: 3 in 37 files


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

[Keith Miller]

Created attachment 303211 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/wasm/js/JSWebAssemblyModule.cpp:123:  Missing space after ,  [whitespace/comma] [3]
ERROR: Source/JavaScriptCore/wasm/js/JSWebAssemblyModule.h:70:  The parameter name "mode" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/wasm/js/JSWebAssemblyInstance.cpp:47:  Missing space after ,  [whitespace/comma] [3]
Total errors found: 3 in 37 files


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

[Keith Miller]

Created attachment 303214 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/wasm/js/JSWebAssemblyModule.cpp:123:  Missing space after ,  [whitespace/comma] [3]
ERROR: Source/JavaScriptCore/wasm/js/JSWebAssemblyModule.h:70:  The parameter name "mode" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/wasm/js/JSWebAssemblyInstance.cpp:47:  Missing space after ,  [whitespace/comma] [3]
Total errors found: 3 in 37 files


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

[Saam Barati]

Comment on attachment 303214 [details]
Patch

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

Initial pass of comments. Gonna keep reviewing.

> Source/JavaScriptCore/wasm/WasmFaultSignalHandler.cpp:42
> +static const bool verbose = false;

Style: Why not just constexpr? Then I suspect this wouldn't need to be in an anonymous namespace.

> Source/JavaScriptCore/wasm/WasmFaultSignalHandler.cpp:85
> +                FirstArgumentGPR = static_cast<uint64_t>(ExceptionType::OutOfBoundsMemoryAccess);
> +                InstructionPointerGPR = reinterpret_cast<uint64_t>(exceptionStub.code().executableAddress());

Style: These feel like they should be functions, and the function can handle the various platforms as needed.

> Source/JavaScriptCore/wasm/WasmFaultSignalHandler.cpp:91
> +    sigaction(signal, &oldSigBusHandler, nullptr);

It might be worth a comment here, saying, we only use fastMemory in processes we control, and we expect the old handler to crash.
Otherwise, we'd need a way to re-install the Wasm handler if we think there is any way old handler would recover from the SIGBUS.

> Source/JavaScriptCore/wasm/WasmMemory.cpp:59
> +// We use this as a heuristic to guess mode a memory import will be. Most of the time we expect users to

"guess mode" => "guess what mode"
Also, maybe you can open a bug to consider alternatives to this? And to test out various programs to see how they behave?

> Source/JavaScriptCore/wasm/WasmMemory.cpp:66
> +Memory::Mode Memory::lastAllocatedMode()
>  {
> -    switch (mode) {
> -    case Mode::BoundsChecking: return "BoundsChecking";
> +    return lastAllocatedMemoryMode;
> +}

I don't think this will work because it's racy inside the process.

> Source/JavaScriptCore/wasm/WasmMemory.cpp:68
> +static_assert(sizeof(uint64_t) == sizeof(size_t), "We rely on allowing the maximum size of Memory we map to be 2^33 which is larger than fits in a 32-bit integer that we'd pass to mprotect if this didn't hold.");

Nit: It's not quite 2^33, it's 2^33 - 2

> Source/JavaScriptCore/wasm/WasmMemory.cpp:70
> +static const size_t fastMemoryMappedBytes = static_cast<size_t>(std::numeric_limits<uint32_t>::max()) * 2; // pointer max + offset max. This is all we need since a load straddling readable memory will trap.

Nit: I think this could use some explaining:
The largest possible pointer is UINT32_MAX, the largest possible offset is also UINT32_MAX, therefore, the largest possible base of a load is UINT32_MAX + UINT32_MAX

Stated this way, I wonder if your size needs +1 to it?

> Source/JavaScriptCore/wasm/WasmMemory.cpp:77
> +inline Deque<void*, maxFastMemories>& availableFastMemories()
> +{
> +    static NeverDestroyed<Deque<void*, maxFastMemories>> availableFastMemories;
> +    return availableFastMemories;
> +}

You need a lock protecting accesses to this, given that it's per process.

> Source/JavaScriptCore/wasm/WasmMemory.cpp:134
> +    // We need to mprotect PROT_NONE all the existing memory. If we can't, for
> +    // whatever reason, we should bail out and get rid of this memory.
> +    if (mprotect(memory, writableSize, PROT_NONE))
> +        return;

This should probably crash IMO

> Source/JavaScriptCore/wasm/WasmMemoryInformation.cpp:91
> +        m_mode = Memory::lastAllocatedMode();

See earlier comment about this being racy and wrong. Why not just get this off the created memory if there is one? Or take a racy guess if you didn't create the memory

> Source/JavaScriptCore/wasm/WasmMemoryInformation.h:70
> +    // This is a unique_ptr so we can pass ModuleInformation as const and still be
> +    // able to take ownership of this.

It's not a unique_ptr. Please remove comment.

> Source/JavaScriptCore/wasm/js/JSWebAssemblyMemory.cpp:67
> +    // We can't use a ref here since it doesn't have a copy constructor...

See below, I think you can delete this.

> Source/JavaScriptCore/wasm/js/JSWebAssemblyMemory.cpp:68
> +    RefPtr<Wasm::Memory> memoryRefPtr = &m_memory.get();

I think webkit style is to call this protectedMemory or something similar.

> Source/JavaScriptCore/wasm/js/JSWebAssemblyMemory.cpp:69
> +    auto destructor = [memoryRefPtr] (void*) { };

I think you can make the above a Ref, and then declare this lambda like:

auto destructor = [memoryProtector = WTFMove(memoryProtector)] (void*) { };

[Keith Miller]

Created attachment 303273 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/wasm/js/JSWebAssemblyModule.cpp:123:  Missing space after ,  [whitespace/comma] [3]
ERROR: Source/JavaScriptCore/wasm/js/JSWebAssemblyModule.h:70:  The parameter name "mode" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/wasm/js/JSWebAssemblyInstance.cpp:47:  Missing space after ,  [whitespace/comma] [3]
Total errors found: 3 in 37 files


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

[Saam Barati]

Comment on attachment 303273 [details]
Patch

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

r=me
I think I found one actual bug. I have some misc suggestions, and some requests for tests. Comments below.

> Source/WTF/ChangeLog:17
> +        Also, add a operator! to Ref has the same semantics as C++ refs.

"Ref has" => "Ref that has"

> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:498
> +    auto makeKind = [&] (B3::Opcode opcode) -> B3::Kind {
> +        if (m_info.memory.mode() == Memory::Signaling)
> +            return trapping(opcode);
> +        return opcode;
> +    };

Nit: This is identical to the lambda inside emitStoreOp. You could make it a member function on B3IRGenerator to remove the duplicate code.

> Source/JavaScriptCore/wasm/WasmFaultSignalHandler.cpp:77
> +            if (start <= faultingInstruction && faultingInstruction <= end) {

Should not be <= end. I think it should be <, right?

> Source/JavaScriptCore/wasm/WasmFaultSignalHandler.cpp:82
> +                MacroAssemblerCodeRef exceptionStub = vm->jitStubs->existingCTIStub(throwExceptionFromWasmThunkGenerator);
> +                // If for whatever reason we don't have a stub then we should just treat this like a regular crash.
> +                if (!exceptionStub.size())
> +                    break;

This seems like it should go outside the loop.

> Source/JavaScriptCore/wasm/WasmMemory.cpp:70
> +static const size_t fastMemoryMappedBytes = (static_cast<size_t>(std::numeric_limits<uint32_t>::max()) + 1) * 2; // pointer max + offset max. This is all we need since a load straddling readable memory will trap.

Nit: I think this should be:
UINT32_MAX*2 + 1, not (UINT32_MAX + 1)*2

Also, please add some tests for this. It'd be nice to test loads at the largest possible (pointer+offset) combination.
Can you also test loads that straddle the mapped PROT_READ/WRITE region?

> Source/JavaScriptCore/wasm/WasmMemory.cpp:89
> +    if (!vm.getCTIStub(throwExceptionFromWasmThunkGenerator).size())

Style nit: size seems like a weird condition here. Why not just use ! on the actual MacroAssemblerCodeRef itself?

> Source/JavaScriptCore/wasm/WasmMemory.cpp:260
> +        if (mprotect(static_cast<uint8_t*>(m_memory) + m_size, static_cast<size_t>(desiredSize - m_size), PROT_READ | PROT_WRITE)) {
> +            dataLogLnIf(verbose, "Memory::grow in-place failed ", *this);
> +            return false;
> +        }

I think this should be a crash.

> Source/JavaScriptCore/wasm/WasmMemoryInformation.cpp:94
> +        if (!isImport) {
> +            m_reservedMemory = Memory::create(vm, initial, maximum, Memory::Signaling);
> +            if (m_reservedMemory) {
> +                ASSERT(!!*m_reservedMemory);
> +                m_mode = m_reservedMemory->mode();
> +            }
> +        }
> +        m_mode = Memory::lastAllocatedMode();

This is still a bit wrong. The if (m_reservedMemory) branch should not drop into the ::lastAllocatedMode() assignment.

> Source/JavaScriptCore/wasm/WasmMemoryInformation.h:62
> +    Memory& takeReservedMemory() { ASSERT(hasReservedMemory()); return *m_reservedMemory.leakRef(); }

Style suggestion:
Currently, this is used in only one place, and it correctly handles the leakRef adoptRef semantics, but I think it'd be more clear if this function just returned a Ref<Memory>&&, and then the caller could not get it wrong.

> Source/JavaScriptCore/wasm/js/JSWebAssemblyMemory.cpp:67
> +    // We can't use a ref here since it doesn't have a copy constructor...

Remove comment.

> Source/JavaScriptCore/wasm/js/JSWebAssemblyModule.cpp:97
> +    // We don't have a code block for this mode, we need to recompile...

Do you have a test for this?
If not, please add.

> Source/JavaScriptCore/wasm/js/JSWebAssemblyModule.cpp:101
> +    auto* codeBlock = buildCodeBlock(vm, exec, plan, mode);
> +    RETURN_IF_EXCEPTION(scope, nullptr);

It would be good to have a test for this. Is it possible to have that happen deterministically?

> Source/JavaScriptCore/wasm/js/WebAssemblyInstanceConstructor.cpp:272
> +

Style: Remove newline.

> Source/JavaScriptCore/wasm/js/WebAssemblyModuleConstructor.cpp:83
> +    return JSWebAssemblyModule::create(vm, state, structure, base + byteOffset, byteSize);

You need a scope.release() before this call.

[Keith Miller]

Comment on attachment 303273 [details]
Patch

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

>> Source/WTF/ChangeLog:17
>> +        Also, add a operator! to Ref has the same semantics as C++ refs.
> 
> "Ref has" => "Ref that has"

Fixed.

>> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:498
>> +    };
> 
> Nit: This is identical to the lambda inside emitStoreOp. You could make it a member function on B3IRGenerator to remove the duplicate code.

Fixed.

>> Source/JavaScriptCore/wasm/WasmFaultSignalHandler.cpp:77
>> +            if (start <= faultingInstruction && faultingInstruction <= end) {
> 
> Should not be <= end. I think it should be <, right?

fixed.

>> Source/JavaScriptCore/wasm/WasmFaultSignalHandler.cpp:82
>> +                    break;
> 
> This seems like it should go outside the loop.

We need to have the vm, which we get from the set.

>> Source/JavaScriptCore/wasm/WasmMemory.cpp:70
>> +static const size_t fastMemoryMappedBytes = (static_cast<size_t>(std::numeric_limits<uint32_t>::max()) + 1) * 2; // pointer max + offset max. This is all we need since a load straddling readable memory will trap.
> 
> Nit: I think this should be:
> UINT32_MAX*2 + 1, not (UINT32_MAX + 1)*2
> 
> Also, please add some tests for this. It'd be nice to test loads at the largest possible (pointer+offset) combination.
> Can you also test loads that straddle the mapped PROT_READ/WRITE region?

Changed for simplicity.

Those tests already existed from before, when we did the bounds checking the first time. Although, I'll add a test for the UINT32_MAX*2 + 1 case.

>> Source/JavaScriptCore/wasm/WasmMemory.cpp:89
>> +    if (!vm.getCTIStub(throwExceptionFromWasmThunkGenerator).size())
> 
> Style nit: size seems like a weird condition here. Why not just use ! on the actual MacroAssemblerCodeRef itself?

Oh, I missed there was a operator! on MacroAssemblerCodeRef. Fixed.

>> Source/JavaScriptCore/wasm/WasmMemory.cpp:260
>> +        }
> 
> I think this should be a crash.

I don't think so since we might have just run out of physical memory on the device, even though we were able to reserve the virtual address space before.

>> Source/JavaScriptCore/wasm/WasmMemoryInformation.cpp:94
>> +        m_mode = Memory::lastAllocatedMode();
> 
> This is still a bit wrong. The if (m_reservedMemory) branch should not drop into the ::lastAllocatedMode() assignment.

whoops, fixed.

>> Source/JavaScriptCore/wasm/WasmMemoryInformation.h:62
>> +    Memory& takeReservedMemory() { ASSERT(hasReservedMemory()); return *m_reservedMemory.leakRef(); }
> 
> Style suggestion:
> Currently, this is used in only one place, and it correctly handles the leakRef adoptRef semantics, but I think it'd be more clear if this function just returned a Ref<Memory>&&, and then the caller could not get it wrong.

changed.

>> Source/JavaScriptCore/wasm/js/JSWebAssemblyModule.cpp:97
>> +    // We don't have a code block for this mode, we need to recompile...
> 
> Do you have a test for this?
> If not, please add.

This just happens in some of the tests.

>> Source/JavaScriptCore/wasm/js/JSWebAssemblyModule.cpp:101
>> +    RETURN_IF_EXCEPTION(scope, nullptr);
> 
> It would be good to have a test for this. Is it possible to have that happen deterministically?

I don't know if there is a way to do that now.

>> Source/JavaScriptCore/wasm/js/WebAssemblyInstanceConstructor.cpp:272
>> +
> 
> Style: Remove newline.

fixed.

[Keith Miller]

Created attachment 303337 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 303337 [details]
Patch for landing

Clearing flags on attachment: 303337

Committed r213386: <http://trac.webkit.org/changeset/213386>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Saam Barati]

Comment on attachment 303337 [details]
Patch for landing

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

> Source/JavaScriptCore/wasm/WasmFaultSignalHandler.cpp:77
> +            if (start <= faultingInstruction && faultingInstruction < end) {

Keith, would it also make sense to make sure that the faulting address is in range of one of the fast memories we allocated? Seems like it'd be easy enough to keep a list of them.

[Keith Miller]

(In reply to comment #24)
> Comment on attachment 303337 [details]
> Patch for landing
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=303337&action=review
> 
> > Source/JavaScriptCore/wasm/WasmFaultSignalHandler.cpp:77
> > +            if (start <= faultingInstruction && faultingInstruction < end) {
> 
> Keith, would it also make sense to make sure that the faulting address is in
> range of one of the fast memories we allocated? Seems like it'd be easy
> enough to keep a list of them.

Yeah, we can do that too.

[Saam Barati]

(In reply to comment #25)
> (In reply to comment #24)
> > Comment on attachment 303337 [details]
> > Patch for landing
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=303337&action=review
> > 
> > > Source/JavaScriptCore/wasm/WasmFaultSignalHandler.cpp:77
> > > +            if (start <= faultingInstruction && faultingInstruction < end) {
> > 
> > Keith, would it also make sense to make sure that the faulting address is in
> > range of one of the fast memories we allocated? Seems like it'd be easy
> > enough to keep a list of them.
> 
> Yeah, we can do that too.

I filed:
https://bugs.webkit.org/show_bug.cgi?id=169290