Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2410763002:
[wasm] GrowMemory should use maximum size declared in WebAssembly.Memory (Closed)
Description[wasm] GrowMemory should use maximum size declared in WebAssembly.Memory
BUG=
Committed: https://crrev.com/0c6354e03b84f41322edf45b81dc4ddb0394b187
Cr-Commit-Position: refs/heads/master@{#40411}
Patch Set 1 #Patch Set 2 : Separate refactoring, add test #
Total comments: 2
Patch Set 3 : Implement Memory.Grow, tests #Patch Set 4 : Fix test #Patch Set 5 : Fix error #Patch Set 6 : Formatting #
Total comments: 22
Patch Set 7 : Brad's review #Patch Set 8 : Format #
Total comments: 6
Patch Set 9 : Fix trybots, Brad's review #
Messages
Total messages: 46 (35 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...
titzer@chromium.org changed reviewers: + titzer@chromium.org
Looks like this is headed in the right direction. https://codereview.chromium.org/2410763002/diff/20001/src/wasm/wasm-module.cc File src/wasm/wasm-module.cc (right): https://codereview.chromium.org/2410763002/diff/20001/src/wasm/wasm-module.cc... src/wasm/wasm-module.cc:105: enum WasmMemoryObjectData { I am wondering if it is possible to hide these in wasm-js.cc instead. Not sure if that will turn out to be simpler or not.
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 unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_avx2_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...) v8_linux64_avx2_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng_trig...)
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_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...) v8_linux64_gyp_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng_trigg...)
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.
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...
gdeepti@chromium.org changed reviewers: + bradnelson@chromium.org
https://codereview.chromium.org/2410763002/diff/20001/src/wasm/wasm-module.cc File src/wasm/wasm-module.cc (right): https://codereview.chromium.org/2410763002/diff/20001/src/wasm/wasm-module.cc... src/wasm/wasm-module.cc:105: enum WasmMemoryObjectData { On 2016/10/11 08:30:06, titzer wrote: > I am wondering if it is possible to hide these in wasm-js.cc instead. Not sure > if that will turn out to be simpler or not. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Message was sent while issue was closed.
https://codereview.chromium.org/2410763002/diff/100001/src/wasm/wasm-js.cc File src/wasm/wasm-js.cc (right): https://codereview.chromium.org/2410763002/diff/100001/src/wasm/wasm-js.cc#ne... src/wasm/wasm-js.cc:593: i::Handle<i::Object> instance_object(receiver->GetInternalField(2), where's this 2 come from? https://codereview.chromium.org/2410763002/diff/100001/src/wasm/wasm-js.cc#ne... src/wasm/wasm-js.cc:601: if (ret == -1) { unsigned, compared to -1 ? https://codereview.chromium.org/2410763002/diff/100001/src/wasm/wasm-js.cc#ne... src/wasm/wasm-js.cc:781: i::JS_OBJECT_TYPE, i::JSObject::kHeaderSize + 3 * i::kPointerSize); Add a size enum item above so this can have a name. https://codereview.chromium.org/2410763002/diff/100001/src/wasm/wasm-module.cc File src/wasm/wasm-module.cc (right): https://codereview.chromium.org/2410763002/diff/100001/src/wasm/wasm-module.c... src/wasm/wasm-module.cc:1433: static const int kWasmMemoryInstanceIndex = 2; Shouldn't this live in wasm-module.h ? https://codereview.chromium.org/2410763002/diff/100001/test/mjsunit/wasm/impo... File test/mjsunit/wasm/import-memory.js (right): https://codereview.chromium.org/2410763002/diff/100001/test/mjsunit/wasm/impo... test/mjsunit/wasm/import-memory.js:135: for (offset = (i-1)*kPageSize; offset < i*kPageSize - 3; offset+=4) { Spacing on these operators is weird. https://google.github.io/styleguide/javascriptguide.xml https://codereview.chromium.org/2410763002/diff/100001/test/mjsunit/wasm/wasm... File test/mjsunit/wasm/wasm-constants.js (right): https://codereview.chromium.org/2410763002/diff/100001/test/mjsunit/wasm/wasm... test/mjsunit/wasm/wasm-constants.js:378: try { Intent is weird here.
Message was sent while issue was closed.
Description was changed from ========== [wasm] GrowMemory should use maximum size declared in WebAssembly.Memory BUG= ========== to ========== [wasm] GrowMemory should use maximum size declared in WebAssembly.Memory BUG= ==========
https://codereview.chromium.org/2410763002/diff/100001/src/wasm/wasm-js.cc File src/wasm/wasm-js.cc (right): https://codereview.chromium.org/2410763002/diff/100001/src/wasm/wasm-js.cc#ne... src/wasm/wasm-js.cc:589: uint32_t delta = args[0]->Uint32Value(context).FromJust(); Still not groking from before. If you use ToUint32(&delta) above you can get this value in one step? https://codereview.chromium.org/2410763002/diff/100001/src/wasm/wasm-module.cc File src/wasm/wasm-module.cc (right): https://codereview.chromium.org/2410763002/diff/100001/src/wasm/wasm-module.c... src/wasm/wasm-module.cc:2234: if (pages == 0) return GetInstanceMemorySize(isolate, instance); Move max_pages = ... to above and reuse max_pages here? https://codereview.chromium.org/2410763002/diff/100001/test/mjsunit/wasm/impo... File test/mjsunit/wasm/import-memory.js (right): https://codereview.chromium.org/2410763002/diff/100001/test/mjsunit/wasm/impo... test/mjsunit/wasm/import-memory.js:185: memory.grow(16381); Use assertThrows here? https://codereview.chromium.org/2410763002/diff/100001/test/mjsunit/wasm/impo... test/mjsunit/wasm/import-memory.js:189: })(); Add tests of: * Memory initial smaller than module imported memory initial. * Memory max larger than module max size (dynamic). https://codereview.chromium.org/2410763002/diff/100001/test/mjsunit/wasm/wasm... File test/mjsunit/wasm/wasm-constants.js (right): https://codereview.chromium.org/2410763002/diff/100001/test/mjsunit/wasm/wasm... test/mjsunit/wasm/wasm-constants.js:378: try { On 2016/10/17 22:13:25, bradnelson wrote: > Intent is weird here. Indent I mean
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/2410763002/diff/100001/src/wasm/wasm-js.cc File src/wasm/wasm-js.cc (right): https://codereview.chromium.org/2410763002/diff/100001/src/wasm/wasm-js.cc#ne... src/wasm/wasm-js.cc:589: uint32_t delta = args[0]->Uint32Value(context).FromJust(); On 2016/10/17 22:23:26, bradnelson wrote: > Still not groking from before. > If you use ToUint32(&delta) above you can get this value in one step? ToUint32(&delta) method can be used only on internal::Object, v8::Value is used here so using the method on value to obtain the delta value - looks like the Uint32Value(context).FromJust() performs the required checks so removing !args[0]->IsUint32()above. https://codereview.chromium.org/2410763002/diff/100001/src/wasm/wasm-js.cc#ne... src/wasm/wasm-js.cc:593: i::Handle<i::Object> instance_object(receiver->GetInternalField(2), On 2016/10/17 22:13:24, bradnelson wrote: > where's this 2 come from? From the internal field of the memory object, replaced with a named enum item. https://codereview.chromium.org/2410763002/diff/100001/src/wasm/wasm-js.cc#ne... src/wasm/wasm-js.cc:601: if (ret == -1) { On 2016/10/17 22:13:25, bradnelson wrote: > unsigned, compared to -1 ? Oops, fixed. For the tests I currently have, the check below throws the runtime error instead of the one here. https://codereview.chromium.org/2410763002/diff/100001/src/wasm/wasm-js.cc#ne... src/wasm/wasm-js.cc:781: i::JS_OBJECT_TYPE, i::JSObject::kHeaderSize + 3 * i::kPointerSize); On 2016/10/17 22:13:24, bradnelson wrote: > Add a size enum item above so this can have a name. Done. https://codereview.chromium.org/2410763002/diff/100001/src/wasm/wasm-module.cc File src/wasm/wasm-module.cc (right): https://codereview.chromium.org/2410763002/diff/100001/src/wasm/wasm-module.c... src/wasm/wasm-module.cc:1433: static const int kWasmMemoryInstanceIndex = 2; On 2016/10/17 22:13:25, bradnelson wrote: > Shouldn't this live in wasm-module.h ? Refactored this and GetMaxInstanceMemorySize so that internal fields on the memory objects have getters/setters as required in wasm-js.cc instead of defining const values in this file. This should not live in wasm-module.h because the memory object should be internal to the JS interface. https://codereview.chromium.org/2410763002/diff/100001/src/wasm/wasm-module.c... src/wasm/wasm-module.cc:2234: if (pages == 0) return GetInstanceMemorySize(isolate, instance); GetInstanceMemorySize, GetMaxInstanceMemorySize are different functions, cannot reuse max_pages here. https://codereview.chromium.org/2410763002/diff/100001/test/mjsunit/wasm/impo... File test/mjsunit/wasm/import-memory.js (right): https://codereview.chromium.org/2410763002/diff/100001/test/mjsunit/wasm/impo... test/mjsunit/wasm/import-memory.js:135: for (offset = (i-1)*kPageSize; offset < i*kPageSize - 3; offset+=4) { On 2016/10/17 22:13:25, bradnelson wrote: > Spacing on these operators is weird. > https://google.github.io/styleguide/javascriptguide.xml Done. https://codereview.chromium.org/2410763002/diff/100001/test/mjsunit/wasm/impo... test/mjsunit/wasm/import-memory.js:185: memory.grow(16381); On 2016/10/17 22:23:26, bradnelson wrote: > Use assertThrows here? Done. https://codereview.chromium.org/2410763002/diff/100001/test/mjsunit/wasm/impo... test/mjsunit/wasm/import-memory.js:189: })(); On 2016/10/17 22:23:26, bradnelson wrote: > Add tests of: > * Memory initial smaller than module imported memory initial. > * Memory max larger than module max size (dynamic). Deferring this to another change because as per my understanding this isn't completely implemented just yet. https://codereview.chromium.org/2410763002/diff/100001/test/mjsunit/wasm/wasm... File test/mjsunit/wasm/wasm-constants.js (right): https://codereview.chromium.org/2410763002/diff/100001/test/mjsunit/wasm/wasm... test/mjsunit/wasm/wasm-constants.js:378: try { On 2016/10/17 22:23:26, bradnelson wrote: > On 2016/10/17 22:13:25, bradnelson wrote: > > Intent is weird here. > > Indent I mean Removed this function because assertThrows covers all use cases of this.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/10987) v8_mac_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng_triggered/bui...)
lgtm https://codereview.chromium.org/2410763002/diff/100001/src/wasm/wasm-js.cc File src/wasm/wasm-js.cc (right): https://codereview.chromium.org/2410763002/diff/100001/src/wasm/wasm-js.cc#ne... src/wasm/wasm-js.cc:589: uint32_t delta = args[0]->Uint32Value(context).FromJust(); On 2016/10/18 02:34:17, gdeepti wrote: > On 2016/10/17 22:23:26, bradnelson wrote: > > Still not groking from before. > > If you use ToUint32(&delta) above you can get this value in one step? > > ToUint32(&delta) method can be used only on internal::Object, v8::Value is used > here so using the method on value to obtain the delta value - looks like the > Uint32Value(context).FromJust() performs the required checks so removing > !args[0]->IsUint32()above. Ah ok. https://codereview.chromium.org/2410763002/diff/140001/test/mjsunit/wasm/impo... File test/mjsunit/wasm/import-memory.js (right): https://codereview.chromium.org/2410763002/diff/140001/test/mjsunit/wasm/impo... test/mjsunit/wasm/import-memory.js:118: builder.addImportedMemory("mine"); Maybe make the module minimum explicit? I keep forgetting if it's 1 or 0 by default? https://codereview.chromium.org/2410763002/diff/140001/test/mjsunit/wasm/impo... test/mjsunit/wasm/import-memory.js:152: assertThrows("memory.grow(1)"); Indent is weird. Maybe do as a closure calling a function? https://codereview.chromium.org/2410763002/diff/140001/test/mjsunit/wasm/impo... test/mjsunit/wasm/import-memory.js:185: assertThrows("memory.grow(16381)"); Maybe do as a closure calling a function? Be consistent on newlines with above.
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.
The CQ bit was checked by gdeepti@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bradnelson@chromium.org Link to the patchset: https://codereview.chromium.org/2410763002/#ps160001 (title: "Fix trybots, Brad's review")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2410763002/diff/140001/test/mjsunit/wasm/impo... File test/mjsunit/wasm/import-memory.js (right): https://codereview.chromium.org/2410763002/diff/140001/test/mjsunit/wasm/impo... test/mjsunit/wasm/import-memory.js:118: builder.addImportedMemory("mine"); On 2016/10/18 04:29:38, bradnelson wrote: > Maybe make the module minimum explicit? I keep forgetting if it's 1 or 0 by > default? Done. https://codereview.chromium.org/2410763002/diff/140001/test/mjsunit/wasm/impo... test/mjsunit/wasm/import-memory.js:152: assertThrows("memory.grow(1)"); On 2016/10/18 04:29:38, bradnelson wrote: > Indent is weird. > Maybe do as a closure calling a function? Done. https://codereview.chromium.org/2410763002/diff/140001/test/mjsunit/wasm/impo... test/mjsunit/wasm/import-memory.js:185: assertThrows("memory.grow(16381)"); On 2016/10/18 04:29:38, bradnelson wrote: > Maybe do as a closure calling a function? > Be consistent on newlines with above. Done.
Message was sent while issue was closed.
Description was changed from ========== [wasm] GrowMemory should use maximum size declared in WebAssembly.Memory BUG= ========== to ========== [wasm] GrowMemory should use maximum size declared in WebAssembly.Memory BUG= ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== [wasm] GrowMemory should use maximum size declared in WebAssembly.Memory BUG= ========== to ========== [wasm] GrowMemory should use maximum size declared in WebAssembly.Memory BUG= Committed: https://crrev.com/0c6354e03b84f41322edf45b81dc4ddb0394b187 Cr-Commit-Position: refs/heads/master@{#40411} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/0c6354e03b84f41322edf45b81dc4ddb0394b187 Cr-Commit-Position: refs/heads/master@{#40411} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
