Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2868103002:
[deserializer] Make large object deserialization GC safe (Closed)
|
Created:
3 years, 7 months ago by Jakob Kummerow Modified:
3 years, 7 months ago CC:
v8-reviews_googlegroups.com, Hannes Payer (out of office), Camillo Bruni Target Ref:
refs/heads/master Project:
v8 Visibility:
Public. |
Description[deserializer] Make large object deserialization GC safe
When black allocation is turned on at deserialization time, then
slots in deserialized objects have to be visited by the incremental
marker. For spaces with reservations, this has always been done; for
large object space with its special handling, this patch adds it.
Additionally, we must ensure that no incremental steps that might
cause incremental marking to finish are performed while there is an
AlwaysAllocateScope around.
BUG=chromium:718859
Review-Url: https://codereview.chromium.org/2868103002
Cr-Commit-Position: refs/heads/master@{#45231}
Committed: https://chromium.googlesource.com/v8/v8/+/6bfee50e15f1dda1bcc164d969d2c94453aa04dd
Patch Set 1 #
Total comments: 8
Patch Set 2 : addressed comments #Patch Set 3 : fix tests for --no-turbo #Patch Set 4 : fix tests for --noopt #
Messages
Total messages: 32 (20 generated)
jkummerow@chromium.org changed reviewers: + ulan@chromium.org
Ulan, PTAL. Camillo, FYI. Big thanks to both of you for your contributions to tracking this one down!
The CQ bit was checked by jkummerow@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/25683) 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...) v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/22221) v8_mac_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng_triggered/bui...)
mlippautz@chromium.org changed reviewers: + mlippautz@chromium.org
Amazing find! https://codereview.chromium.org/2868103002/diff/1/test/cctest/test-serialize.cc File test/cctest/test-serialize.cc (right): https://codereview.chromium.org/2868103002/diff/1/test/cctest/test-serialize.... test/cctest/test-serialize.cc:1177: IncrementalMarking* marking = heap->incremental_marking(); I think you can use heap::SimulateIncrementalMarking(heap, false) instead of the block 1177-1185. https://codereview.chromium.org/2868103002/diff/1/test/cctest/test-serialize.... test/cctest/test-serialize.cc:1201: while (!marking->IsComplete()) { I think you can use heap::SimulateIncrementalMarking(heap, true) instead of the loop.
ulan@google.com changed reviewers: + ulan@google.com
LGTM! Please address Michael's comments regarding the test before landing.
On 2017/05/09 19:27:56, ulan_google wrote: > LGTM! > > Please address Michael's comments regarding the test before landing. deserializer lgtm
hpayer@chromium.org changed reviewers: + hpayer@chromium.org
Can you elaborate on the AlwaysAllocate/incremental marking change? https://codereview.chromium.org/2868103002/diff/1/src/snapshot/deserializer.cc File src/snapshot/deserializer.cc (right): https://codereview.chromium.org/2868103002/diff/1/src/snapshot/deserializer.c... src/snapshot/deserializer.cc:182: RecordWritesForLargeObjects(); Ideally, we would move this one into RegisterReservationsForBlackAllocation and rename RegisterReservationsForBlackAllocation to something more generic like RegisterDeserializedObjectsForBlackAllocation. https://codereview.chromium.org/2868103002/diff/1/src/snapshot/deserializer.c... src/snapshot/deserializer.cc:184: isolate->heap()->RegisterReservationsForBlackAllocation(reservations_); I guess that means we never have reservations for LO? Whenever we iterate reservations we go to LO space.
ulan@chromium.org changed reviewers: - ulan@google.com
lgtm (from chromium account)
After discussing the always allocate scope offline. LGTM
Thanks for the reviews; comments addressed. https://codereview.chromium.org/2868103002/diff/1/src/snapshot/deserializer.cc File src/snapshot/deserializer.cc (right): https://codereview.chromium.org/2868103002/diff/1/src/snapshot/deserializer.c... src/snapshot/deserializer.cc:182: RecordWritesForLargeObjects(); On 2017/05/09 21:19:51, Hannes Payer wrote: > Ideally, we would move this one into RegisterReservationsForBlackAllocation and > rename RegisterReservationsForBlackAllocation to something more generic like > RegisterDeserializedObjectsForBlackAllocation. Done. https://codereview.chromium.org/2868103002/diff/1/src/snapshot/deserializer.c... src/snapshot/deserializer.cc:184: isolate->heap()->RegisterReservationsForBlackAllocation(reservations_); On 2017/05/09 21:19:51, Hannes Payer wrote: > I guess that means we never have reservations for LO? Whenever we iterate > reservations we go to LO space. Correct; LO space doesn't use reservations (because it can't -- there's no way to batch allocations for several large objects). https://codereview.chromium.org/2868103002/diff/1/test/cctest/test-serialize.cc File test/cctest/test-serialize.cc (right): https://codereview.chromium.org/2868103002/diff/1/test/cctest/test-serialize.... test/cctest/test-serialize.cc:1177: IncrementalMarking* marking = heap->incremental_marking(); On 2017/05/09 18:27:44, Michael Lippautz wrote: > I think you can use > heap::SimulateIncrementalMarking(heap, false) > instead of the block 1177-1185. Done. https://codereview.chromium.org/2868103002/diff/1/test/cctest/test-serialize.... test/cctest/test-serialize.cc:1201: while (!marking->IsComplete()) { On 2017/05/09 18:27:44, Michael Lippautz wrote: > I think you can use > heap::SimulateIncrementalMarking(heap, true) > instead of the loop. Done.
The CQ bit was checked by jkummerow@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 jkummerow@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ulan@google.com, ulan@chromium.org, hpayer@chromium.org, yangguo@chromium.org Link to the patchset: https://codereview.chromium.org/2868103002/#ps40001 (title: "fix tests for --no-turbo")
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
Try jobs failed on following builders: 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_linux_arm_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng_trigger...)
The CQ bit was checked by jkummerow@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ulan@google.com, ulan@chromium.org, hpayer@chromium.org, yangguo@chromium.org Link to the patchset: https://codereview.chromium.org/2868103002/#ps60001 (title: "fix tests for --noopt")
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": 60001, "attempt_start_ts": 1494418518697370,
"parent_rev": "61101232574569a69241f2d8835931db12a16eda", "commit_rev":
"6bfee50e15f1dda1bcc164d969d2c94453aa04dd"}
Message was sent while issue was closed.
Description was changed from ========== [deserializer] Make large object deserialization GC safe When black allocation is turned on at deserialization time, then slots in deserialized objects have to be visited by the incremental marker. For spaces with reservations, this has always been done; for large object space with its special handling, this patch adds it. Additionally, we must ensure that no incremental steps that might cause incremental marking to finish are performed while there is an AlwaysAllocateScope around. BUG=chromium:718859 ========== to ========== [deserializer] Make large object deserialization GC safe When black allocation is turned on at deserialization time, then slots in deserialized objects have to be visited by the incremental marker. For spaces with reservations, this has always been done; for large object space with its special handling, this patch adds it. Additionally, we must ensure that no incremental steps that might cause incremental marking to finish are performed while there is an AlwaysAllocateScope around. BUG=chromium:718859 Review-Url: https://codereview.chromium.org/2868103002 Cr-Commit-Position: refs/heads/master@{#45231} Committed: https://chromium.googlesource.com/v8/v8/+/6bfee50e15f1dda1bcc164d969d2c94453a... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/v8/v8/+/6bfee50e15f1dda1bcc164d969d2c94453a... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
