Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2972803002:
[wasm] Use WeakFixedArray for list of instances sharing a WasmMemoryObject. (Closed)
Description[wasm] Use WeakFixedArray for list of instances sharing a WasmMemoryObject.
This CL refactors the WasmMemoryObject and WasmInstanceObject classes to
use WeakFixedArray instead of using a doubly-linked list of instances. This
simplifies the lifetime management of instances by not requiring them to
be unlinked from this list upon GC. It also simplifies the iteration over
the instances using a given WasmMemoryObject.
Note that, contrary to my naive assumption at the outset, it is still necessary for the InstanceFinalizer (called upon a WasmInstanceObject death) to unlink itself from a WasmMemoryObject's instances list, due to finalizer ordering.
R=deepti@chromium.org, mlippautz@chromium.org
BUG=
Review-Url: https://codereview.chromium.org/2972803002
Cr-Commit-Position: refs/heads/master@{#46482}
Committed: https://chromium.googlesource.com/v8/v8/+/0a61361e47db80be620704333ac1abaa0d980256
Patch Set 1 #Patch Set 2 : [wasm] Use WeakFixedArray for list of instances sharing a WasmMemoryObject. #Patch Set 3 : Rebase #
Total comments: 7
Patch Set 4 : Address review comments #Patch Set 5 : [wasm] Use WeakFixedArray for list of instances sharing a WasmMemoryObject. #
Total comments: 7
Patch Set 6 : [wasm] Use WeakFixedArray for list of instances sharing a WasmMemoryObject. #Patch Set 7 : Remove unnecessary handle scope. #Patch Set 8 : [wasm] Use WeakFixedArray for list of instances sharing a WasmMemoryObject. #
Messages
Total messages: 43 (33 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...
Description was changed from ========== [wasm] Use WeakFixedArray for list of instances sharing a WasmMemoryObject. This CL refactors the WasmMemoryObject and WasmInstanceObject classes to use WeakFixedArray instead of using a doubly-linked list of instances. This simplifies the lifetime management of instances by not requiring them to be unlinked from this list upon GC. It also simplifies the iteration over the instances using a given WasmMemoryObject. R=deepti@chromium.org, mlippautz@chromium.org BUG= ========== to ========== [wasm] Use WeakFixedArray for list of instances sharing a WasmMemoryObject. This CL refactors the WasmMemoryObject and WasmInstanceObject classes to use WeakFixedArray instead of using a doubly-linked list of instances. This simplifies the lifetime management of instances by not requiring them to be unlinked from this list upon GC. It also simplifies the iteration over the instances using a given WasmMemoryObject. Note that, contrary to my naive assumption at the outset, it is still necessary for the InstanceFinalizer (called upon a WasmInstanceObject death) to unlink itself from a WasmMemoryObject's instances list, due to finalizer ordering. R=deepti@chromium.org, mlippautz@chromium.org BUG= ==========
titzer@chromium.org changed reviewers: + clemensh@chromium.org
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...)
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.
titzer@chromium.org changed reviewers: + gdeepti@chromium.org - deepti@chromium.org
PTAL
Lgtm, with nits. Thanks for fixing this! https://codereview.chromium.org/2972803002/diff/40001/src/wasm/wasm-objects.cc File src/wasm/wasm-objects.cc (right): https://codereview.chromium.org/2972803002/diff/40001/src/wasm/wasm-objects.c... src/wasm/wasm-objects.cc:351: WeakFixedArray::Add(old_instances, instance, &unused); Variable unused not needed here as WeakFixedArray::Add has a default value for assigned_index? https://codereview.chromium.org/2972803002/diff/40001/src/wasm/wasm-objects.c... src/wasm/wasm-objects.cc:401: if (!elem->IsWasmInstanceObject()) continue; Just to confirm, this is to skip over empty slots? https://codereview.chromium.org/2972803002/diff/40001/src/wasm/wasm-objects.c... src/wasm/wasm-objects.cc:405: UncheckedUpdateInstanceMemory(isolate, instance, old_mem_start, old_size); Nit: Move declaration of old_mem_start to be scoped to this loop as it is not being used anywhere else.
https://codereview.chromium.org/2972803002/diff/40001/src/wasm/wasm-module.cc File src/wasm/wasm-module.cc (right): https://codereview.chromium.org/2972803002/diff/40001/src/wasm/wasm-module.cc... src/wasm/wasm-module.cc:117: // {Managed<WasmModule>} has been collected earlier in this GC cycle. Comment lgtm :)
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/2972803002/diff/40001/src/wasm/wasm-objects.cc File src/wasm/wasm-objects.cc (right): https://codereview.chromium.org/2972803002/diff/40001/src/wasm/wasm-objects.c... src/wasm/wasm-objects.cc:351: WeakFixedArray::Add(old_instances, instance, &unused); On 2017/07/06 17:42:55, gdeepti wrote: > Variable unused not needed here as WeakFixedArray::Add has a default value for > assigned_index? Good catch. Done. https://codereview.chromium.org/2972803002/diff/40001/src/wasm/wasm-objects.c... src/wasm/wasm-objects.cc:401: if (!elem->IsWasmInstanceObject()) continue; On 2017/07/06 17:42:55, gdeepti wrote: > Just to confirm, this is to skip over empty slots? Acknowledged. https://codereview.chromium.org/2972803002/diff/40001/src/wasm/wasm-objects.c... src/wasm/wasm-objects.cc:405: UncheckedUpdateInstanceMemory(isolate, instance, old_mem_start, old_size); On 2017/07/06 17:42:55, gdeepti wrote: > Nit: Move declaration of old_mem_start to be scoped to this loop as it is not > being used anywhere else. 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 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.
LGTM, nice cleanup! Some nits. https://codereview.chromium.org/2972803002/diff/80001/src/wasm/wasm-module.cc File src/wasm/wasm-module.cc (right): https://codereview.chromium.org/2972803002/diff/80001/src/wasm/wasm-module.cc... src/wasm/wasm-module.cc:122: HandleScope scope(isolate); If this handle scope is needed, could you add a comment explaining why? https://codereview.chromium.org/2972803002/diff/80001/src/wasm/wasm-objects.cc File src/wasm/wasm-objects.cc (right): https://codereview.chromium.org/2972803002/diff/80001/src/wasm/wasm-objects.c... src/wasm/wasm-objects.cc:358: memory->instances()->Remove(instance); This is now linear in the number of instances, instead of constant. I think you should at least mention this in the CL description. https://codereview.chromium.org/2972803002/diff/80001/src/wasm/wasm-objects.c... src/wasm/wasm-objects.cc:385: uint32_t max_pages; This code is much clearer now 👍 Hard to see if it's semantically equivalent (because the old code was a mess), so let's hope that we have enough test coverage here.
https://codereview.chromium.org/2972803002/diff/80001/src/wasm/wasm-module.cc File src/wasm/wasm-module.cc (right): https://codereview.chromium.org/2972803002/diff/80001/src/wasm/wasm-module.cc... src/wasm/wasm-module.cc:122: HandleScope scope(isolate); On 2017/07/07 11:56:21, Clemens Hammacher wrote: > If this handle scope is needed, could you add a comment explaining why? Yes, because there is no outer handle scope. Added comment. https://codereview.chromium.org/2972803002/diff/80001/src/wasm/wasm-objects.cc File src/wasm/wasm-objects.cc (right): https://codereview.chromium.org/2972803002/diff/80001/src/wasm/wasm-objects.c... src/wasm/wasm-objects.cc:358: memory->instances()->Remove(instance); On 2017/07/07 11:56:21, Clemens Hammacher wrote: > This is now linear in the number of instances, instead of constant. I think you > should at least mention this in the CL description. Add a comment to the declaration. https://codereview.chromium.org/2972803002/diff/80001/src/wasm/wasm-objects.c... src/wasm/wasm-objects.cc:385: uint32_t max_pages; On 2017/07/07 11:56:21, Clemens Hammacher wrote: > This code is much clearer now 👍 > Hard to see if it's semantically equivalent (because the old code was a mess), > so let's hope that we have enough test coverage here. Acknowledged.
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/2972803002/diff/80001/src/wasm/wasm-module.cc File src/wasm/wasm-module.cc (right): https://codereview.chromium.org/2972803002/diff/80001/src/wasm/wasm-module.cc... src/wasm/wasm-module.cc:122: HandleScope scope(isolate); On 2017/07/07 at 12:07:41, titzer wrote: > On 2017/07/07 11:56:21, Clemens Hammacher wrote: > > If this handle scope is needed, could you add a comment explaining why? > > Yes, because there is no outer handle scope. Added comment. Wait, how does the code above (guarded by `trap_handler::UseTrapHandler()`) work without an outer handle scope?
On 2017/07/07 12:14:10, Clemens Hammacher wrote: > https://codereview.chromium.org/2972803002/diff/80001/src/wasm/wasm-module.cc > File src/wasm/wasm-module.cc (right): > > https://codereview.chromium.org/2972803002/diff/80001/src/wasm/wasm-module.cc... > src/wasm/wasm-module.cc:122: HandleScope scope(isolate); > On 2017/07/07 at 12:07:41, titzer wrote: > > On 2017/07/07 11:56:21, Clemens Hammacher wrote: > > > If this handle scope is needed, could you add a comment explaining why? > > > > Yes, because there is no outer handle scope. Added comment. > > Wait, how does the code above (guarded by `trap_handler::UseTrapHandler()`) work > without an outer handle scope? Ah yes. You are right. I see it now that in src/heap/heap.cc it opens a handle scope before calling GC callbacks. I can remove this now.
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.
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.
The CQ bit was checked by titzer@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from gdeepti@chromium.org, mlippautz@chromium.org, clemensh@chromium.org Link to the patchset: https://codereview.chromium.org/2972803002/#ps140001 (title: "[wasm] Use WeakFixedArray for list of instances sharing a WasmMemoryObject.")
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": 140001, "attempt_start_ts": 1499435430192480,
"parent_rev": "0a4b65ff29e1f503564965ee1755c2fda6795b54", "commit_rev":
"0a61361e47db80be620704333ac1abaa0d980256"}
Message was sent while issue was closed.
Description was changed from ========== [wasm] Use WeakFixedArray for list of instances sharing a WasmMemoryObject. This CL refactors the WasmMemoryObject and WasmInstanceObject classes to use WeakFixedArray instead of using a doubly-linked list of instances. This simplifies the lifetime management of instances by not requiring them to be unlinked from this list upon GC. It also simplifies the iteration over the instances using a given WasmMemoryObject. Note that, contrary to my naive assumption at the outset, it is still necessary for the InstanceFinalizer (called upon a WasmInstanceObject death) to unlink itself from a WasmMemoryObject's instances list, due to finalizer ordering. R=deepti@chromium.org, mlippautz@chromium.org BUG= ========== to ========== [wasm] Use WeakFixedArray for list of instances sharing a WasmMemoryObject. This CL refactors the WasmMemoryObject and WasmInstanceObject classes to use WeakFixedArray instead of using a doubly-linked list of instances. This simplifies the lifetime management of instances by not requiring them to be unlinked from this list upon GC. It also simplifies the iteration over the instances using a given WasmMemoryObject. Note that, contrary to my naive assumption at the outset, it is still necessary for the InstanceFinalizer (called upon a WasmInstanceObject death) to unlink itself from a WasmMemoryObject's instances list, due to finalizer ordering. R=deepti@chromium.org, mlippautz@chromium.org BUG= Review-Url: https://codereview.chromium.org/2972803002 Cr-Commit-Position: refs/heads/master@{#46482} Committed: https://chromium.googlesource.com/v8/v8/+/0a61361e47db80be620704333ac1abaa0d9... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/v8/v8/+/0a61361e47db80be620704333ac1abaa0d9... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
