Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2471883003:
[wasm] WebAssembly.Memory object can be referenced by multiple Instance objects. (Closed)
Description[wasm] WebAssembly.Memory object can be referenced by multiple Instance objects.
Add support for WebAssembly.Memory objects to be simultaneously referenced by multiple Instance objects. GrowingMemory should maintain a consistent view of memory across instances.
- Store a link to instances that share WebAssembly.Memory in the WasmMemoryObject, updated on instantiate.
- Implement WasmInstanceWrapper as a wrapper around the instance object to keep track of previous/next instances, instance object is stored as a WeakCell that can be garbage collected.
- MemoryInstanceFinalizer maintains a valid list of instances when an instance is garbage collected.
- Refactor GrowInstanceMemory to GrowMemoryBuffer that allocates a new buffer, and UncheckedUpdateInstanceMemory that updates memory references for an instance.
R=titzer@chromium.org, mtrofin@chromium.org, bradnelson@chromium.org
Committed: https://crrev.com/30ef8e33f3a199a27ca8512bcee314c9522d03f6
Committed: https://crrev.com/3c98e339599b068f1ed630afb7601ff942424d31
Committed: https://crrev.com/e108f90d5c17588910f1f91c56ceba2580277000
Cr-Original-Original-Commit-Position: refs/heads/master@{#41121}
Cr-Original-Commit-Position: refs/heads/master@{#41198}
Cr-Commit-Position: refs/heads/master@{#41234}
Patch Set 1 #Patch Set 2 : Rebase, add Dcheck #
Total comments: 29
Patch Set 3 : rebase #Patch Set 4 : Rebase, Refactor to use wasm-objects #Patch Set 5 : Get correct max_size #Patch Set 6 : Merge Finalizers #Patch Set 7 : Review changes #Patch Set 8 : Rebase #Patch Set 9 : Rebase #Patch Set 10 : Fix tests #Patch Set 11 : Fix GC stress test #Patch Set 12 : Fix bad merge #Patch Set 13 : Remove unused function definition #
Total comments: 8
Patch Set 14 : Andreas's review #
Messages
Total messages: 88 (64 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...
Description was changed from ========== [wasm] WebAssembly.Memory object can be referenced by multiple Instance objects. Add support for WebAssembly.Memory objects to be simultaneously referenced by multiple Instance objects. GrowingMemory should maintain a consistent view of memory across instances. - Store a link to instances that share WebAssembly.Memory in the WasmMemoryObject, updated on instantiate. - Implement WasmInstanceWrapper as a wrapper around the instance object to keep track of previous/next instances, instance object is stored as a WeakCell that can be garbage collected. - MemoryInstanceFinalizer maintains a valid list of instances when an instance is garbage collected. - Refactor GrowInstanceMemory to GrowMemoryBuffer that allocates a new buffer, and UncheckedUpdateInstanceMemory that updates memory references for an instance. R=titzer@chromium.org, mtrofin@chromium.org, bradnelson@chromium.org ========== to ========== [wasm] WebAssembly.Memory object can be referenced by multiple Instance objects. Add support for WebAssembly.Memory objects to be simultaneously referenced by multiple Instance objects. GrowingMemory should maintain a consistent view of memory across instances. - Store a link to instances that share WebAssembly.Memory in the WasmMemoryObject, updated on instantiate. - Implement WasmInstanceWrapper as a wrapper around the instance object to keep track of previous/next instances, instance object is stored as a WeakCell that can be garbage collected. - MemoryInstanceFinalizer maintains a valid list of instances when an instance is garbage collected. - Refactor GrowInstanceMemory to GrowMemoryBuffer that allocates a new buffer, and UncheckedUpdateInstanceMemory that updates memory references for an instance. R=titzer@chromium.org, mtrofin@chromium.org, bradnelson@chromium.org ==========
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: This issue passed the CQ dry run.
PTAL
https://codereview.chromium.org/2471883003/diff/20001/src/wasm/wasm-js.cc File src/wasm/wasm-js.cc (right): https://codereview.chromium.org/2471883003/diff/20001/src/wasm/wasm-js.cc#new... src/wasm/wasm-js.cc:37: kWasmMemoryInstancesLink, I think this might be a bit simpler if we just used a regular FixedArray here and only grow it when we run out of slots, instead of trying to manage a weak list.
https://codereview.chromium.org/2471883003/diff/20001/src/wasm/wasm-js.cc File src/wasm/wasm-js.cc (right): https://codereview.chromium.org/2471883003/diff/20001/src/wasm/wasm-js.cc#new... src/wasm/wasm-js.cc:37: kWasmMemoryInstancesLink, On 2016/11/07 19:34:07, titzer wrote: > I think this might be a bit simpler if we just used a regular FixedArray here > and only grow it when we run out of slots, instead of trying to manage a weak > list. Do you mean using a FixedArray to store weak links to the instances, and grow the array when we run out of slots for instances, i.e, check for empty slots that could have been garbage collected and grow if needed? I'm not sure what the optimal size of the array in this case would be. Experimenting with it a little bit, I've found the weak list to be a simpler approach because of the book keeping overhead. I'm probably missing something here - could you elaborate on why this would be better as a FixedArray?
https://codereview.chromium.org/2471883003/diff/20001/src/wasm/wasm-js.cc File src/wasm/wasm-js.cc (right): https://codereview.chromium.org/2471883003/diff/20001/src/wasm/wasm-js.cc#new... src/wasm/wasm-js.cc:37: kWasmMemoryInstancesLink, On 2016/11/07 19:34:07, titzer wrote: > I think this might be a bit simpler if we just used a regular FixedArray here > and only grow it when we run out of slots, instead of trying to manage a weak > list. So this is currently keeping a doubly-linked (strong) chain of wrappers, with only the link to the instance being weak. The instances keep a link to their wrapper, so that deletion is direct. Doing this with a FixedArray of weak references to the instances would mean more bookkeeping around edge cases I would think? https://codereview.chromium.org/2471883003/diff/20001/src/wasm/wasm-js.h File src/wasm/wasm-js.h (right): https://codereview.chromium.org/2471883003/diff/20001/src/wasm/wasm-js.h#newc... src/wasm/wasm-js.h:64: Handle<Object> value); value -> instance? https://codereview.chromium.org/2471883003/diff/20001/src/wasm/wasm-module.cc File src/wasm/wasm-module.cc (right): https://codereview.chromium.org/2471883003/diff/20001/src/wasm/wasm-module.cc... src/wasm/wasm-module.cc:2382: &MemoryInstanceFinalizer, You should be able to piggy-back this on the existing instance finalizer.
https://codereview.chromium.org/2471883003/diff/20001/src/wasm/wasm-js.cc File src/wasm/wasm-js.cc (right): https://codereview.chromium.org/2471883003/diff/20001/src/wasm/wasm-js.cc#new... src/wasm/wasm-js.cc:251: if (args.Length() > 2 && args[2]->IsObject()) { there's another place we check if args.Length() > 2, etc, just above. Cache the result? After that, you can probably avoid mem_obj being first declared and then set. https://codereview.chromium.org/2471883003/diff/20001/src/wasm/wasm-js.cc#new... src/wasm/wasm-js.cc:934: const int kWasmMemInstanceWrapper = 5; This is fragile, because this value here ("5") needs to be kept in sync with the corresponding value on the wasm-module side of things. Better expose an API on that side. https://codereview.chromium.org/2471883003/diff/20001/src/wasm/wasm-module.cc File src/wasm/wasm-module.cc (right): https://codereview.chromium.org/2471883003/diff/20001/src/wasm/wasm-module.cc... src/wasm/wasm-module.cc:1112: int num_imported_functions = ProcessImports(code_table, instance); the call to ProcessImports may setup a memory instances chain. Between here and line 1150, we might get GC (if not today, then at some other point in the future). As a result, the relocation may happen more than once. In addition, for maintainability, it may be best we deferred linking the memory in the chain and the relocation for this instance in one place in code. So maybe ProcessImports just sets memory_, but then you do the SetWasmMemory around line 1150. https://codereview.chromium.org/2471883003/diff/20001/src/wasm/wasm-module.cc... src/wasm/wasm-module.cc:2170: int32_t wasm::GrowWebAssemblyMemory(Isolate* isolate, please add comment explaining what the return is. https://codereview.chromium.org/2471883003/diff/20001/src/wasm/wasm-module.cc... src/wasm/wasm-module.cc:2175: if (WasmModule::kV8MaxPages < max_pages) return -1; Should this be a DCHECK rather, and have the caller ensure this is not happening? Also, what's the spec say for this case? https://codereview.chromium.org/2471883003/diff/20001/src/wasm/wasm-module.cc... src/wasm/wasm-module.cc:2183: if (pages == 0) return GetInstanceMemorySize(isolate, instance); why support pages == 0? Should this be an early DCHECK rather? https://codereview.chromium.org/2471883003/diff/20001/src/wasm/wasm-module.cc... src/wasm/wasm-module.cc:2193: while (instance_wrapper->has_next()) { nit: perhaps paranoia, but you could do this in a for loop, thus having a better readability of how you advance through the list. https://codereview.chromium.org/2471883003/diff/20001/src/wasm/wasm-module.cc... src/wasm/wasm-module.cc:2301: static void MemoryInstanceFinalizer(const v8::WeakCallbackInfo<void>& data) { I'd piggy-back on the existing finalizer registration, and add a "grand finalizer" that then orchestrates this and the compiled modules one. The grand finalizer can do the parameter unpacking, too, perhaps. Also, it can ensure ordering (i.e. first memory, then instances - or vice-versa, whichever), thus ensuring determinism. https://codereview.chromium.org/2471883003/diff/20001/src/wasm/wasm-module.cc... src/wasm/wasm-module.cc:2330: // the memory object. This comment makes me believe even moreso that we need to explicitly control the ordering of the finalizers https://codereview.chromium.org/2471883003/diff/20001/src/wasm/wasm-module.cc... src/wasm/wasm-module.cc:2331: const int kWasmMemObjectInstancesLink = 2; please see my earlier comment about fragility. Please move this logic outside, to avoid relying on "2" being the value here. https://codereview.chromium.org/2471883003/diff/20001/test/mjsunit/wasm/insta... File test/mjsunit/wasm/instantiate-module-basic.js (right): https://codereview.chromium.org/2471883003/diff/20001/test/mjsunit/wasm/insta... test/mjsunit/wasm/instantiate-module-basic.js:218: print("MustBeMemory..."); do you need this print?
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...
Refactored to include recent move to wasm-objects, ptal. https://codereview.chromium.org/2471883003/diff/20001/src/wasm/wasm-js.cc File src/wasm/wasm-js.cc (right): https://codereview.chromium.org/2471883003/diff/20001/src/wasm/wasm-js.cc#new... src/wasm/wasm-js.cc:251: if (args.Length() > 2 && args[2]->IsObject()) { On 2016/11/09 18:57:09, Mircea Trofin wrote: > there's another place we check if args.Length() > 2, etc, just above. Cache the > result? After that, you can probably avoid mem_obj being first declared and then > set. This is no longer required after refactoring, removed. https://codereview.chromium.org/2471883003/diff/20001/src/wasm/wasm-js.cc#new... src/wasm/wasm-js.cc:934: const int kWasmMemInstanceWrapper = 5; On 2016/11/09 18:57:09, Mircea Trofin wrote: > This is fragile, because this value here ("5") needs to be kept in sync with the > corresponding value on the wasm-module side of things. Better expose an API on > that side. Done. https://codereview.chromium.org/2471883003/diff/20001/src/wasm/wasm-js.h File src/wasm/wasm-js.h (right): https://codereview.chromium.org/2471883003/diff/20001/src/wasm/wasm-js.h#newc... src/wasm/wasm-js.h:64: Handle<Object> value); On 2016/11/09 01:40:11, bradnelson wrote: > value -> instance? Refactored to remove the function. https://codereview.chromium.org/2471883003/diff/20001/src/wasm/wasm-module.cc File src/wasm/wasm-module.cc (right): https://codereview.chromium.org/2471883003/diff/20001/src/wasm/wasm-module.cc... src/wasm/wasm-module.cc:1112: int num_imported_functions = ProcessImports(code_table, instance); On 2016/11/09 18:57:09, Mircea Trofin wrote: > the call to ProcessImports may setup a memory instances chain. Between here and > line 1150, we might get GC (if not today, then at some other point in the > future). As a result, the relocation may happen more than once. > > In addition, for maintainability, it may be best we deferred linking the memory > in the chain and the relocation for this instance in one place in code. So maybe > ProcessImports just sets memory_, but then you do the SetWasmMemory around line > 1150. Maybe I'm misunderstanding your point here, but setting up/updating the instances chain for the memory object does not cause relocations. My rationale here is that between here and line 1150 below, if we get a GC, depending on how the wrapper is set up the instances chain will be updated but I do not think this will trigger multiple relocations. Agree with this from a maintainability standpoint, moved to where memory is updated. https://codereview.chromium.org/2471883003/diff/20001/src/wasm/wasm-module.cc... src/wasm/wasm-module.cc:2170: int32_t wasm::GrowWebAssemblyMemory(Isolate* isolate, On 2016/11/09 18:57:10, Mircea Trofin wrote: > please add comment explaining what the return is. This is as per Spec for GrowMemory - Return the previous memory size in units of pages or -1 on failure. https://github.com/WebAssembly/design/blob/357a311f9322ebb37192dc15429c3c3667... https://codereview.chromium.org/2471883003/diff/20001/src/wasm/wasm-module.cc... src/wasm/wasm-module.cc:2175: if (WasmModule::kV8MaxPages < max_pages) return -1; On 2016/11/09 18:57:10, Mircea Trofin wrote: > Should this be a DCHECK rather, and have the caller ensure this is not > happening? > > Also, what's the spec say for this case? As per the spec, grow_memory should return -1 for failure cases. This is more of a failure case based on the V8 implementation because V8MaxPages < SpecMaxPages. So my interpretation is that this should fail gracefully instead of crashing when trying to allocate a larger number of pages. The Spec says system allocation for grow_memory may fail, and failure cases return -1. Can change to DCHECK if others think this should be a DCHECK. https://codereview.chromium.org/2471883003/diff/20001/src/wasm/wasm-module.cc... src/wasm/wasm-module.cc:2183: if (pages == 0) return GetInstanceMemorySize(isolate, instance); On 2016/11/09 18:57:09, Mircea Trofin wrote: > why support pages == 0? Should this be an early DCHECK rather? The Spec says - "Return the previous memory size in units of pages or -1 on failure." It does not say anything about the 0 case specifically, but 0 is a valid delta value, and the previous size in this case will be the current size of the memory. https://codereview.chromium.org/2471883003/diff/20001/src/wasm/wasm-module.cc... src/wasm/wasm-module.cc:2193: while (instance_wrapper->has_next()) { On 2016/11/09 18:57:09, Mircea Trofin wrote: > nit: perhaps paranoia, but you could do this in a for loop, thus having a better > readability of how you advance through the list. I would prefer to leave this as is, as this seems to be more readable to me. Can change if you feel strongly about it. https://codereview.chromium.org/2471883003/diff/20001/src/wasm/wasm-module.cc... src/wasm/wasm-module.cc:2301: static void MemoryInstanceFinalizer(const v8::WeakCallbackInfo<void>& data) { On 2016/11/09 18:57:10, Mircea Trofin wrote: > I'd piggy-back on the existing finalizer registration, and add a "grand > finalizer" that then orchestrates this and the compiled modules one. The grand > finalizer can do the parameter unpacking, too, perhaps. Also, it can ensure > ordering (i.e. first memory, then instances - or vice-versa, whichever), thus > ensuring determinism. The MemoryInstanceFinalizer is optional, i.e, only needed if instances_link/memory_object associated with the link exist - so I've taken the InstanceFinalizer for the CompiledModules to be the "grand finalizer" and branch to update the instances chain if needed. https://codereview.chromium.org/2471883003/diff/20001/src/wasm/wasm-module.cc... src/wasm/wasm-module.cc:2330: // the memory object. On 2016/11/09 18:57:10, Mircea Trofin wrote: > This comment makes me believe even moreso that we need to explicitly control the > ordering of the finalizers Agreed, Done. https://codereview.chromium.org/2471883003/diff/20001/src/wasm/wasm-module.cc... src/wasm/wasm-module.cc:2331: const int kWasmMemObjectInstancesLink = 2; On 2016/11/09 18:57:10, Mircea Trofin wrote: > please see my earlier comment about fragility. Please move this logic outside, > to avoid relying on "2" being the value here. Done. https://codereview.chromium.org/2471883003/diff/20001/src/wasm/wasm-module.cc... src/wasm/wasm-module.cc:2382: &MemoryInstanceFinalizer, On 2016/11/09 01:40:11, bradnelson wrote: > You should be able to piggy-back this on the existing instance finalizer. Done. https://codereview.chromium.org/2471883003/diff/20001/test/mjsunit/wasm/insta... File test/mjsunit/wasm/instantiate-module-basic.js (right): https://codereview.chromium.org/2471883003/diff/20001/test/mjsunit/wasm/insta... test/mjsunit/wasm/instantiate-module-basic.js:218: print("MustBeMemory..."); On 2016/11/09 18:57:10, Mircea Trofin wrote: > do you need this print? This is consistent with other tests in this file that print the name of the test running. I happened to stumble on it when this failed due to a missing return in wasm-js.cc. and added it for consistence, debugging.
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: This issue passed the CQ dry run.
lgtm
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
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/2471883003/#ps180001 (title: "Fix tests")
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] WebAssembly.Memory object can be referenced by multiple Instance objects. Add support for WebAssembly.Memory objects to be simultaneously referenced by multiple Instance objects. GrowingMemory should maintain a consistent view of memory across instances. - Store a link to instances that share WebAssembly.Memory in the WasmMemoryObject, updated on instantiate. - Implement WasmInstanceWrapper as a wrapper around the instance object to keep track of previous/next instances, instance object is stored as a WeakCell that can be garbage collected. - MemoryInstanceFinalizer maintains a valid list of instances when an instance is garbage collected. - Refactor GrowInstanceMemory to GrowMemoryBuffer that allocates a new buffer, and UncheckedUpdateInstanceMemory that updates memory references for an instance. R=titzer@chromium.org, mtrofin@chromium.org, bradnelson@chromium.org ========== to ========== [wasm] WebAssembly.Memory object can be referenced by multiple Instance objects. Add support for WebAssembly.Memory objects to be simultaneously referenced by multiple Instance objects. GrowingMemory should maintain a consistent view of memory across instances. - Store a link to instances that share WebAssembly.Memory in the WasmMemoryObject, updated on instantiate. - Implement WasmInstanceWrapper as a wrapper around the instance object to keep track of previous/next instances, instance object is stored as a WeakCell that can be garbage collected. - MemoryInstanceFinalizer maintains a valid list of instances when an instance is garbage collected. - Refactor GrowInstanceMemory to GrowMemoryBuffer that allocates a new buffer, and UncheckedUpdateInstanceMemory that updates memory references for an instance. R=titzer@chromium.org, mtrofin@chromium.org, bradnelson@chromium.org ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== [wasm] WebAssembly.Memory object can be referenced by multiple Instance objects. Add support for WebAssembly.Memory objects to be simultaneously referenced by multiple Instance objects. GrowingMemory should maintain a consistent view of memory across instances. - Store a link to instances that share WebAssembly.Memory in the WasmMemoryObject, updated on instantiate. - Implement WasmInstanceWrapper as a wrapper around the instance object to keep track of previous/next instances, instance object is stored as a WeakCell that can be garbage collected. - MemoryInstanceFinalizer maintains a valid list of instances when an instance is garbage collected. - Refactor GrowInstanceMemory to GrowMemoryBuffer that allocates a new buffer, and UncheckedUpdateInstanceMemory that updates memory references for an instance. R=titzer@chromium.org, mtrofin@chromium.org, bradnelson@chromium.org ========== to ========== [wasm] WebAssembly.Memory object can be referenced by multiple Instance objects. Add support for WebAssembly.Memory objects to be simultaneously referenced by multiple Instance objects. GrowingMemory should maintain a consistent view of memory across instances. - Store a link to instances that share WebAssembly.Memory in the WasmMemoryObject, updated on instantiate. - Implement WasmInstanceWrapper as a wrapper around the instance object to keep track of previous/next instances, instance object is stored as a WeakCell that can be garbage collected. - MemoryInstanceFinalizer maintains a valid list of instances when an instance is garbage collected. - Refactor GrowInstanceMemory to GrowMemoryBuffer that allocates a new buffer, and UncheckedUpdateInstanceMemory that updates memory references for an instance. R=titzer@chromium.org, mtrofin@chromium.org, bradnelson@chromium.org Committed: https://crrev.com/30ef8e33f3a199a27ca8512bcee314c9522d03f6 Cr-Commit-Position: refs/heads/master@{#41121} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/30ef8e33f3a199a27ca8512bcee314c9522d03f6 Cr-Commit-Position: refs/heads/master@{#41121}
Message was sent while issue was closed.
A revert of this CL (patchset #10 id:180001) has been created in https://codereview.chromium.org/2512323004/ by machenbach@chromium.org. The reason for reverting is: Breaks gc stress: https://build.chromium.org/p/client.v8/builders/V8%20Linux%20-%20gc%20stress/....
Message was sent while issue was closed.
Description was changed from ========== [wasm] WebAssembly.Memory object can be referenced by multiple Instance objects. Add support for WebAssembly.Memory objects to be simultaneously referenced by multiple Instance objects. GrowingMemory should maintain a consistent view of memory across instances. - Store a link to instances that share WebAssembly.Memory in the WasmMemoryObject, updated on instantiate. - Implement WasmInstanceWrapper as a wrapper around the instance object to keep track of previous/next instances, instance object is stored as a WeakCell that can be garbage collected. - MemoryInstanceFinalizer maintains a valid list of instances when an instance is garbage collected. - Refactor GrowInstanceMemory to GrowMemoryBuffer that allocates a new buffer, and UncheckedUpdateInstanceMemory that updates memory references for an instance. R=titzer@chromium.org, mtrofin@chromium.org, bradnelson@chromium.org Committed: https://crrev.com/30ef8e33f3a199a27ca8512bcee314c9522d03f6 Cr-Commit-Position: refs/heads/master@{#41121} ========== to ========== [wasm] WebAssembly.Memory object can be referenced by multiple Instance objects. Add support for WebAssembly.Memory objects to be simultaneously referenced by multiple Instance objects. GrowingMemory should maintain a consistent view of memory across instances. - Store a link to instances that share WebAssembly.Memory in the WasmMemoryObject, updated on instantiate. - Implement WasmInstanceWrapper as a wrapper around the instance object to keep track of previous/next instances, instance object is stored as a WeakCell that can be garbage collected. - MemoryInstanceFinalizer maintains a valid list of instances when an instance is garbage collected. - Refactor GrowInstanceMemory to GrowMemoryBuffer that allocates a new buffer, and UncheckedUpdateInstanceMemory that updates memory references for an instance. R=titzer@chromium.org, mtrofin@chromium.org, bradnelson@chromium.org Committed: https://crrev.com/30ef8e33f3a199a27ca8512bcee314c9522d03f6 Cr-Commit-Position: refs/heads/master@{#41121} ==========
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 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.
Fixed to use a TENURED object for the memory constructor, fixes the failing gc_stress test, attempting to reland change.
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/2471883003/#ps240001 (title: "Remove unused function definition")
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": 240001, "attempt_start_ts": 1479870411880730,
"parent_rev": "e9bf9acf5e7626fc76e85e9159c1ce0425429af9", "commit_rev":
"0e9d4b7cbffdd7463caca2730962b774bf27b982"}
Message was sent while issue was closed.
Description was changed from ========== [wasm] WebAssembly.Memory object can be referenced by multiple Instance objects. Add support for WebAssembly.Memory objects to be simultaneously referenced by multiple Instance objects. GrowingMemory should maintain a consistent view of memory across instances. - Store a link to instances that share WebAssembly.Memory in the WasmMemoryObject, updated on instantiate. - Implement WasmInstanceWrapper as a wrapper around the instance object to keep track of previous/next instances, instance object is stored as a WeakCell that can be garbage collected. - MemoryInstanceFinalizer maintains a valid list of instances when an instance is garbage collected. - Refactor GrowInstanceMemory to GrowMemoryBuffer that allocates a new buffer, and UncheckedUpdateInstanceMemory that updates memory references for an instance. R=titzer@chromium.org, mtrofin@chromium.org, bradnelson@chromium.org Committed: https://crrev.com/30ef8e33f3a199a27ca8512bcee314c9522d03f6 Cr-Commit-Position: refs/heads/master@{#41121} ========== to ========== [wasm] WebAssembly.Memory object can be referenced by multiple Instance objects. Add support for WebAssembly.Memory objects to be simultaneously referenced by multiple Instance objects. GrowingMemory should maintain a consistent view of memory across instances. - Store a link to instances that share WebAssembly.Memory in the WasmMemoryObject, updated on instantiate. - Implement WasmInstanceWrapper as a wrapper around the instance object to keep track of previous/next instances, instance object is stored as a WeakCell that can be garbage collected. - MemoryInstanceFinalizer maintains a valid list of instances when an instance is garbage collected. - Refactor GrowInstanceMemory to GrowMemoryBuffer that allocates a new buffer, and UncheckedUpdateInstanceMemory that updates memory references for an instance. R=titzer@chromium.org, mtrofin@chromium.org, bradnelson@chromium.org Committed: https://crrev.com/30ef8e33f3a199a27ca8512bcee314c9522d03f6 Cr-Commit-Position: refs/heads/master@{#41121} ==========
Message was sent while issue was closed.
Committed patchset #13 (id:240001)
Message was sent while issue was closed.
Description was changed from ========== [wasm] WebAssembly.Memory object can be referenced by multiple Instance objects. Add support for WebAssembly.Memory objects to be simultaneously referenced by multiple Instance objects. GrowingMemory should maintain a consistent view of memory across instances. - Store a link to instances that share WebAssembly.Memory in the WasmMemoryObject, updated on instantiate. - Implement WasmInstanceWrapper as a wrapper around the instance object to keep track of previous/next instances, instance object is stored as a WeakCell that can be garbage collected. - MemoryInstanceFinalizer maintains a valid list of instances when an instance is garbage collected. - Refactor GrowInstanceMemory to GrowMemoryBuffer that allocates a new buffer, and UncheckedUpdateInstanceMemory that updates memory references for an instance. R=titzer@chromium.org, mtrofin@chromium.org, bradnelson@chromium.org Committed: https://crrev.com/30ef8e33f3a199a27ca8512bcee314c9522d03f6 Cr-Commit-Position: refs/heads/master@{#41121} ========== to ========== [wasm] WebAssembly.Memory object can be referenced by multiple Instance objects. Add support for WebAssembly.Memory objects to be simultaneously referenced by multiple Instance objects. GrowingMemory should maintain a consistent view of memory across instances. - Store a link to instances that share WebAssembly.Memory in the WasmMemoryObject, updated on instantiate. - Implement WasmInstanceWrapper as a wrapper around the instance object to keep track of previous/next instances, instance object is stored as a WeakCell that can be garbage collected. - MemoryInstanceFinalizer maintains a valid list of instances when an instance is garbage collected. - Refactor GrowInstanceMemory to GrowMemoryBuffer that allocates a new buffer, and UncheckedUpdateInstanceMemory that updates memory references for an instance. R=titzer@chromium.org, mtrofin@chromium.org, bradnelson@chromium.org Committed: https://crrev.com/30ef8e33f3a199a27ca8512bcee314c9522d03f6 Committed: https://crrev.com/3c98e339599b068f1ed630afb7601ff942424d31 Cr-Original-Commit-Position: refs/heads/master@{#41121} Cr-Commit-Position: refs/heads/master@{#41198} ==========
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/3c98e339599b068f1ed630afb7601ff942424d31 Cr-Commit-Position: refs/heads/master@{#41198}
Message was sent while issue was closed.
A revert of this CL (patchset #13 id:240001) has been created in https://codereview.chromium.org/2529573002/ by hablich@chromium.org. The reason for reverting is: Test crashes after an unrelated revert: https://chromegw.corp.google.com/i/client.v8/builders/V8%20Linux%20-%20gc%20s... Reverting because of recommendation from WASM team..
Message was sent while issue was closed.
ahaas@chromium.org changed reviewers: + ahaas@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2471883003/diff/240001/src/wasm/wasm-module.cc File src/wasm/wasm-module.cc (right): https://codereview.chromium.org/2471883003/diff/240001/src/wasm/wasm-module.c... src/wasm/wasm-module.cc:561: // If the memory object is destroyed, nothing needs to be done here. Since you have a pointer to a heap object here (I guess instance is a heap object?), could you open a {DisallowHeapAllocation no_gc;} scope for this function? https://codereview.chromium.org/2471883003/diff/240001/src/wasm/wasm-module.c... src/wasm/wasm-module.cc:601: JSObject** p = reinterpret_cast<JSObject**>(data.GetParameter()); same here https://codereview.chromium.org/2471883003/diff/240001/src/wasm/wasm-module.c... src/wasm/wasm-module.cc:2222: Handle<WasmMemoryObject> memory_object(WasmMemoryObject::cast(*receiver)); Could you just say Handle<WasmMemoryObject> memory_object = Handle<WasmMemoryObject>::cast(receiver);? https://codereview.chromium.org/2471883003/diff/240001/src/wasm/wasm-objects.cc File src/wasm/wasm-objects.cc (right): https://codereview.chromium.org/2471883003/diff/240001/src/wasm/wasm-objects.... src/wasm/wasm-objects.cc:461: FixedArray* array = FixedArray::cast(obj); same here.
Message was sent while issue was closed.
Description was changed from ========== [wasm] WebAssembly.Memory object can be referenced by multiple Instance objects. Add support for WebAssembly.Memory objects to be simultaneously referenced by multiple Instance objects. GrowingMemory should maintain a consistent view of memory across instances. - Store a link to instances that share WebAssembly.Memory in the WasmMemoryObject, updated on instantiate. - Implement WasmInstanceWrapper as a wrapper around the instance object to keep track of previous/next instances, instance object is stored as a WeakCell that can be garbage collected. - MemoryInstanceFinalizer maintains a valid list of instances when an instance is garbage collected. - Refactor GrowInstanceMemory to GrowMemoryBuffer that allocates a new buffer, and UncheckedUpdateInstanceMemory that updates memory references for an instance. R=titzer@chromium.org, mtrofin@chromium.org, bradnelson@chromium.org Committed: https://crrev.com/30ef8e33f3a199a27ca8512bcee314c9522d03f6 Committed: https://crrev.com/3c98e339599b068f1ed630afb7601ff942424d31 Cr-Original-Commit-Position: refs/heads/master@{#41121} Cr-Commit-Position: refs/heads/master@{#41198} ========== to ========== [wasm] WebAssembly.Memory object can be referenced by multiple Instance objects. Add support for WebAssembly.Memory objects to be simultaneously referenced by multiple Instance objects. GrowingMemory should maintain a consistent view of memory across instances. - Store a link to instances that share WebAssembly.Memory in the WasmMemoryObject, updated on instantiate. - Implement WasmInstanceWrapper as a wrapper around the instance object to keep track of previous/next instances, instance object is stored as a WeakCell that can be garbage collected. - MemoryInstanceFinalizer maintains a valid list of instances when an instance is garbage collected. - Refactor GrowInstanceMemory to GrowMemoryBuffer that allocates a new buffer, and UncheckedUpdateInstanceMemory that updates memory references for an instance. R=titzer@chromium.org, mtrofin@chromium.org, bradnelson@chromium.org Committed: https://crrev.com/30ef8e33f3a199a27ca8512bcee314c9522d03f6 Committed: https://crrev.com/3c98e339599b068f1ed630afb7601ff942424d31 Cr-Original-Commit-Position: refs/heads/master@{#41121} Cr-Commit-Position: refs/heads/master@{#41198} ==========
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/2471883003/diff/240001/src/wasm/wasm-module.cc File src/wasm/wasm-module.cc (right): https://codereview.chromium.org/2471883003/diff/240001/src/wasm/wasm-module.c... src/wasm/wasm-module.cc:561: // If the memory object is destroyed, nothing needs to be done here. On 2016/11/23 10:25:55, ahaas wrote: > Since you have a pointer to a heap object here (I guess instance is a heap > object?), could you open a {DisallowHeapAllocation no_gc;} scope for this > function? Done. https://codereview.chromium.org/2471883003/diff/240001/src/wasm/wasm-module.c... src/wasm/wasm-module.cc:601: JSObject** p = reinterpret_cast<JSObject**>(data.GetParameter()); On 2016/11/23 10:25:55, ahaas wrote: > same here Done. https://codereview.chromium.org/2471883003/diff/240001/src/wasm/wasm-module.c... src/wasm/wasm-module.cc:2222: Handle<WasmMemoryObject> memory_object(WasmMemoryObject::cast(*receiver)); On 2016/11/23 10:25:55, ahaas wrote: > Could you just say > Handle<WasmMemoryObject> memory_object = > Handle<WasmMemoryObject>::cast(receiver);? Done. https://codereview.chromium.org/2471883003/diff/240001/src/wasm/wasm-objects.cc File src/wasm/wasm-objects.cc (right): https://codereview.chromium.org/2471883003/diff/240001/src/wasm/wasm-objects.... src/wasm/wasm-objects.cc:461: FixedArray* array = FixedArray::cast(obj); On 2016/11/23 10:25:55, ahaas wrote: > same here. Done.
Not able to reproduce this failure - - Tested at commit e3f5c515faa7ec90f044a765a5cd6b3e3be0d90d (Last revert before revert of this CL) - Tested at tip of trunk and tip of trunk with different combination of reverted CLs - Bisected with different CLs that were landed after this CL and cannot seem to reproduce this failure. Attempting to land this CL and will keep an eye on the tree and revert if the test crashes again.
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/2471883003/#ps260001 (title: "Andreas's review")
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": 260001, "attempt_start_ts": 1479933709659150,
"parent_rev": "390adc09c05f09ee1f11ad42785e162b3b086386", "commit_rev":
"771e6d4fb799473fd8e2eaaf2223380df3a7a666"}
Message was sent while issue was closed.
Description was changed from ========== [wasm] WebAssembly.Memory object can be referenced by multiple Instance objects. Add support for WebAssembly.Memory objects to be simultaneously referenced by multiple Instance objects. GrowingMemory should maintain a consistent view of memory across instances. - Store a link to instances that share WebAssembly.Memory in the WasmMemoryObject, updated on instantiate. - Implement WasmInstanceWrapper as a wrapper around the instance object to keep track of previous/next instances, instance object is stored as a WeakCell that can be garbage collected. - MemoryInstanceFinalizer maintains a valid list of instances when an instance is garbage collected. - Refactor GrowInstanceMemory to GrowMemoryBuffer that allocates a new buffer, and UncheckedUpdateInstanceMemory that updates memory references for an instance. R=titzer@chromium.org, mtrofin@chromium.org, bradnelson@chromium.org Committed: https://crrev.com/30ef8e33f3a199a27ca8512bcee314c9522d03f6 Committed: https://crrev.com/3c98e339599b068f1ed630afb7601ff942424d31 Cr-Original-Commit-Position: refs/heads/master@{#41121} Cr-Commit-Position: refs/heads/master@{#41198} ========== to ========== [wasm] WebAssembly.Memory object can be referenced by multiple Instance objects. Add support for WebAssembly.Memory objects to be simultaneously referenced by multiple Instance objects. GrowingMemory should maintain a consistent view of memory across instances. - Store a link to instances that share WebAssembly.Memory in the WasmMemoryObject, updated on instantiate. - Implement WasmInstanceWrapper as a wrapper around the instance object to keep track of previous/next instances, instance object is stored as a WeakCell that can be garbage collected. - MemoryInstanceFinalizer maintains a valid list of instances when an instance is garbage collected. - Refactor GrowInstanceMemory to GrowMemoryBuffer that allocates a new buffer, and UncheckedUpdateInstanceMemory that updates memory references for an instance. R=titzer@chromium.org, mtrofin@chromium.org, bradnelson@chromium.org Committed: https://crrev.com/30ef8e33f3a199a27ca8512bcee314c9522d03f6 Committed: https://crrev.com/3c98e339599b068f1ed630afb7601ff942424d31 Cr-Original-Commit-Position: refs/heads/master@{#41121} Cr-Commit-Position: refs/heads/master@{#41198} ==========
Message was sent while issue was closed.
Committed patchset #14 (id:260001)
Message was sent while issue was closed.
Description was changed from ========== [wasm] WebAssembly.Memory object can be referenced by multiple Instance objects. Add support for WebAssembly.Memory objects to be simultaneously referenced by multiple Instance objects. GrowingMemory should maintain a consistent view of memory across instances. - Store a link to instances that share WebAssembly.Memory in the WasmMemoryObject, updated on instantiate. - Implement WasmInstanceWrapper as a wrapper around the instance object to keep track of previous/next instances, instance object is stored as a WeakCell that can be garbage collected. - MemoryInstanceFinalizer maintains a valid list of instances when an instance is garbage collected. - Refactor GrowInstanceMemory to GrowMemoryBuffer that allocates a new buffer, and UncheckedUpdateInstanceMemory that updates memory references for an instance. R=titzer@chromium.org, mtrofin@chromium.org, bradnelson@chromium.org Committed: https://crrev.com/30ef8e33f3a199a27ca8512bcee314c9522d03f6 Committed: https://crrev.com/3c98e339599b068f1ed630afb7601ff942424d31 Cr-Original-Commit-Position: refs/heads/master@{#41121} Cr-Commit-Position: refs/heads/master@{#41198} ========== to ========== [wasm] WebAssembly.Memory object can be referenced by multiple Instance objects. Add support for WebAssembly.Memory objects to be simultaneously referenced by multiple Instance objects. GrowingMemory should maintain a consistent view of memory across instances. - Store a link to instances that share WebAssembly.Memory in the WasmMemoryObject, updated on instantiate. - Implement WasmInstanceWrapper as a wrapper around the instance object to keep track of previous/next instances, instance object is stored as a WeakCell that can be garbage collected. - MemoryInstanceFinalizer maintains a valid list of instances when an instance is garbage collected. - Refactor GrowInstanceMemory to GrowMemoryBuffer that allocates a new buffer, and UncheckedUpdateInstanceMemory that updates memory references for an instance. R=titzer@chromium.org, mtrofin@chromium.org, bradnelson@chromium.org Committed: https://crrev.com/30ef8e33f3a199a27ca8512bcee314c9522d03f6 Committed: https://crrev.com/3c98e339599b068f1ed630afb7601ff942424d31 Committed: https://crrev.com/e108f90d5c17588910f1f91c56ceba2580277000 Cr-Original-Original-Commit-Position: refs/heads/master@{#41121} Cr-Original-Commit-Position: refs/heads/master@{#41198} Cr-Commit-Position: refs/heads/master@{#41234} ==========
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/e108f90d5c17588910f1f91c56ceba2580277000 Cr-Commit-Position: refs/heads/master@{#41234}
Message was sent while issue was closed.
A revert of this CL (patchset #14 id:260001) has been created in https://codereview.chromium.org/2538183003/ by petermarshall@chromium.org. The reason for reverting is: Probably causes bot failures (see https://bugs.chromium.org/p/v8/issues/detail?id=5707).. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
