[webkit-reviews] review requested: [Bug 170628] WebAssembly: manage memory better : [Attachment 306919] patch

[bugzilla-daemon at webkit.org]

JF Bastien <jfbastien at apple.com> has asked  for review:
Bug 170628: WebAssembly: manage memory better
https://bugs.webkit.org/show_bug.cgi?id=170628

Attachment 306919: patch

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




--- Comment #19 from JF Bastien <jfbastien at apple.com> ---
Created attachment 306919

  --> https://bugs.webkit.org/attachment.cgi?id=306919&action=review

patch

> 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?

Because I want to get different memories with different declarations each time,
and would rather not pre-allocate the memory before 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.

Filed https://bugs.webkit.org/show_bug.cgi?id=170773


> > 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.

I updated the ChangeLog with some of the details I posted before, and improved
comments around this code. I also simplified some of the atomics.


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

Copy / pasta! Fixed.