Add 2GB-4GB heap testing #10811
Add 2GB-4GB heap testing #10811
Conversation
tests/browser/test_2GB_fail.cpp
Outdated
| for (int i = 0; i < NUM_CHUNKS; i++) { | ||
| printf("alloc %d\n", i); | ||
| try { | ||
| chunks[i].resize(CHUNK_SIZE); |
There was a problem hiding this comment.
Why not just call emscripten_resize_heap directly with the large value?
There was a problem hiding this comment.
Of even you wan to go through the c library, is there any need to reproduce the NUM_CHUNKS/CHUNK_SIZE loop here? Why not just call malloc in a loop?
There was a problem hiding this comment.
Yeah, I just wanted to use normal libc stuff, to get testing close to what users do.
Not sure what you mean by "reproduce" in that sentence? This is basically calling malloc in a loop (just with nicer C++ I think)?
There was a problem hiding this comment.
Its just looks a lot more complicatled to me than a simple malloc loop. With this test you are also implicatly testing all kind of other things such as the C++ standard library. I know we have a lot of tests like this, but I always have a preference for keeping tests as narrow as possible.
You are also using C++ exceptions which makes that whole thing depend on way more emscripten infrastructure.
There was a problem hiding this comment.
I'm not saying you have to make this change, just explaining my initial reaction to seeing this code in this test case.
It's not easy to test this because (1) we need super-new V8, and
(2) this will allocate over 2GB during testing, which strains the bots'
memory limits and risks OOMs. But we should try to have some
testing at least.
This adds new tests in
browser. I think in the long term we wantto test this there anyhow, to make sure this works in actual
browsers and not just a shell environment. For now, we still need
to wait for the V8 update to reach chrome beta, so this doesn't
actually run in a browser, but I still put the test in
browser as that suite runs sequentially, which means
no other test will compete for memory with it, reducing OOM
risks. As we expect to convert this to a browser test eventually
this seems not that bad for now (however, if we want non-browser
2GB+ testing, we should probably add a
sequentialtest mode).The main new test,
test_4GB, allocates over 3GB and alsoverifies that we can read and write to that memory in both C
and JS.
This also found a minor bug: without 2GB+ heaps we did not
properly error on trying to allocate 2GB, the value actually
overflowed and we entered the wrong code path. The new
test_2GB_failverifies that is fixed (the fix is by always>>> 0the memory size, which also removes an ifdef,nicely).