[webkit-reviews] review denied: [Bug 170628] WebAssembly: manage memory better : [Attachment 306873] patch

[bugzilla-daemon at webkit.org]

Keith Miller <keith_miller at apple.com> has denied JF Bastien
<jfbastien at apple.com>'s request for review:
Bug 170628: WebAssembly: manage memory better
https://bugs.webkit.org/show_bug.cgi?id=170628

Attachment 306873: patch

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




--- Comment #16 from Keith Miller <keith_miller at apple.com> ---
Comment on attachment 306873
  --> https://bugs.webkit.org/attachment.cgi?id=306873
patch

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

r- on removal of locking for memories, until convinced otherwise. Also, have
some nits.

> JSTests/wasm/function-tests/memory-access-past-4gib.js:61
> +    const memory = new WebAssembly.Memory(memoryDeclaration);
> +    if (verbose)
> +	   print(WebAssemblyMemoryMode(memory));

Why not move this out of the loop?

> Source/JavaScriptCore/ChangeLog:31
> +	   watermark of fast memories was allocated.

Maybe we should have a system where Wasm memories get unmapped if enough GCs
happen without them reentering use.

> Source/JavaScriptCore/ChangeLog:50
> +	   on iOS. Once we fix th above issues we'll want to re-visit and

typo: th => the

> Source/JavaScriptCore/ChangeLog:64
> +	   * b3/B3LowerToAir.cpp: fix a baaaaddd bug where unsigned->signed
> +	   conversion allowed out-of-bounds reads by -2GiB. I'll follow-up in
> +	   bug #170692 to prevent this type of bug once and for all.

Interesting, I'm surprised that no tests caught this.

> Source/JavaScriptCore/b3/B3LowerToAir.cpp:3177
> +		   RELEASE_ASSERT(limitValue <= value->redzoneLimit());

I would make this a debug assert.

> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:353
> +		   RELEASE_ASSERT(m_memorySizeGPR == pinnedGPR);
> +		   break;
> +	       case MemoryMode::Signaling:
> +		   RELEASE_ASSERT(InvalidGPRReg == pinnedGPR);
> +		   break;
> +	       case MemoryMode::NumberOfMemoryModes:
> +		   RELEASE_ASSERT_NOT_REACHED();

Nit: I'm not sure these need to be release asserts. It seems unlikely this will
catch any release bugs in practice. This function might also be called many
times.

> Source/JavaScriptCore/wasm/WasmMemory.cpp:51
> +constexpr size_t fastMemoryCacheHardLimit { 16};

nit: { 16} => { 16 }

> Source/JavaScriptCore/wasm/WasmMemory.cpp:58
> +std::atomic<void*> fastMemoryCache[fastMemoryCacheHardLimit] = {
ATOMIC_VAR_INIT(nullptr) };
> +std::atomic<void*>
currentlyActiveFastMemories[fastMemoryAllocationSoftLimit] = {
ATOMIC_VAR_INIT(nullptr) };
> +std::atomic<size_t> currentlyAllocatedFastMemories = ATOMIC_VAR_INIT(0);
> +std::atomic<size_t> observedMaximumFastMemory = ATOMIC_VAR_INIT(0);

I'm not sure I see what the real advantage of not using a lock here. It feels
like it just makes the other code more complicated. It seems really unlikely
that we are going to be allocating more than one memory at the same time in
different threads. Even if we are, we potentially do a full GC anyway, so we
can't be too worried about a latency problem here.

> Source/JavaScriptCore/wasm/WasmMemory.cpp:124
> +	       dataLogLnIf(verbose && memory, "tryGetFastMemory waited on GC
and retried successfully");

How is this ever true?

> Websites/webkit.org/docs/b3/intermediate-representation.html:661
>	   WasmBoundsCheckValue class.</dd>

You should update this to comment on the InvalidGPR case and the maximum.