Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
4 years, 1 month ago by titzer Modified:
4 years, 1 month ago Reviewers:
bradnelson, rossberg CC:
v8-reviews_googlegroups.com Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[wasm] Support for restricted table imports.
This CL implements basic table import functionality.
Missing: growing of tables (WebAssembly.Grow) doesn't change dispatch tables
Missing: allowing larger table imports than minimum size
R=rossberg@chromium.org,bradnelson@chromium.org
BUG=v8:5507
Committed: https://crrev.com/b7aff1ff647595b7e529b6df0c719d513473703a
Cr-Commit-Position: refs/heads/master@{#40661}
Patch Set 1 #Patch Set 2 : [wasm] Support for restricted table imports. #Patch Set 3 : Fix signed/unsigned mismatch #Patch Set 4 : Implemented .Set() #
Total comments: 18
Patch Set 5 : [wasm] Support for restricted table imports. #Patch Set 6 : [wasm] Support for restricted table imports. #Patch Set 7 : Fix test #Patch Set 8 : Fix GC stress issue; tables weren't being reset correctly #
Created: 4 years, 1 month ago
Messages
Total messages: 39 (27 generated)
Description was changed from ========== [wasm] Support for restricted table imports. This CL implements basic table import functionality. R=rossberg@chromium.org,bradnelson@chromium.org BUG=v8:5507 ========== to ========== [wasm] Support for restricted table imports. This CL implements basic table import functionality. Missing: growing of tables (WebAssembly.Grow) doesnt change dispatch tables Missing: updates of tables (WebAssembly.Set) doesnt change dispatch tables Missing: consistency across mutations of tables Missing: allowing larger table imports than minimum size R=rossberg@chromium.org,bradnelson@chromium.org BUG=v8:5507 ==========
The CQ bit was checked by titzer@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...
PTAL. This is missing some core functionality, but might be worth landing in its incomplete state nonetheless.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_win64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel_ng/builds/16882)
The CQ bit was checked by titzer@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.
Description was changed from ========== [wasm] Support for restricted table imports. This CL implements basic table import functionality. Missing: growing of tables (WebAssembly.Grow) doesnt change dispatch tables Missing: updates of tables (WebAssembly.Set) doesnt change dispatch tables Missing: consistency across mutations of tables Missing: allowing larger table imports than minimum size R=rossberg@chromium.org,bradnelson@chromium.org BUG=v8:5507 ========== to ========== [wasm] Support for restricted table imports. This CL implements basic table import functionality. Missing: growing of tables (WebAssembly.Grow) doesn't change dispatch tables Missing: allowing larger table imports than minimum size R=rossberg@chromium.org,bradnelson@chromium.org BUG=v8:5507 ==========
The CQ bit was checked by titzer@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...
On 2016/10/27 20:34:29, titzer wrote: > PTAL. This is missing some core functionality, but might be worth landing in its > incomplete state nonetheless. PTAL, I've implemented the proper SET functionality and added tests.
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 titzer@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...
lgtm w/questions https://codereview.chromium.org/2454503005/diff/60001/src/compiler/wasm-compi... File src/compiler/wasm-compiler.cc (right): https://codereview.chromium.org/2454503005/diff/60001/src/compiler/wasm-compi... src/compiler/wasm-compiler.cc:2155: EnsureFunctionTableNodes(); Don't care for Ensure, but not sure a good alternative. I assume Domenic stuck this in here lazily to keep it localized. Any reason this can't be done up front? https://codereview.chromium.org/2454503005/diff/60001/src/wasm/wasm-js.cc File src/wasm/wasm-js.cc (right): https://codereview.chromium.org/2454503005/diff/60001/src/wasm/wasm-js.cc#new... src/wasm/wasm-js.cc:32: static const int kWasmTableDispatchTablesFieldIndex = 2; Any reason not to move these into the enum? https://codereview.chromium.org/2454503005/diff/60001/src/wasm/wasm-js.cc#new... src/wasm/wasm-js.cc:861: static bool IsBrand(i::Handle<i::Object> value, i::Handle<i::Symbol> symbol) { HasBrand is a little better. https://codereview.chromium.org/2454503005/diff/60001/src/wasm/wasm-module.cc File src/wasm/wasm-module.cc (right): https://codereview.chromium.org/2454503005/diff/60001/src/wasm/wasm-module.cc... src/wasm/wasm-module.cc:945: GetWasmFunctionForImportWrapper(isolate, function), Seems like CompileImportWrapper below could get used / moved up here? https://codereview.chromium.org/2454503005/diff/60001/src/wasm/wasm-module.h File src/wasm/wasm-module.h (right): https://codereview.chromium.org/2454503005/diff/60001/src/wasm/wasm-module.h#... src/wasm/wasm-module.h:180: static const size_t kV8MaxTableSize = 16 * 1024 * 1024; Reasonable limit, any particular motivation for this value? https://codereview.chromium.org/2454503005/diff/60001/test/mjsunit/wasm/indir... File test/mjsunit/wasm/indirect-tables.js (right): https://codereview.chromium.org/2454503005/diff/60001/test/mjsunit/wasm/indir... test/mjsunit/wasm/indirect-tables.js:201: let table = new WebAssembly.Table({element: "anyfunc", initial: kTableSize}); >80 https://codereview.chromium.org/2454503005/diff/60001/test/mjsunit/wasm/indir... test/mjsunit/wasm/indirect-tables.js:203: let i2 = new WebAssembly.Instance(m2, {base: i, table: table, js_div: js_div}); >80 https://codereview.chromium.org/2454503005/diff/60001/test/mjsunit/wasm/indir... test/mjsunit/wasm/indirect-tables.js:342: // TODO(titzer): a test that all instances using a table get updated Also one for import table size doesn't match initial size? https://codereview.chromium.org/2454503005/diff/60001/test/mjsunit/wasm/wasm-... File test/mjsunit/wasm/wasm-module-builder.js (right): https://codereview.chromium.org/2454503005/diff/60001/test/mjsunit/wasm/wasm-... test/mjsunit/wasm/wasm-module-builder.js:208: let o = {module: module, name: name, kind: kExternalTable, initial: initial, maximum: maximum}; >80
https://codereview.chromium.org/2454503005/diff/60001/src/compiler/wasm-compi... File src/compiler/wasm-compiler.cc (right): https://codereview.chromium.org/2454503005/diff/60001/src/compiler/wasm-compi... src/compiler/wasm-compiler.cc:2155: EnsureFunctionTableNodes(); On 2016/10/28 16:59:01, bradnelson wrote: > Don't care for Ensure, but not sure a good alternative. > I assume Domenic stuck this in here lazily to keep it localized. > Any reason this can't be done up front? Just to save nodes, since I assume most functions don't have direct calls. The previous code was allocating new nodes every time, which isn't a huge deal. I also made it easier to extend to multiple tables. https://codereview.chromium.org/2454503005/diff/60001/src/wasm/wasm-js.cc File src/wasm/wasm-js.cc (right): https://codereview.chromium.org/2454503005/diff/60001/src/wasm/wasm-js.cc#new... src/wasm/wasm-js.cc:32: static const int kWasmTableDispatchTablesFieldIndex = 2; On 2016/10/28 16:59:01, bradnelson wrote: > Any reason not to move these into the enum? Yeah, would be OK. I'm planning on moving this all to a wasm-objects.h header and hiding all the gunk behind a nicer typed interface, so no sense cleaning up too much now, IMHO. https://codereview.chromium.org/2454503005/diff/60001/src/wasm/wasm-js.cc#new... src/wasm/wasm-js.cc:861: static bool IsBrand(i::Handle<i::Object> value, i::Handle<i::Symbol> symbol) { On 2016/10/28 16:59:01, bradnelson wrote: > HasBrand is a little better. Done. https://codereview.chromium.org/2454503005/diff/60001/src/wasm/wasm-module.cc File src/wasm/wasm-module.cc (right): https://codereview.chromium.org/2454503005/diff/60001/src/wasm/wasm-module.cc... src/wasm/wasm-module.cc:945: GetWasmFunctionForImportWrapper(isolate, function), On 2016/10/28 16:59:01, bradnelson wrote: > Seems like CompileImportWrapper below could get used / moved up here? Done. https://codereview.chromium.org/2454503005/diff/60001/src/wasm/wasm-module.h File src/wasm/wasm-module.h (right): https://codereview.chromium.org/2454503005/diff/60001/src/wasm/wasm-module.h#... src/wasm/wasm-module.h:180: static const size_t kV8MaxTableSize = 16 * 1024 * 1024; On 2016/10/28 16:59:01, bradnelson wrote: > Reasonable limit, any particular motivation for this value? Just spitballing a reasonable limit on table size; since each entry is 16 bytes on 64-bit, that's about 128MB. https://codereview.chromium.org/2454503005/diff/60001/test/mjsunit/wasm/indir... File test/mjsunit/wasm/indirect-tables.js (right): https://codereview.chromium.org/2454503005/diff/60001/test/mjsunit/wasm/indir... test/mjsunit/wasm/indirect-tables.js:201: let table = new WebAssembly.Table({element: "anyfunc", initial: kTableSize}); On 2016/10/28 16:59:01, bradnelson wrote: > >80 Done. https://codereview.chromium.org/2454503005/diff/60001/test/mjsunit/wasm/indir... test/mjsunit/wasm/indirect-tables.js:203: let i2 = new WebAssembly.Instance(m2, {base: i, table: table, js_div: js_div}); On 2016/10/28 16:59:01, bradnelson wrote: > >80 Done. https://codereview.chromium.org/2454503005/diff/60001/test/mjsunit/wasm/indir... test/mjsunit/wasm/indirect-tables.js:342: // TODO(titzer): a test that all instances using a table get updated On 2016/10/28 16:59:01, bradnelson wrote: > Also one for import table size doesn't match initial size? Done. https://codereview.chromium.org/2454503005/diff/60001/test/mjsunit/wasm/wasm-... File test/mjsunit/wasm/wasm-module-builder.js (right): https://codereview.chromium.org/2454503005/diff/60001/test/mjsunit/wasm/wasm-... test/mjsunit/wasm/wasm-module-builder.js:208: let o = {module: module, name: name, kind: kExternalTable, initial: initial, maximum: maximum}; On 2016/10/28 16:59:01, bradnelson wrote: > >80 Done.
lgtm
The CQ bit was checked by titzer@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] Support for restricted table imports. This CL implements basic table import functionality. Missing: growing of tables (WebAssembly.Grow) doesn't change dispatch tables Missing: allowing larger table imports than minimum size R=rossberg@chromium.org,bradnelson@chromium.org BUG=v8:5507 ========== to ========== [wasm] Support for restricted table imports. This CL implements basic table import functionality. Missing: growing of tables (WebAssembly.Grow) doesn't change dispatch tables Missing: allowing larger table imports than minimum size R=rossberg@chromium.org,bradnelson@chromium.org BUG=v8:5507 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
A revert of this CL (patchset #7 id:120001) has been created in https://codereview.chromium.org/2456193006/ by machenbach@chromium.org. The reason for reverting is: GC stress failures: https://build.chromium.org/p/client.v8/builders/V8%20Linux64%20GC%20Stress%20....
Message was sent while issue was closed.
Description was changed from ========== [wasm] Support for restricted table imports. This CL implements basic table import functionality. Missing: growing of tables (WebAssembly.Grow) doesn't change dispatch tables Missing: allowing larger table imports than minimum size R=rossberg@chromium.org,bradnelson@chromium.org BUG=v8:5507 ========== to ========== [wasm] Support for restricted table imports. This CL implements basic table import functionality. Missing: growing of tables (WebAssembly.Grow) doesn't change dispatch tables Missing: allowing larger table imports than minimum size R=rossberg@chromium.org,bradnelson@chromium.org BUG=v8:5507 ==========
The CQ bit was checked by titzer@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 titzer@chromium.org
The CQ bit was checked by titzer@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/2454503005/#ps140001 (title: "Fix GC stress issue; tables weren't being reset correctly")
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] Support for restricted table imports. This CL implements basic table import functionality. Missing: growing of tables (WebAssembly.Grow) doesn't change dispatch tables Missing: allowing larger table imports than minimum size R=rossberg@chromium.org,bradnelson@chromium.org BUG=v8:5507 ========== to ========== [wasm] Support for restricted table imports. This CL implements basic table import functionality. Missing: growing of tables (WebAssembly.Grow) doesn't change dispatch tables Missing: allowing larger table imports than minimum size R=rossberg@chromium.org,bradnelson@chromium.org BUG=v8:5507 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== [wasm] Support for restricted table imports. This CL implements basic table import functionality. Missing: growing of tables (WebAssembly.Grow) doesn't change dispatch tables Missing: allowing larger table imports than minimum size R=rossberg@chromium.org,bradnelson@chromium.org BUG=v8:5507 ========== to ========== [wasm] Support for restricted table imports. This CL implements basic table import functionality. Missing: growing of tables (WebAssembly.Grow) doesn't change dispatch tables Missing: allowing larger table imports than minimum size R=rossberg@chromium.org,bradnelson@chromium.org BUG=v8:5507 Committed: https://crrev.com/404e215458df2676f21017cec00a7ae51fb163c1 Cr-Commit-Position: refs/heads/master@{#40652} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/404e215458df2676f21017cec00a7ae51fb163c1 Cr-Commit-Position: refs/heads/master@{#40652}
Message was sent while issue was closed.
Description was changed from ========== [wasm] Support for restricted table imports. This CL implements basic table import functionality. Missing: growing of tables (WebAssembly.Grow) doesn't change dispatch tables Missing: allowing larger table imports than minimum size R=rossberg@chromium.org,bradnelson@chromium.org BUG=v8:5507 Committed: https://crrev.com/404e215458df2676f21017cec00a7ae51fb163c1 Cr-Commit-Position: refs/heads/master@{#40652} ========== to ========== [wasm] Support for restricted table imports. This CL implements basic table import functionality. Missing: growing of tables (WebAssembly.Grow) doesn't change dispatch tables Missing: allowing larger table imports than minimum size R=rossberg@chromium.org,bradnelson@chromium.org BUG=v8:5507 Committed: https://crrev.com/b7aff1ff647595b7e529b6df0c719d513473703a Cr-Commit-Position: refs/heads/master@{#40661} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/b7aff1ff647595b7e529b6df0c719d513473703a Cr-Commit-Position: refs/heads/master@{#40661} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
