[webkit-reviews] review granted: [Bug 175150] [Linux] Clear WasmMemory with madvice instead of memset : [Attachment 317404] Patch

[bugzilla-daemon at webkit.org]

Filip Pizlo <fpizlo at apple.com> has granted Yusuke Suzuki
<utatane.tea at gmail.com>'s request for review:
Bug 175150: [Linux] Clear WasmMemory with madvice instead of memset
https://bugs.webkit.org/show_bug.cgi?id=175150

Attachment 317404: Patch

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




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

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

This patch seems sensible to me.  r=me.

>>>>>> Source/JavaScriptCore/wasm/WasmMemory.cpp:263
>>>>>> +    while (madvise(startAddress, sizeInBytes, MADV_DONTNEED) == -1 &&
errno == EAGAIN) { }
>>>>> 
>>>>> This feels wrong. The programmer is telling us they need this memory
(most likely because they will run a wasm program and access it), but you’re
telling the kernel you don’t expect this memory to be accessed anytime soon.
This feels wrong, and it seems possible that it will slow down actual Wasm
programs.
>>>> 
>>>> That's why we immediately execute OSAllocator::commit after that.
>>> 
>>> For the case of the small size, memset(...) would be nice here.
>>> I don't think memset all the area of this here, it exhausts memory even if
it is not used.
>>> The threshold should be decided based on the result of some benchmark.
>> 
>> I mean, I don't think clearing all the area here by using memset is good
way. I t exhausts memory even if it is not used. (And currently, it exhausts
memory in Linux).
> 
> It exhausts memory also on Mac.
> 
> Basically this test is asking for _two_ memory segments with an _initial_
size of 4GBs.
> 
> See
https://trac.webkit.org/browser/trunk/JSTests/wasm/js-api/test_memory_construct
or.js?rev=220265#L44
> and
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Object
s/WebAssembly/Memory
> 
> And 6 subtests are executed on parallel. So...., the only way this test can
run fine is if your machine has 48GB of RAM.
> 
> On Mac it throws an error (therefore the messages I saw here
https://bugs.webkit.org/show_bug.cgi?id=175150#c18 about dmesg).
> On Linux it either does the same (throw) or gets killed by the Linux OOM
killer.
> 
> On Mac causing an OOM condition seems more or less fine (you maybe even don't
notice). On Linux is not fine. Your computer will freeze for several seconds. 
A similar issue was discussed on bug 175100
> 
> 
> Do we really need to try to allocate so much memory for this test? The Mac
JSC bots "only" have 16GBs of RAM. I think this test is really throwing
exceptions there because there is not enough memory to run the 6 subtests at
the same time. And instead of testing what it was meant to test, its testing
that it can throw an error (that is catched) for not enough memory.
> 
> However, I think this patch is a really interesting approach and can make
things better when some application requests more initial memory than it will
really needs. Ideally this should be done with some heuristics based on the
requested size. If the size is small using memset(0) is probably better than
calling MADV_DONTNEED. I'm speculating here as I have no real idea, some
benchmark or empiric testing will be needed.

I agree with Carlos that we should not have a test that allocates 4GB of RAM,
unless some precautions are taken, like putting "//@ runDefault" at the top to
make it only run one of the configurations instead of N.  Also, instead of
allocating 4GB, we should think hard about what we can do to test the same code
by requesting less memory.  If it's an OOM test, we could add options to force
the memory allocator to assume that there is less memory than there really is,
so that we can drive the VM to OOM in a more reasonable amount of memory.