Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2396473003:
[wasm] Refactor GrowMemory runtime call. (Closed)
Description[wasm] Refactor GrowMemory runtime call.
Refactor to move module specific functionality to wasm-module.cc, this provides a better interface for the grow() method on WebAssembly.memory objects.
R=mtrofin@chromium.org, titzer@chromium.org
Committed: https://crrev.com/096b5f649b6a5d93ac4ab1d5f8bef95468e41089
Cr-Commit-Position: refs/heads/master@{#39967}
Patch Set 1 #Patch Set 2 : Rebase #Patch Set 3 : Rebase again for weird trybot failures #
Total comments: 18
Patch Set 4 : Mircea's review #Patch Set 5 : Fix header #
Messages
Total messages: 26 (19 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: Try jobs failed on following builders: v8_linux_mips64el_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...)
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: Try jobs failed on following builders: v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/13920)
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.
lgtm, after comments are addressed. https://codereview.chromium.org/2396473003/diff/40001/src/wasm/wasm-module.cc File src/wasm/wasm-module.cc (right): https://codereview.chromium.org/2396473003/diff/40001/src/wasm/wasm-module.cc... src/wasm/wasm-module.cc:1761: Address old_mem_start, new_mem_start; could you please initialize all these and the uints below? https://codereview.chromium.org/2396473003/diff/40001/src/wasm/wasm-module.cc... src/wasm/wasm-module.cc:1779: if (new_mem_start == NULL) { nullptr? https://codereview.chromium.org/2396473003/diff/40001/src/wasm/wasm-module.cc... src/wasm/wasm-module.cc:1783: // Double check the API allocator actually zero-initialized the memory. could you encapsulate this and then reuse it below, too? e.g. void ValidateAllocatorInitializedMemory(blah blah) { #if DEBUG ... #endif } https://codereview.chromium.org/2396473003/diff/40001/src/wasm/wasm-module.cc... src/wasm/wasm-module.cc:1796: new_size = old_size + pages * wasm::WasmModule::kPageSize; In addition to the check below, we should check that std::numeric_limits<uint32_t> - old_size >= pages*wasm::WasmModule::kPageSize. https://codereview.chromium.org/2396473003/diff/40001/src/wasm/wasm-module.cc... src/wasm/wasm-module.cc:1803: static_cast<uint32_t>(new_size))); this static_cast is unnecessary, new_size is uint32_t already https://codereview.chromium.org/2396473003/diff/40001/src/wasm/wasm-module.cc... src/wasm/wasm-module.cc:1804: if (new_mem_start == NULL) { nullptr https://codereview.chromium.org/2396473003/diff/40001/src/wasm/wasm-module.cc... src/wasm/wasm-module.cc:1813: memcpy(new_mem_start, old_mem_start, old_size); we should check earlier that old_size <= new_size, otherwise this memcpy will try copying more than it should. https://codereview.chromium.org/2396473003/diff/40001/src/wasm/wasm-module.cc... src/wasm/wasm-module.cc:1823: return (old_size / WasmModule::kPageSize); A DCHECK here that old_size % WasmModule::kPageSize == 0? https://codereview.chromium.org/2396473003/diff/40001/src/wasm/wasm-module.h File src/wasm/wasm-module.h (right): https://codereview.chromium.org/2396473003/diff/40001/src/wasm/wasm-module.h#... src/wasm/wasm-module.h:517: void SetInstanceMemory(Handle<JSObject> instance, JSArrayBuffer* buffer); We can delete SetInstanceMemory, I think. It was there just for GrowMemory.
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/2396473003/diff/40001/src/wasm/wasm-module.cc File src/wasm/wasm-module.cc (right): https://codereview.chromium.org/2396473003/diff/40001/src/wasm/wasm-module.cc... src/wasm/wasm-module.cc:1761: Address old_mem_start, new_mem_start; On 2016/10/04 22:01:53, Mircea Trofin wrote: > could you please initialize all these and the uints below? Done. https://codereview.chromium.org/2396473003/diff/40001/src/wasm/wasm-module.cc... src/wasm/wasm-module.cc:1779: if (new_mem_start == NULL) { On 2016/10/04 22:01:53, Mircea Trofin wrote: > nullptr? Used NewArrayBuffer method instead of allocating, that takes care of this check. https://codereview.chromium.org/2396473003/diff/40001/src/wasm/wasm-module.cc... src/wasm/wasm-module.cc:1783: // Double check the API allocator actually zero-initialized the memory. On 2016/10/04 22:01:53, Mircea Trofin wrote: > could you encapsulate this and then reuse it below, too? e.g. > > void ValidateAllocatorInitializedMemory(blah blah) { > #if DEBUG > ... > #endif > } Used NewArrayBuffer method instead which sets up a new array buffer and validates that memory is initialized. https://codereview.chromium.org/2396473003/diff/40001/src/wasm/wasm-module.cc... src/wasm/wasm-module.cc:1796: new_size = old_size + pages * wasm::WasmModule::kPageSize; On 2016/10/04 22:01:53, Mircea Trofin wrote: > In addition to the check below, we should check that > std::numeric_limits<uint32_t> - old_size >= pages*wasm::WasmModule::kPageSize. Done. https://codereview.chromium.org/2396473003/diff/40001/src/wasm/wasm-module.cc... src/wasm/wasm-module.cc:1803: static_cast<uint32_t>(new_size))); On 2016/10/04 22:01:53, Mircea Trofin wrote: > this static_cast is unnecessary, new_size is uint32_t already Done. https://codereview.chromium.org/2396473003/diff/40001/src/wasm/wasm-module.cc... src/wasm/wasm-module.cc:1804: if (new_mem_start == NULL) { On 2016/10/04 22:01:53, Mircea Trofin wrote: > nullptr Used NewArrayBuffer method instead of allocating, that takes care of this check. https://codereview.chromium.org/2396473003/diff/40001/src/wasm/wasm-module.cc... src/wasm/wasm-module.cc:1813: memcpy(new_mem_start, old_mem_start, old_size); On 2016/10/04 22:01:53, Mircea Trofin wrote: > we should check earlier that old_size <= new_size, otherwise this memcpy will > try copying more than it should. Done. https://codereview.chromium.org/2396473003/diff/40001/src/wasm/wasm-module.cc... src/wasm/wasm-module.cc:1823: return (old_size / WasmModule::kPageSize); On 2016/10/04 22:01:53, Mircea Trofin wrote: > A DCHECK here that old_size % WasmModule::kPageSize == 0? Done. https://codereview.chromium.org/2396473003/diff/40001/src/wasm/wasm-module.h File src/wasm/wasm-module.h (right): https://codereview.chromium.org/2396473003/diff/40001/src/wasm/wasm-module.h#... src/wasm/wasm-module.h:517: void SetInstanceMemory(Handle<JSObject> instance, JSArrayBuffer* buffer); On 2016/10/04 22:01:53, Mircea Trofin wrote: > We can delete SetInstanceMemory, I think. It was there just for > GrowMemory. Removed from header, retaining in wasm-module.cc for modularity.
The CQ bit was checked by gdeepti@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mtrofin@chromium.org Link to the patchset: https://codereview.chromium.org/2396473003/#ps80001 (title: "Fix header")
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.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== [wasm] Refactor GrowMemory runtime call. Refactor to move module specific functionality to wasm-module.cc, this provides a better interface for the grow() method on WebAssembly.memory objects. R=mtrofin@chromium.org, titzer@chromium.org ========== to ========== [wasm] Refactor GrowMemory runtime call. Refactor to move module specific functionality to wasm-module.cc, this provides a better interface for the grow() method on WebAssembly.memory objects. R=mtrofin@chromium.org, titzer@chromium.org Committed: https://crrev.com/096b5f649b6a5d93ac4ab1d5f8bef95468e41089 Cr-Commit-Position: refs/heads/master@{#39967} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/096b5f649b6a5d93ac4ab1d5f8bef95468e41089 Cr-Commit-Position: refs/heads/master@{#39967} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
