Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2977113002:
[wasm] Fix user properties for exported wasm functions and add extensive tests. (Closed)
Description[wasm] Fix user properties for exported wasm functions and add extensive tests.
R=ishell@chromium.org,clemensh@chromium.org
BUG=chromium:742659
Review-Url: https://codereview.chromium.org/2977113002
Cr-Commit-Position: refs/heads/master@{#46772}
Committed: https://chromium.googlesource.com/v8/v8/+/57b9a3b142f221aa5454bb6a30361c2caf68a4c5
Patch Set 1 #Patch Set 2 : debug verify #
Total comments: 3
Patch Set 3 : [wasm] Fix user properties for exported wasm functions and add extensive tests. #Patch Set 4 : Replace with private symbols #Patch Set 5 : Install API once only. #Patch Set 6 : Fix test for previous #Patch Set 7 : Remove .orig files #
Total comments: 20
Patch Set 8 : Address review comments #
Total comments: 1
Patch Set 9 : Address nit #
Messages
Total messages: 42 (32 generated)
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: 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_linux_arm_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng/builds/...) v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/25802)
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: 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_linux_arm64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng/build...) v8_linux_verify_csa_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_verify_csa_rel_ng/...)
Tests lgtm with nits, leaving the rest for Igor to review. https://codereview.chromium.org/2977113002/diff/20001/test/mjsunit/wasm/user-... File test/mjsunit/wasm/user-properties.js (right): https://codereview.chromium.org/2977113002/diff/20001/test/mjsunit/wasm/user-... test/mjsunit/wasm/user-properties.js:76: var f = id; wouldn't it be easier to just write "var f = x => x;"? https://codereview.chromium.org/2977113002/diff/20001/test/mjsunit/wasm/user-... test/mjsunit/wasm/user-properties.js:97: for (f of [x => (x + 19 + globalCounter), minus18]) { This is the only use of minus18, so you should just inline it: (x => x - 18).
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...
https://codereview.chromium.org/2977113002/diff/20001/test/mjsunit/wasm/user-... File test/mjsunit/wasm/user-properties.js (right): https://codereview.chromium.org/2977113002/diff/20001/test/mjsunit/wasm/user-... test/mjsunit/wasm/user-properties.js:97: for (f of [x => (x + 19 + globalCounter), minus18]) { On 2017/07/14 13:04:35, Clemens Hammacher wrote: > This is the only use of minus18, so you should just inline it: (x => x - 18). This was intentional to have a JSFunction instance that is allocated only once and won't be GC'able.
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...
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...) v8_linux_verify_csa_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_verify_csa_rel_ng/...) v8_linux_verify_csa_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_verify_csa_rel_ng_...) v8_win64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel_ng/builds/30791) v8_win64_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel_ng_triggered/b...)
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: Try jobs failed on following builders: v8_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/29233) v8_linux_dbg_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng_triggered/b...)
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 2017/07/14 13:12:41, titzer wrote: > https://codereview.chromium.org/2977113002/diff/20001/test/mjsunit/wasm/user-... > File test/mjsunit/wasm/user-properties.js (right): > > https://codereview.chromium.org/2977113002/diff/20001/test/mjsunit/wasm/user-... > test/mjsunit/wasm/user-properties.js:97: for (f of [x => (x + 19 + > globalCounter), minus18]) { > On 2017/07/14 13:04:35, Clemens Hammacher wrote: > > This is the only use of minus18, so you should just inline it: (x => x - 18). > > This was intentional to have a JSFunction instance that is allocated only once > and won't be GC'able. ishell@, PTAL
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2977113002/diff/120001/src/wasm/wasm-objects.cc File src/wasm/wasm-objects.cc (right): https://codereview.chromium.org/2977113002/diff/120001/src/wasm/wasm-objects.... src/wasm/wasm-objects.cc:489: if (!object) return false; This check is not necessary. IsJSFunction() will do a Smi check anyway. https://codereview.chromium.org/2977113002/diff/120001/src/wasm/wasm-objects.... src/wasm/wasm-objects.cc:494: Handle<Symbol> symbol(js_function->GetHeap()->wasm_instance_symbol()); This way we will reuse an existing handle instead of creating a new one: Handle<Symbol> symbol(js_function->GetIsolate()->wasm_instance_symbol()); https://codereview.chromium.org/2977113002/diff/120001/src/wasm/wasm-objects.... src/wasm/wasm-objects.cc:498: return result.ToHandleChecked()->IsWasmInstanceObject(); Canonical way of using MaybeHandles would look like this: MaybeHandle<Object> maybe_result = ... Handle<Object> result; if (!maybe_result.ToHandle(&result)) return false; return result->IsWasmInstanceObject(); https://codereview.chromium.org/2977113002/diff/120001/src/wasm/wasm-objects.... src/wasm/wasm-objects.cc:507: Handle<Symbol> symbol(GetHeap()->wasm_instance_symbol()); GetInstance()->wasm_instance_symbol() to avoid handle allocation. https://codereview.chromium.org/2977113002/diff/120001/src/wasm/wasm-objects.... src/wasm/wasm-objects.cc:509: JSObject::GetPropertyOrElement(handle(this), symbol); You are calling handlified code from non-handlified one. Although it should be fine in your case, in general it's a bad practice. Could you please handlify the method or at least add DisallowHeapAllocation scope. https://codereview.chromium.org/2977113002/diff/120001/src/wasm/wasm-objects.... src/wasm/wasm-objects.cc:514: Handle<Symbol> symbol(GetHeap()->wasm_function_index_symbol()); Same here. https://codereview.chromium.org/2977113002/diff/120001/src/wasm/wasm-objects.... src/wasm/wasm-objects.cc:516: JSObject::GetPropertyOrElement(handle(this), symbol); And here. If the index field ever become an inobject unboxed double field (if someone will change the way the function is created) then this function will allocate a heap number and the caller will not expect that from a non-handlified function. https://codereview.chromium.org/2977113002/diff/120001/src/wasm/wasm-objects.... src/wasm/wasm-objects.cc:539: handle(isolate->native_context()->sloppy_function_map()), name, Just isolate->sloppy_function_map() to make it nicer. https://codereview.chromium.org/2977113002/diff/120001/src/wasm/wasm-objects.... src/wasm/wasm-objects.cc:547: isolate->heap()->wasm_function_index_symbol()); s/heap()->// to avoid handle creation. https://codereview.chromium.org/2977113002/diff/120001/test/mjsunit/wasm/user... File test/mjsunit/wasm/user-properties.js (right): https://codereview.chromium.org/2977113002/diff/120001/test/mjsunit/wasm/user... test/mjsunit/wasm/user-properties.js:40: Object.defineProperty(obj, 'name', {value: "crazy"}); // special for JSFunctions. It would be nice to respect the 80 chars limit.
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...
https://codereview.chromium.org/2977113002/diff/120001/src/wasm/wasm-objects.cc File src/wasm/wasm-objects.cc (right): https://codereview.chromium.org/2977113002/diff/120001/src/wasm/wasm-objects.... src/wasm/wasm-objects.cc:489: if (!object) return false; On 2017/07/18 17:32:39, Igor Sheludko wrote: > This check is not necessary. IsJSFunction() will do a Smi check anyway. Done. https://codereview.chromium.org/2977113002/diff/120001/src/wasm/wasm-objects.... src/wasm/wasm-objects.cc:494: Handle<Symbol> symbol(js_function->GetHeap()->wasm_instance_symbol()); On 2017/07/18 17:32:38, Igor Sheludko wrote: > This way we will reuse an existing handle instead of creating a new one: > Handle<Symbol> symbol(js_function->GetIsolate()->wasm_instance_symbol()); Done. https://codereview.chromium.org/2977113002/diff/120001/src/wasm/wasm-objects.... src/wasm/wasm-objects.cc:498: return result.ToHandleChecked()->IsWasmInstanceObject(); On 2017/07/18 17:32:38, Igor Sheludko wrote: > Canonical way of using MaybeHandles would look like this: > > MaybeHandle<Object> maybe_result = ... > Handle<Object> result; > if (!maybe_result.ToHandle(&result)) return false; > return result->IsWasmInstanceObject(); Done. https://codereview.chromium.org/2977113002/diff/120001/src/wasm/wasm-objects.... src/wasm/wasm-objects.cc:507: Handle<Symbol> symbol(GetHeap()->wasm_instance_symbol()); On 2017/07/18 17:32:38, Igor Sheludko wrote: > GetInstance()->wasm_instance_symbol() to avoid handle allocation. Done. https://codereview.chromium.org/2977113002/diff/120001/src/wasm/wasm-objects.... src/wasm/wasm-objects.cc:509: JSObject::GetPropertyOrElement(handle(this), symbol); On 2017/07/18 17:32:38, Igor Sheludko wrote: > You are calling handlified code from non-handlified one. Although it should be > fine in your case, in general it's a bad practice. > Could you please handlify the method or at least add DisallowHeapAllocation > scope. Done. https://codereview.chromium.org/2977113002/diff/120001/src/wasm/wasm-objects.... src/wasm/wasm-objects.cc:514: Handle<Symbol> symbol(GetHeap()->wasm_function_index_symbol()); On 2017/07/18 17:32:38, Igor Sheludko wrote: > Same here. Done. https://codereview.chromium.org/2977113002/diff/120001/src/wasm/wasm-objects.... src/wasm/wasm-objects.cc:516: JSObject::GetPropertyOrElement(handle(this), symbol); On 2017/07/18 17:32:38, Igor Sheludko wrote: > And here. If the index field ever become an inobject unboxed double field (if > someone will change the way the function is created) then this function will > allocate a heap number and the caller will not expect that from a non-handlified > function. I've added a DisallowHeapAllocation here. https://codereview.chromium.org/2977113002/diff/120001/src/wasm/wasm-objects.... src/wasm/wasm-objects.cc:539: handle(isolate->native_context()->sloppy_function_map()), name, On 2017/07/18 17:32:38, Igor Sheludko wrote: > Just isolate->sloppy_function_map() to make it nicer. Done. https://codereview.chromium.org/2977113002/diff/120001/src/wasm/wasm-objects.... src/wasm/wasm-objects.cc:547: isolate->heap()->wasm_function_index_symbol()); On 2017/07/18 17:32:39, Igor Sheludko wrote: > s/heap()->// to avoid handle creation. Done. https://codereview.chromium.org/2977113002/diff/120001/test/mjsunit/wasm/user... File test/mjsunit/wasm/user-properties.js (right): https://codereview.chromium.org/2977113002/diff/120001/test/mjsunit/wasm/user... test/mjsunit/wasm/user-properties.js:40: Object.defineProperty(obj, 'name', {value: "crazy"}); // special for JSFunctions. On 2017/07/18 17:32:39, Igor Sheludko wrote: > It would be nice to respect the 80 chars limit. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm with a nit: https://codereview.chromium.org/2977113002/diff/140001/src/wasm/wasm-objects.cc File src/wasm/wasm-objects.cc (right): https://codereview.chromium.org/2977113002/diff/140001/src/wasm/wasm-objects.... src/wasm/wasm-objects.cc:517: Handle<Symbol> symbol(GetHeap()->wasm_function_index_symbol()); s/GetHeap()/GetIsolate()->factory()/
The CQ bit was checked by titzer@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from clemensh@chromium.org, ishell@chromium.org Link to the patchset: https://codereview.chromium.org/2977113002/#ps160001 (title: "Address nit")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/07/19 08:28:57, Igor Sheludko wrote: > lgtm with a nit: > > https://codereview.chromium.org/2977113002/diff/140001/src/wasm/wasm-objects.cc > File src/wasm/wasm-objects.cc (right): > > https://codereview.chromium.org/2977113002/diff/140001/src/wasm/wasm-objects.... > src/wasm/wasm-objects.cc:517: Handle<Symbol> > symbol(GetHeap()->wasm_function_index_symbol()); > s/GetHeap()/GetIsolate()->factory()/ Done. Thanks!
CQ is committing da patch.
Bot data: {"patchset_id": 160001, "attempt_start_ts": 1500482338562790,
"parent_rev": "3e47cb87d63bb1ca70d547834be3d9f462046a87", "commit_rev":
"57b9a3b142f221aa5454bb6a30361c2caf68a4c5"}
Message was sent while issue was closed.
Description was changed from ========== [wasm] Fix user properties for exported wasm functions and add extensive tests. R=ishell@chromium.org,clemensh@chromium.org BUG=chromium:742659 ========== to ========== [wasm] Fix user properties for exported wasm functions and add extensive tests. R=ishell@chromium.org,clemensh@chromium.org BUG=chromium:742659 Review-Url: https://codereview.chromium.org/2977113002 Cr-Commit-Position: refs/heads/master@{#46772} Committed: https://chromium.googlesource.com/v8/v8/+/57b9a3b142f221aa5454bb6a30361c2caf6... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as https://chromium.googlesource.com/v8/v8/+/57b9a3b142f221aa5454bb6a30361c2caf6... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
