[webkit-reviews] review granted: [Bug 177473] WebAssembly: no VM / JS version of everything but Instance : [Attachment 322103] patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Oct 2 10:30:58 PDT 2017


Filip Pizlo <fpizlo at apple.com> has granted JF Bastien <jfbastien at apple.com>'s
request for review:
Bug 177473: WebAssembly: no VM / JS version of everything but Instance
https://bugs.webkit.org/show_bug.cgi?id=177473

Attachment 322103: patch

https://bugs.webkit.org/attachment.cgi?id=322103&action=review




--- Comment #28 from Filip Pizlo <fpizlo at apple.com> ---
Comment on attachment 322103
  --> https://bugs.webkit.org/attachment.cgi?id=322103
patch

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

r=me with comments

> Source/JavaScriptCore/wasm/WasmMemory.cpp:302
> -	   return adoptRef(new Memory(initial, maximum));
> +	   return adoptRef(new Memory(initial, maximum,
std::forward<WTF::Function<void()>>(notifyMemoryPressure),
std::forward<WTF::Function<void()>>(syncTryToReclaimMemory),
std::forward<WTF::Function<void(PageCount, PageCount)>>(growSuccessCallback)));

I think it's weird that you're using std::forward.  Shouldn't this be WTFMove?

> Source/JavaScriptCore/wasm/WasmTable.cpp:93
> +    auto checkedGrow = [&] (auto& container) {
> +	   Checked reallocSizeChecked = newSizeChecked;
> +	   reallocSizeChecked *= sizeof(*container.get());
> +	   uint32_t reallocSize;
> +	   if (reallocSizeChecked.safeGet(reallocSize) ==
CheckedState::DidOverflow)
> +	       return false;
> +	   container.realloc(reallocSize);
> +	   for (uint32_t i = m_size; i < newSize; ++i)
> +	       default_construct_at(&container.get()[i]);
> +	   return true;
> +    };

Nice

> Source/JavaScriptCore/wasm/js/JSWebAssemblyInstance.cpp:344
> +	       RefPtr<Wasm::Memory> memory =
Wasm::Memory::create(moduleInformation.memory.initial(),
moduleInformation.memory.maximum(),
> +		   [&vm] () { vm.heap.collectAsync(CollectionScope::Full); },
> +		   [&vm] () { vm.heap.collectSync(CollectionScope::Full); },
> +		   [&vm, jsMemory] (Wasm::PageCount oldPageCount,
Wasm::PageCount newPageCount) { jsMemory->growSuccessCallback(vm, oldPageCount,
newPageCount); });

That's pretty cool!

The one downside is that it's hard to tell what each callback is for.  One way
to make this easy to read is to introduce dummy enums for each WTF::Function:

enum NotifyLowMemory { NotifyLowMemoryTag }
enum SyncTryToReclaim { SyncTryToReclaimTag }
enum GrowMemory { GrowMemoryTag }

And then this would do:

RefPtr<...> memory = ...(...,
    [&vm] (NotifyLowMemory) { vm.heap.collectAsync(...); },
    [&vm] (SyncTryToRedlaim) ...

I don't feel strongly about it, but it might be nice.

> Source/JavaScriptCore/wasm/js/WasmToJS.cpp:445
> +		   jit.purifyNaN(fprReg);
> +		   jit.moveDoubleTo64(fprReg, scratch);
> +		   materializeDoubleEncodeOffset(doubleEncodeOffsetGPRReg);
> +		   jit.add64(doubleEncodeOffsetGPRReg, scratch);
> +		   jit.store64(scratch,
calleeFrame.withOffset(calleeFrameOffset));
> +		   calleeFrameOffset += sizeof(Register);

Looks very similar to the code above.  Can you create a helper lambda or
something?


More information about the webkit-reviews mailing list