Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2653183003:
[wasm] Memory buffer should be detached after Memory.Grow (Closed)
Description[wasm] Memory buffer should be detached after Memory.Grow
Memory.Grow should detach the ArrayBuffer associated with the Mem object after Grow. Currently, when guard pages are enabled protection is changed to make more of the buffer accessible. This does not work for when the buffer should be detached after grow, because the memory object has a reference to the same buffer befor/after grow.
R=titzer@chromium.org, eholk@chromium.org
Review-Url: https://codereview.chromium.org/2653183003
Cr-Commit-Position: refs/heads/master@{#42717}
Committed: https://chromium.googlesource.com/v8/v8/+/e6d13999fd1bdb7ca403e157e6cafd05ce4def26
Patch Set 1 #
Total comments: 14
Patch Set 2 : Eric's review #Patch Set 3 : Ben's review #Patch Set 4 : Remove stray printf #Patch Set 5 : Enable js-api test for d=0 #Patch Set 6 : Fix #Patch Set 7 : Fix #
Total comments: 2
Patch Set 8 : Ben's review #
Messages
Total messages: 34 (23 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...
Tracking issue for Memory.Grow to use the same backing store and fixing when memory is detached - https://bugs.chromium.org/p/v8/issues/detail?id=5886
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2653183003/diff/1/src/wasm/wasm-js.cc File src/wasm/wasm-js.cc (right): https://codereview.chromium.org/2653183003/diff/1/src/wasm/wasm-js.cc#newcode736 src/wasm/wasm-js.cc:736: if (delta_size != 0) { There was a discussion about whether a delta_size of 0 should detach here: https://github.com/WebAssembly/design/issues/966 It's not resolved, but IMO it makes more intuitive sense to always detach if the operation doesn't fail. https://codereview.chromium.org/2653183003/diff/1/src/wasm/wasm-js.cc#newcode738 src/wasm/wasm-js.cc:738: const bool has_guard_regions = I think it also makes sense to move the detachment of the old bugger into the GrowWebAssemblyMemory operation, since it might get lucky and reuse the underlying memory, so it would detach the old buffer itself. https://codereview.chromium.org/2653183003/diff/1/src/wasm/wasm-module.cc File src/wasm/wasm-module.cc (right): https://codereview.chromium.org/2653183003/diff/1/src/wasm/wasm-module.cc#new... src/wasm/wasm-module.cc:2355: (!old_buffer.is_null() && old_buffer->has_guard_region()) ? true : false; You don't need to do the {x ? true : false}, you can just do {x}. https://codereview.chromium.org/2653183003/diff/1/src/wasm/wasm-module.cc#new... src/wasm/wasm-module.cc:2357: NewArrayBuffer(isolate, new_size, enable_guard_regions); I think should adjust the old logic here (just changing the protection) and not just allocate a new buffer every time.
https://codereview.chromium.org/2653183003/diff/1/src/wasm/wasm-js.cc File src/wasm/wasm-js.cc (right): https://codereview.chromium.org/2653183003/diff/1/src/wasm/wasm-js.cc#newcode740 src/wasm/wasm-js.cc:740: : false; nit: Is the ? operator needed here? `(!old_buffer.is_null() && old_buffer->has_guard_region())` is already a boolean. https://codereview.chromium.org/2653183003/diff/1/src/wasm/wasm-js.cc#newcode742 src/wasm/wasm-js.cc:742: void* backing_store = old_buffer->backing_store(); It might not hurt to add `DCHECK(backing_store != nullptr)`, just in case that was the actual issue we saw yesterday. https://codereview.chromium.org/2653183003/diff/1/src/wasm/wasm-module.cc File src/wasm/wasm-module.cc (right): https://codereview.chromium.org/2653183003/diff/1/src/wasm/wasm-module.cc#new... src/wasm/wasm-module.cc:65: void* memory = buffer->backing_store(); `DCHECK(memory != nullptr)` would probably be good here too (although hopefully the was_neutered check covers that).
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/2653183003/diff/1/src/wasm/wasm-js.cc File src/wasm/wasm-js.cc (right): https://codereview.chromium.org/2653183003/diff/1/src/wasm/wasm-js.cc#newcode740 src/wasm/wasm-js.cc:740: : false; On 2017/01/25 18:16:57, Eric Holk wrote: > nit: Is the ? operator needed here? `(!old_buffer.is_null() && > old_buffer->has_guard_region())` is already a boolean. Done. https://codereview.chromium.org/2653183003/diff/1/src/wasm/wasm-js.cc#newcode742 src/wasm/wasm-js.cc:742: void* backing_store = old_buffer->backing_store(); On 2017/01/25 18:16:57, Eric Holk wrote: > It might not hurt to add `DCHECK(backing_store != nullptr)`, just in case that > was the actual issue we saw yesterday. Done. https://codereview.chromium.org/2653183003/diff/1/src/wasm/wasm-module.cc File src/wasm/wasm-module.cc (right): https://codereview.chromium.org/2653183003/diff/1/src/wasm/wasm-module.cc#new... src/wasm/wasm-module.cc:65: void* memory = buffer->backing_store(); On 2017/01/25 18:16:57, Eric Holk wrote: > `DCHECK(memory != nullptr)` would probably be good here too (although hopefully > the was_neutered check covers that). Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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 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/2653183003/diff/1/src/wasm/wasm-js.cc File src/wasm/wasm-js.cc (right): https://codereview.chromium.org/2653183003/diff/1/src/wasm/wasm-js.cc#newcode736 src/wasm/wasm-js.cc:736: if (delta_size != 0) { On 2017/01/25 09:28:57, titzer wrote: > There was a discussion about whether a delta_size of 0 should detach here: > https://github.com/WebAssembly/design/issues/966 > > It's not resolved, but IMO it makes more intuitive sense to always detach if the > operation doesn't fail. I opened that issue because I was not able to get a conclusive answer as to which way that should work and figured it might be good to have it clarified in the spec, fixed to always detach. https://codereview.chromium.org/2653183003/diff/1/src/wasm/wasm-js.cc#newcode738 src/wasm/wasm-js.cc:738: const bool has_guard_regions = On 2017/01/25 09:28:56, titzer wrote: > I think it also makes sense to move the detachment of the old bugger into the > GrowWebAssemblyMemory operation, since it might get lucky and reuse the > underlying memory, so it would detach the old buffer itself. Done. https://codereview.chromium.org/2653183003/diff/1/src/wasm/wasm-module.cc File src/wasm/wasm-module.cc (right): https://codereview.chromium.org/2653183003/diff/1/src/wasm/wasm-module.cc#new... src/wasm/wasm-module.cc:2355: (!old_buffer.is_null() && old_buffer->has_guard_region()) ? true : false; On 2017/01/25 09:28:57, titzer wrote: > You don't need to do the {x ? true : false}, you can just do {x}. Done. https://codereview.chromium.org/2653183003/diff/1/src/wasm/wasm-module.cc#new... src/wasm/wasm-module.cc:2357: NewArrayBuffer(isolate, new_size, enable_guard_regions); On 2017/01/25 09:28:57, titzer wrote: > I think should adjust the old logic here (just changing the protection) and not > just allocate a new buffer every time. After offline discussions with Eric (summarized here - https://bugs.chromium.org/p/v8/issues/detail?id=5886), we decided that this is the safest/simplest way to fix it for now because additional book keeping is needed in the Memory finalizer for this to be safe. Would this happen automatically? If so, I can revert back to the previous logic.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_asan_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng/buil...) v8_linux64_asan_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng_trig...)
lgtm, as long as it also looks good to Ben.
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.
gdeepti@chromium.org changed reviewers: + bradnelson@chromium.org
+ bradnelson@ as Ben is in transit.
lgtm https://codereview.chromium.org/2653183003/diff/110001/src/wasm/wasm-module.cc File src/wasm/wasm-module.cc (right): https://codereview.chromium.org/2653183003/diff/110001/src/wasm/wasm-module.c... src/wasm/wasm-module.cc:2361: const bool enable_guard_regions = Can you please add a TODO here to reimplement the old functionality. It's only necessary to do that before guard regions get turned on, though.
https://codereview.chromium.org/2653183003/diff/110001/src/wasm/wasm-module.cc File src/wasm/wasm-module.cc (right): https://codereview.chromium.org/2653183003/diff/110001/src/wasm/wasm-module.c... src/wasm/wasm-module.cc:2361: const bool enable_guard_regions = On 2017/01/26 21:35:27, titzer wrote: > Can you please add a TODO here to reimplement the old functionality. It's only > necessary to do that before guard regions get turned on, though. Done.
The CQ bit was checked by gdeepti@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from eholk@chromium.org, titzer@chromium.org Link to the patchset: https://codereview.chromium.org/2653183003/#ps130001 (title: "Ben's review")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 130001, "attempt_start_ts": 1485467025943840,
"parent_rev": "373320aef92177fd1870825b47e8896df00efc52", "commit_rev":
"e6d13999fd1bdb7ca403e157e6cafd05ce4def26"}
Message was sent while issue was closed.
Description was changed from ========== [wasm] Memory buffer should be detached after Memory.Grow Memory.Grow should detach the ArrayBuffer associated with the Mem object after Grow. Currently, when guard pages are enabled protection is changed to make more of the buffer accessible. This does not work for when the buffer should be detached after grow, because the memory object has a reference to the same buffer befor/after grow. R=titzer@chromium.org, eholk@chromium.org ========== to ========== [wasm] Memory buffer should be detached after Memory.Grow Memory.Grow should detach the ArrayBuffer associated with the Mem object after Grow. Currently, when guard pages are enabled protection is changed to make more of the buffer accessible. This does not work for when the buffer should be detached after grow, because the memory object has a reference to the same buffer befor/after grow. R=titzer@chromium.org, eholk@chromium.org Review-Url: https://codereview.chromium.org/2653183003 Cr-Commit-Position: refs/heads/master@{#42717} Committed: https://chromium.googlesource.com/v8/v8/+/e6d13999fd1bdb7ca403e157e6cafd05ce4... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:130001) as https://chromium.googlesource.com/v8/v8/+/e6d13999fd1bdb7ca403e157e6cafd05ce4... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
