Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2319983002:
[wasm] GrowMemory should use array_buffer_allocator instead of realloc. (Closed)
Description[wasm] GrowMemory should use array_buffer_allocator instead of realloc.
- Using realloc is still unsafe as the allocator, using array_buffer_allocator
- Fixing tests to avoid overlapping stores, adding more tests
BUG=v8:5344
R=ahaas@chromium.org, mlippautz@chromium.org
Committed: https://crrev.com/2a4b5933b8c0dc73a086bc1ffa5d3de3581f6808
Cr-Commit-Position: refs/heads/master@{#39329}
Patch Set 1 #
Total comments: 11
Patch Set 2 : Review changes #
Messages
Total messages: 20 (12 generated)
The CQ bit was checked by gdeepti@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2319983002/diff/1/src/runtime/runtime-wasm.cc File src/runtime/runtime-wasm.cc (right): https://codereview.chromium.org/2319983002/diff/1/src/runtime/runtime-wasm.cc... src/runtime/runtime-wasm.cc:105: isolate->heap()->UnregisterArrayBuffer(*old_buffer); Is this the right way to deal with the old buffer? My understanding is unregistering the old_buffer will get the garbage collector to free the memory.
Description was changed from ========== [wasm] GrowMemory should use array_buffer_allocator instead of realloc. - Using realloc is still unsafe as the allocator, using array_buffer_allocator - Fixing tests to avoid overlapping stores, adding more tests BUG=chromium:5344 R=ahaas@chromium.org, mlippautz@chromium.org ========== to ========== [wasm] GrowMemory should use array_buffer_allocator instead of realloc. - Using realloc is still unsafe as the allocator, using array_buffer_allocator - Fixing tests to avoid overlapping stores, adding more tests BUG=v8:5344 R=ahaas@chromium.org, mlippautz@chromium.org ==========
Only looked at runtime/runtime-wasm.cc Deferring to Andreas for the full review. Thanks for fixing these issues! https://codereview.chromium.org/2319983002/diff/1/src/runtime/runtime-wasm.cc File src/runtime/runtime-wasm.cc (right): https://codereview.chromium.org/2319983002/diff/1/src/runtime/runtime-wasm.cc... src/runtime/runtime-wasm.cc:105: isolate->heap()->UnregisterArrayBuffer(*old_buffer); On 2016/09/08 06:04:47, gdeepti wrote: > Is this the right way to deal with the old buffer? My understanding is > unregistering the old_buffer will get the garbage collector to free the memory. UnregisterArrayBuffer will make the GC stop tracking the backing store of this JSArrayBuffer. So it's not that the buffer is freed but rather ownership is transferred from the GC to the embedder. The prime example is graphics code that works with JSArrayBuffer until a certain point where they want to send it to the GPU without copy, so they unregister the buffer, effectively taking over ownership (from the GC). For wasm you have 2 choices: - Don't externalize and UnregisterArrayBuffer (get rid of line 104 and 105). The GC will collect the backing store for you in this case because you didn't specify it as external on line 109. - Add a array_buffer_allocator()->Free() after UnregisterArrayBuffer because wasm is now the owner of the backing store.
https://codereview.chromium.org/2319983002/diff/1/src/runtime/runtime-wasm.cc File src/runtime/runtime-wasm.cc (right): https://codereview.chromium.org/2319983002/diff/1/src/runtime/runtime-wasm.cc... src/runtime/runtime-wasm.cc:105: isolate->heap()->UnregisterArrayBuffer(*old_buffer); On 2016/09/08 at 09:43:31, Michael Lippautz wrote: > On 2016/09/08 06:04:47, gdeepti wrote: > > Is this the right way to deal with the old buffer? My understanding is > > unregistering the old_buffer will get the garbage collector to free the memory. > > UnregisterArrayBuffer will make the GC stop tracking the backing store of this JSArrayBuffer. So it's not that the buffer is freed but rather ownership is transferred from the GC to the embedder. > > The prime example is graphics code that works with JSArrayBuffer until a certain point where they want to send it to the GPU without copy, so they unregister the buffer, effectively taking over ownership (from the GC). > > For wasm you have 2 choices: > - Don't externalize and UnregisterArrayBuffer (get rid of line 104 and 105). The GC will collect the backing store for you in this case because you didn't specify it as external on line 109. > - Add a array_buffer_allocator()->Free() after UnregisterArrayBuffer because wasm is now the owner of the backing store. Hi Deepti, I talked with Ben about it, and we think that it would be best to go for option 1, "don't externalize and UnregisterArrayBuffer". https://codereview.chromium.org/2319983002/diff/1/test/mjsunit/wasm/grow-memo... File test/mjsunit/wasm/grow-memory.js (right): https://codereview.chromium.org/2319983002/diff/1/test/mjsunit/wasm/grow-memo... test/mjsunit/wasm/grow-memory.js:35: for(offset = 0; offset <= (kPageSize - 4); offset+=4) { For this test it is actually better to write to every offset. Writing to every offset is only bad if you first do all the writing and then do all the reading. If you alternate reading and writing, then you can write at every offset. https://codereview.chromium.org/2319983002/diff/1/test/mjsunit/wasm/grow-memo... test/mjsunit/wasm/grow-memory.js:46: for (offset = kPageSize; offset <= 4*kPageSize -4; offset+=4) { same here. https://codereview.chromium.org/2319983002/diff/1/test/mjsunit/wasm/grow-memo... test/mjsunit/wasm/grow-memory.js:57: for (offset = 4*kPageSize - 3; offset <= 4*kPageSize + 4; offset+=4) { same here. https://codereview.chromium.org/2319983002/diff/1/test/mjsunit/wasm/grow-memo... test/mjsunit/wasm/grow-memory.js:61: for (offset = 19*kPageSize - 10; offset <= 19*kPageSize - 4; offset+=4) { same here. https://codereview.chromium.org/2319983002/diff/1/test/mjsunit/wasm/grow-memo... test/mjsunit/wasm/grow-memory.js:86: for(offset = 0; offset <= kPageSize - 4; offset+=4) { same here.
The CQ bit was checked by gdeepti@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2319983002/diff/1/src/runtime/runtime-wasm.cc File src/runtime/runtime-wasm.cc (right): https://codereview.chromium.org/2319983002/diff/1/src/runtime/runtime-wasm.cc... src/runtime/runtime-wasm.cc:105: isolate->heap()->UnregisterArrayBuffer(*old_buffer); On 2016/09/08 09:43:31, Michael Lippautz (OOO Sep 25) wrote: > On 2016/09/08 06:04:47, gdeepti wrote: > > Is this the right way to deal with the old buffer? My understanding is > > unregistering the old_buffer will get the garbage collector to free the > memory. > > UnregisterArrayBuffer will make the GC stop tracking the backing store of this > JSArrayBuffer. So it's not that the buffer is freed but rather ownership is > transferred from the GC to the embedder. > > The prime example is graphics code that works with JSArrayBuffer until a certain > point where they want to send it to the GPU without copy, so they unregister the > buffer, effectively taking over ownership (from the GC). > > For wasm you have 2 choices: > - Don't externalize and UnregisterArrayBuffer (get rid of line 104 and 105). The > GC will collect the backing store for you in this case because you didn't > specify it as external on line 109. > - Add a array_buffer_allocator()->Free() after UnregisterArrayBuffer because > wasm is now the owner of the backing store. Thanks for explaining that, I've removed the code that externalizes and uses UnregisterArrayBuffer. https://codereview.chromium.org/2319983002/diff/1/test/mjsunit/wasm/grow-memo... File test/mjsunit/wasm/grow-memory.js (right): https://codereview.chromium.org/2319983002/diff/1/test/mjsunit/wasm/grow-memo... test/mjsunit/wasm/grow-memory.js:35: for(offset = 0; offset <= (kPageSize - 4); offset+=4) { On 2016/09/08 12:23:34, ahaas wrote: > For this test it is actually better to write to every offset. Writing to every > offset is only bad if you first do all the writing and then do all the reading. > If you alternate reading and writing, then you can write at every offset. It is better to test at every offset, but introducing overlapping stores feels non intuitive. And also realized that bounds check should be checked for different load sizes. Added 8/16/32 bit read write tests so that all the read write operations and bounds check for these are tested. Not testing 64 bit in this file because ToJS doesn't allow us to export 64 bit values. Will follow up with grow_memory cctest suite with the next CL that fixes bounds check for 0 initial memory. This addresses the comments below as well. https://codereview.chromium.org/2319983002/diff/1/test/mjsunit/wasm/grow-memo... test/mjsunit/wasm/grow-memory.js:86: for(offset = 0; offset <= kPageSize - 4; offset+=4) { On 2016/09/08 12:23:34, ahaas wrote: > same here. Changing this back to the way it was for now.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/09/08 at 22:44:51, gdeepti wrote: > https://codereview.chromium.org/2319983002/diff/1/src/runtime/runtime-wasm.cc > File src/runtime/runtime-wasm.cc (right): > > https://codereview.chromium.org/2319983002/diff/1/src/runtime/runtime-wasm.cc... > src/runtime/runtime-wasm.cc:105: isolate->heap()->UnregisterArrayBuffer(*old_buffer); > On 2016/09/08 09:43:31, Michael Lippautz (OOO Sep 25) wrote: > > On 2016/09/08 06:04:47, gdeepti wrote: > > > Is this the right way to deal with the old buffer? My understanding is > > > unregistering the old_buffer will get the garbage collector to free the > > memory. > > > > UnregisterArrayBuffer will make the GC stop tracking the backing store of this > > JSArrayBuffer. So it's not that the buffer is freed but rather ownership is > > transferred from the GC to the embedder. > > > > The prime example is graphics code that works with JSArrayBuffer until a certain > > point where they want to send it to the GPU without copy, so they unregister the > > buffer, effectively taking over ownership (from the GC). > > > > For wasm you have 2 choices: > > - Don't externalize and UnregisterArrayBuffer (get rid of line 104 and 105). The > > GC will collect the backing store for you in this case because you didn't > > specify it as external on line 109. > > - Add a array_buffer_allocator()->Free() after UnregisterArrayBuffer because > > wasm is now the owner of the backing store. > > Thanks for explaining that, I've removed the code that externalizes and uses UnregisterArrayBuffer. > > https://codereview.chromium.org/2319983002/diff/1/test/mjsunit/wasm/grow-memo... > File test/mjsunit/wasm/grow-memory.js (right): > > https://codereview.chromium.org/2319983002/diff/1/test/mjsunit/wasm/grow-memo... > test/mjsunit/wasm/grow-memory.js:35: for(offset = 0; offset <= (kPageSize - 4); offset+=4) { > On 2016/09/08 12:23:34, ahaas wrote: > > For this test it is actually better to write to every offset. Writing to every > > offset is only bad if you first do all the writing and then do all the reading. > > If you alternate reading and writing, then you can write at every offset. > > It is better to test at every offset, but introducing overlapping stores feels non intuitive. And also realized that bounds check should be checked for different load sizes. Added 8/16/32 bit read write tests so that all the read write operations and bounds check for these are tested. Not testing 64 bit in this file because ToJS doesn't allow us to export 64 bit values. Will follow up with grow_memory cctest suite with the next CL that fixes bounds check for 0 initial memory. This addresses the comments below as well. > > https://codereview.chromium.org/2319983002/diff/1/test/mjsunit/wasm/grow-memo... > test/mjsunit/wasm/grow-memory.js:86: for(offset = 0; offset <= kPageSize - 4; offset+=4) { > On 2016/09/08 12:23:34, ahaas wrote: > > same here. > > Changing this back to the way it was for now. LGTM. Can you please add tests for 64 bit loads and store with float64 values in a separate CL?
The CQ bit was checked by ahaas@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [wasm] GrowMemory should use array_buffer_allocator instead of realloc. - Using realloc is still unsafe as the allocator, using array_buffer_allocator - Fixing tests to avoid overlapping stores, adding more tests BUG=v8:5344 R=ahaas@chromium.org, mlippautz@chromium.org ========== to ========== [wasm] GrowMemory should use array_buffer_allocator instead of realloc. - Using realloc is still unsafe as the allocator, using array_buffer_allocator - Fixing tests to avoid overlapping stores, adding more tests BUG=v8:5344 R=ahaas@chromium.org, mlippautz@chromium.org ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== [wasm] GrowMemory should use array_buffer_allocator instead of realloc. - Using realloc is still unsafe as the allocator, using array_buffer_allocator - Fixing tests to avoid overlapping stores, adding more tests BUG=v8:5344 R=ahaas@chromium.org, mlippautz@chromium.org ========== to ========== [wasm] GrowMemory should use array_buffer_allocator instead of realloc. - Using realloc is still unsafe as the allocator, using array_buffer_allocator - Fixing tests to avoid overlapping stores, adding more tests BUG=v8:5344 R=ahaas@chromium.org, mlippautz@chromium.org Committed: https://crrev.com/2a4b5933b8c0dc73a086bc1ffa5d3de3581f6808 Cr-Commit-Position: refs/heads/master@{#39329} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/2a4b5933b8c0dc73a086bc1ffa5d3de3581f6808 Cr-Commit-Position: refs/heads/master@{#39329} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
