Bug 175150 – [Linux] Clear WasmMemory with madvice instead of memset

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

[Carlos Alberto Lopez Perez]

The test wasm/js-api/test_memory_constructor.js requires lot of RAM to run.

On Linux/WebKitGTK+ it currently peaks around 8GB of RSS and spawns 6 subtests at the same time, therefore it requires ~48 GB of RAM to run without trouble.

On Mac it peaks around 2.5GB of RSS and also spawns 6 subtests (So I guess it will run fine there with 16GB of RAM).

[Carlos Alberto Lopez Perez]

Created attachment 317140 [details]
Patch

[Carlos Alberto Lopez Perez]

Comment on attachment 317140 [details]
Patch

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

> JSTests/wasm/js-api/test_memory_constructor.js:5
>  // FIXME: use the assert library: https://bugs.webkit.org/show_bug.cgi?id=165684
> +//@ skip if $memoryLimited
>  import Builder from '../Builder.js';
>  
>  function assert(b) {

I know this kind of patches (actually gardening related) don't need review.
But I wanted some feedback/review in this specific case.

This test is causing OOM related problems on the Linux bots (We are now running the JSC tests with --memory limited after https://bugs.webkit.org/show_bug.cgi?id=175140 ).
So, I want to land this ASAP and I will land this patch now unreviewed.

Feedback is still welcome (and of course we are always on time to rework or revert the patch if some of you don't think this change is right)

[Carlos Alberto Lopez Perez]

Committed r220265: <http://trac.webkit.org/changeset/220265>

[Radar WebKit Bug Importer]

<rdar://problem/33720817>

[Saam Barati]

Comment on attachment 317140 [details]
Patch

Carlos, is the fast memory optimization enabled on Linux? If so, we’ll allocate about 4GB for a WebAssembly memory if we can. A good place to start is o see if we enable this on Linux. If so, that’s eating up a lot of virtual memory.

[Yusuke Suzuki]

(In reply to Saam Barati from comment #5)
> Comment on attachment 317140 [details]
> Patch
> 
> Carlos, is the fast memory optimization enabled on Linux? If so, we’ll
> allocate about 4GB for a WebAssembly memory if we can. A good place to start
> is o see if we enable this on Linux. If so, that’s eating up a lot of
> virtual memory.

Yes, it is enabled in Linux ports.

[Saam Barati]

(In reply to Yusuke Suzuki from comment #6)
> (In reply to Saam Barati from comment #5)
> > Comment on attachment 317140 [details]
> > Patch
> > 
> > Carlos, is the fast memory optimization enabled on Linux? If so, we’ll
> > allocate about 4GB for a WebAssembly memory if we can. A good place to start
> > is o see if we enable this on Linux. If so, that’s eating up a lot of
> > virtual memory.
> 
> Yes, it is enabled in Linux ports.

It seems likely this is why a lot of memory is being used. That said, it should be just virtual memory, and hopefully not need actual 48GB physical memory. Has this recently regressed? Or has it always been an issue? Maybe it's gigacage related since that code touched the Wasm memory allocation code path. Maybe we're missing a syscall letting the kernel know that it doesn't actually need to allocate physical memory for these pages.

[Yusuke Suzuki]

(In reply to Saam Barati from comment #7)
> (In reply to Yusuke Suzuki from comment #6)
> > (In reply to Saam Barati from comment #5)
> > > Comment on attachment 317140 [details]
> > > Patch
> > > 
> > > Carlos, is the fast memory optimization enabled on Linux? If so, we’ll
> > > allocate about 4GB for a WebAssembly memory if we can. A good place to start
> > > is o see if we enable this on Linux. If so, that’s eating up a lot of
> > > virtual memory.
> > 
> > Yes, it is enabled in Linux ports.
> 
> It seems likely this is why a lot of memory is being used. That said, it
> should be just virtual memory, and hopefully not need actual 48GB physical
> memory. Has this recently regressed? Or has it always been an issue? Maybe
> it's gigacage related since that code touched the Wasm memory allocation
> code path. Maybe we're missing a syscall letting the kernel know that it
> doesn't actually need to allocate physical memory for these pages.

I've just checked and I think it is regression.
Actually, RSS becomes 4GB~ in Linux jsc process. I'm looking into this.

[Yusuke Suzuki]

(In reply to Yusuke Suzuki from comment #8)
> I've just checked and I think it is regression.
> Actually, RSS becomes 4GB~ in Linux jsc process. I'm looking into this.

In WasmMemory, in fast memory case, we use memset to initialize memory.
Yeah, but of course, it populates backing store. Is it intentional?
I wonder if this also populates memory in Darwin.

https://github.com/WebKit/webkit/blob/master/Source/JavaScriptCore/wasm/WasmMemory.cpp#L305

[Yusuke Suzuki]

(In reply to Yusuke Suzuki from comment #9)
> (In reply to Yusuke Suzuki from comment #8)
> > I've just checked and I think it is regression.
> > Actually, RSS becomes 4GB~ in Linux jsc process. I'm looking into this.
> 
> In WasmMemory, in fast memory case, we use memset to initialize memory.
> Yeah, but of course, it populates backing store. Is it intentional?
> I wonder if this also populates memory in Darwin.
> 
> https://github.com/WebKit/webkit/blob/master/Source/JavaScriptCore/wasm/
> WasmMemory.cpp#L305

I think

1. When freeing large memory with AllocationKind == virtual, we should set LargeRange with physicalSize = size because virtual memory could be populated anyway.
2. Do not call memset in WasmMemory. We should rely on bmalloc's large memory allocation. It is responsibility of bmalloc to zero-fill this virtual memory area without populating backing pages. In Linux case, we should call madvice(MADV_DONTNEED).

Fil, Saam and Geoff, what do you think of?

[Yusuke Suzuki]

Reopening to attach new patch.

[Yusuke Suzuki]

Created attachment 317342 [details]
Patch

[Yusuke Suzuki]

Created attachment 317375 [details]
Patch

[Carlos Alberto Lopez Perez]

Comment on attachment 317375 [details]
Patch

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

> JSTests/wasm/js-api/test_memory_constructor.js:4
>  // FIXME: use the assert library: https://bugs.webkit.org/show_bug.cgi?id=165684
> -//@ skip if $memoryLimited
>  import Builder from '../Builder.js';
>  
>  function assert(b) {

When this test runs it spawns 6 jsc process (6 subtests). The difference between this 6 subtests is on the parameter passed:

By checking the ps output I see the base arguments for all 6 subtests are: "--useFTLJIT=true --useFunctionDotArguments=true --maxPerThreadStackUsage=1572864 -m"

And then, the difference on the arguments (from the base ones) for each one of this 6 subtests are:

Subtest 1: Pass no extra paremeter.
Subtest 2: Pass --forceCodeBlockToJettisonDueToOldAge=true
Subtest 3: Pass --useCallICsForWebAssemblyToJSCalls=false 
Subtest 4: Pass --useFastTLSForWasmContext=true --useConcurrentJIT=false --thresholdForJITAfterWarmUp=100 --scribbleFreeCells=true 
Subtest 5: Pass --useFastTLSForWasmContext=false 
Subtest 6: Pass --useWebAssemblyFastMemory=false 


Before this patch, all 6 subtests where allocating 8GB of RSS.

I have tested the patch and after it, 5 of the 6 subtests now allocate only a few MB of RSS (instead of 8GB).

So this is amazing! :)


But there is still one that keeps using 8GB of RAM of RSS (Resident Set Size).

And its the 6# one, the one that runs with --useWebAssemblyFastMemory=false 

You can check it manually by doing this:

# Terminal 1
$ while true; do if ps x -o pid,rss,vsz,command | grep js[c]|grep -v python|grep -v ruby; then  free -m; echo ; fi;done


# Terminal 2
$ cd WebKit
$ cd JSTests/wasm
$ ../../WebKitBuild/Release/bin/jsc --useFTLJIT=true --useFunctionDotArguments=true --maxPerThreadStackUsage=1572864 -m --useWebAssemblyFastMemory=false js-api/test_memory_constructor.js

You should see on the other terminal how the jsc process still peaks at 8GB of RSS for the JSC process.


If we can fix this also this 6th case, then we can remove the "@ skip if $memoryLimited" line.

But otherwise maybe is better to keep it, as running this test still requires 8GB of RAM (which I guess is more than what a memorylimited machine can use without issues)

[Carlos Alberto Lopez Perez]

(In reply to Yusuke Suzuki from comment #9)
> (In reply to Yusuke Suzuki from comment #8)
> > I've just checked and I think it is regression.
> > Actually, RSS becomes 4GB~ in Linux jsc process. I'm looking into this.
> 
> In WasmMemory, in fast memory case, we use memset to initialize memory.
> Yeah, but of course, it populates backing store. Is it intentional?
> I wonder if this also populates memory in Darwin.
> 
> https://github.com/WebKit/webkit/blob/master/Source/JavaScriptCore/wasm/
> WasmMemory.cpp#L305

I think this also populates memory on Darwin. Right? I will be really surprised if this is not the case.

When I tested this on a mac, I saw each one of the 6 subtests peaking at 2.5GB of RSS (on Mac). On Linux they peak at 8GB of RSS.


After the patch https://bugs.webkit.org/attachment.cgi?id=317375 5 of the 6 subtests (the ones that run with useWebAssemblyFastMemory=true) only use a few MBs of RSS (less than 20MB). But on Mac this 5 tests still allocate 2.5GB of RSS (I believe, I don't have a Mac at hand now, I may confirm this tomorrow)

[Yusuke Suzuki]

Created attachment 317404 [details]
Patch

[Yusuke Suzuki]

I keep memoryLimited annotation since this patch only solves the problem under fast memory.
I don't think using madvise to non fast memory thing is not good solution. So, I keep the current one as is. But it is not a problem since Linux ports always use fast memory right now. And fast memory will not be disabled if WebKitGTK+ does not support WebKitLegacy.

[Carlos Alberto Lopez Perez]

(In reply to Carlos Alberto Lopez Perez from comment #15)
> (In reply to Yusuke Suzuki from comment #9)
> > (In reply to Yusuke Suzuki from comment #8)
> > > I've just checked and I think it is regression.
> > > Actually, RSS becomes 4GB~ in Linux jsc process. I'm looking into this.
> > 
> > In WasmMemory, in fast memory case, we use memset to initialize memory.
> > Yeah, but of course, it populates backing store. Is it intentional?
> > I wonder if this also populates memory in Darwin.
> > 
> > https://github.com/WebKit/webkit/blob/master/Source/JavaScriptCore/wasm/
> > WasmMemory.cpp#L305
> 
> I think this also populates memory on Darwin. Right? I will be really
> surprised if this is not the case.
> 
> When I tested this on a mac, I saw each one of the 6 subtests peaking at
> 2.5GB of RSS (on Mac). On Linux they peak at 8GB of RSS.
> 
> 
> After the patch https://bugs.webkit.org/attachment.cgi?id=317375 5 of the 6
> subtests (the ones that run with useWebAssemblyFastMemory=true) only use a
> few MBs of RSS (less than 20MB). But on Mac this 5 tests still allocate
> 2.5GB of RSS (I believe, I don't have a Mac at hand now, I may confirm this
> tomorrow)

Confirmed. Running this test on Mac also requires lot of RAM. So I guess this means that memset(0) also causes allocation of real RAM on Darwin (seems logic).

Each one of this 6 subtests want to use 8GB of RSS also on Mac, but since the Mac that I have for tests "only" has 16GB of RAM and there are 6 subtests on parallel they only get to use 2.5GB (16/6) of RSS each one before the Darwin kernel does something with them (Killing them?). On the dmesg log this appears for all of them:

CODE SIGNING: cs_invalid_page(0x10daeb000): p=11709[jsc] final status 0x0, allowing (remove VALID) page

No idea what that means.


So: the same issue that happens on Linux seems to happen on Mac. But on Mac an OOM situation doesn't cause a bad user experience (desktop freezes) like on Linux.


I tested this by running:

1) Terminal 1
$ while true; d if ps x -o pid,rss,vsz,command | grep js[c]|grep -v python|grep -v ruby; then  top -l 1 -s 0 | grep PhysMem; echo ; fi;done


2) Terminal 2
$ Tools/Scripts/run-javascriptcore-tests --release --no-build --no-testapi --no-testmasm --no-testair --no-testb3  --no-mozilla-tests --no-quick  --filter js-api/test_memory_constructor.js


The attached patch here makes the situation much better on Linux, as all the 5 subtests that run with useWebAssemblyFastMemory=true now only need to allocate a few MBs (less than 20MB). On mac the patch makes no difference, is neutral there.

[Carlos Alberto Lopez Perez]

(In reply to Carlos Alberto Lopez Perez from comment #18)
> I tested this by running:
> 
> 1) Terminal 1
> $ while true; d if ps x -o pid,rss,vsz,command | grep js[c]|grep -v
> python|grep -v ruby; then  top -l 1 -s 0 | grep PhysMem; echo ; fi;done
> 

while true; do if ps x -o pid,rss,vsz,command | grep js[c]|grep -v python|grep -v ruby; then  top -l 1 -s 0 | grep PhysMem; echo ; fi; done

[Yusuke Suzuki]

This patch intends to fix OOM issue in Linux. So, the change is guarded by OS(LINUX). In the different OSes, no change is made by this patch.

[Carlos Alberto Lopez Perez]

(In reply to Yusuke Suzuki from comment #20)
> This patch intends to fix OOM issue in Linux. So, the change is guarded by
> OS(LINUX). In the different OSes, no change is made by this patch.

Right.

The patch is a really good improvement and I'll r+ it tomorrow if no further comments by Saam or someone else.

But I was wondering why using memset(0) here for Mac? Is just because on Mac you can trigger an OOM condition without having to fear to bring down or freeze the system? That don't seems right to me.

[Carlos Alberto Lopez Perez]

(In reply to Carlos Alberto Lopez Perez from comment #21)
> But I was wondering why using memset(0) here for Mac? Is just because on Mac
> you can trigger an OOM condition without having to fear to bring down or
> freeze the system? That don't seems right to me.

To be clear: this question is not related with this patch, but more general wondering about why the current code is doing this.

Using memset(0) is what is currently happening before any patch

[Carlos Alberto Lopez Perez]

(In reply to Carlos Alberto Lopez Perez from comment #22)
> (In reply to Carlos Alberto Lopez Perez from comment #21)
> > But I was wondering why using memset(0) here for Mac? Is just because on Mac
> > you can trigger an OOM condition without having to fear to bring down or
> > freeze the system? That don't seems right to me.
> 
> To be clear: this question is not related with this patch, but more general
> wondering about why the current code is doing this.
> 
> Using memset(0) is what is currently happening before any patch

A few git blames reveal this: https://trac.webkit.org/browser/trunk/Source/JavaScriptCore/wasm/WasmMemory.cpp?rev=219595#L69 which was then removed (the comment) on r220118 <https://trac.webkit.org/r220118>

So bug 170343 seems related.

[Michael Catanzaro]

(In reply to Yusuke Suzuki from comment #17)
> I keep memoryLimited annotation since this patch only solves the problem
> under fast memory.
> I don't think using madvise to non fast memory thing is not good solution.
> So, I keep the current one as is. But it is not a problem since Linux ports
> always use fast memory right now. And fast memory will not be disabled if
> WebKitGTK+ does not support WebKitLegacy.

Not sure if I understand this correctly, but: we do support using system malloc, and we have distributors that depend on it because bmalloc does not work on certain architectures.

[Yusuke Suzuki]

(In reply to Michael Catanzaro from comment #24)
> (In reply to Yusuke Suzuki from comment #17)
> > I keep memoryLimited annotation since this patch only solves the problem
> > under fast memory.
> > I don't think using madvise to non fast memory thing is not good solution.
> > So, I keep the current one as is. But it is not a problem since Linux ports
> > always use fast memory right now. And fast memory will not be disabled if
> > WebKitGTK+ does not support WebKitLegacy.
> 
> Not sure if I understand this correctly, but: we do support using system
> malloc, and we have distributors that depend on it because bmalloc does not
> work on certain architectures.

If we use system malloc, our Gigacage::tryAllocateVirtualPages just execute OSAllocator. And our commitZeroPages in this patch just works fine.
So, both is ok.

[Yusuke Suzuki]

(In reply to Carlos Alberto Lopez Perez from comment #22)
> (In reply to Carlos Alberto Lopez Perez from comment #21)
> > But I was wondering why using memset(0) here for Mac? Is just because on Mac
> > you can trigger an OOM condition without having to fear to bring down or
> > freeze the system? That don't seems right to me.
> 
> To be clear: this question is not related with this patch, but more general
> wondering about why the current code is doing this.
> 
> Using memset(0) is what is currently happening before any patch

As is described in bug 170343, changing the strategy based on required memory size would be promising.
But, as a first step, doing on-demand-paging in this patch and then deciding the threshold sounds better.

[Saam Barati]

Comment on attachment 317404 [details]
Patch

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

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

[Yusuke Suzuki]

Comment on attachment 317404 [details]
Patch

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

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

[Yusuke Suzuki]

Comment on attachment 317404 [details]
Patch

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

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

[Yusuke Suzuki]

Comment on attachment 317404 [details]
Patch

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

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

[Carlos Alberto Lopez Perez]

(In reply to Yusuke Suzuki from comment #30)
> Comment on attachment 317404 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=317404&action=review
> 
> >>>> 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_constructor.js?rev=220265#L44
and https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/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.

[Filip Pizlo]

Comment on attachment 317404 [details]
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_constructor.js?rev=220265#L44
> and https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/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.

[Saam Barati]

(In reply to Filip Pizlo from comment #32)
> Comment on attachment 317404 [details]
> 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_constructor.js?rev=220265#L44
> > and https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/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.

I don't think the test is stressing OOM as far as I remember. I'm ok with just setting it to @runDefault

[Saam Barati]

(In reply to Yusuke Suzuki from comment #28)
> Comment on attachment 317404 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=317404&action=review
> 
> >> 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.

This makes sense.

[Saam Barati]

I just reread the test again. It's asking for the largest possible amount of memory that is legal for a WebAssembly.memory. It seems reasonable to only run this once, or to revise the test to be less crazy.

[Saam Barati]

Comment on attachment 317404 [details]
Patch

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

> Source/JavaScriptCore/wasm/WasmMemory.cpp:269
> +    bool writable = true;
> +    bool executable = false;

Nit: these variables can be defined outside of the #if

[Yusuke Suzuki]

Comment on attachment 317404 [details]
Patch

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

>>>>>>>>> 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_constructor.js?rev=220265#L44
>>>> and https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/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.
>> 
>> I don't think the test is stressing OOM as far as I remember. I'm ok with just setting it to @runDefault
> 
> This makes sense.

OK, I filed the separate bug for runWebAssemblyDefault thing.

>> Source/JavaScriptCore/wasm/WasmMemory.cpp:269
>> +    bool executable = false;
> 
> Nit: these variables can be defined outside of the #if

Nice, fixed.

[Yusuke Suzuki]

Committed r220401: <http://trac.webkit.org/changeset/220401>