Bug 175150

Created attachment 317140 [details] Patch

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)

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

<rdar://problem/33720817>

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.

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

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

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

(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

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

Reopening to attach new patch.

Created attachment 317342 [details] Patch

Created attachment 317375 [details] Patch

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)

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

Created attachment 317404 [details] Patch

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.

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

(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

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.

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

(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

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

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

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

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

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.

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.

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.

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

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

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.

(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

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

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.

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

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.

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