Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2179033005:
[heap] Don't consider mementos on pages below age mark (Closed)
|
Created:
4 years, 4 months ago by Michael Lippautz Modified:
4 years, 4 months ago Reviewers:
Hannes Payer (out of office) CC:
v8-reviews_googlegroups.com, Hannes Payer (out of office), ulan Base URL:
https://chromium.googlesource.com/v8/v8.git@master Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[heap] Don't consider mementos on pages below age mark
Objects that reside below the age mark could be on pages that have been moved
within new space. In this case mementos survived which can actually point to
already-collected allocation sites.
BUG=chromium:631050, chromium:581412
R=hpayer@chromium.org
Committed: https://crrev.com/e97b8686f234c4f49bec8f31bd1702d6908901cc
Cr-Commit-Position: refs/heads/master@{#38094}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Addressed comment #Patch Set 3 : Strict FROM space check #Patch Set 4 : Better age mark check #Patch Set 5 : Pick right age mark #
Total comments: 1
Messages
Total messages: 31 (23 generated)
Description was changed from ========== [heap] Fix finding mementos on pages below age mark Don't use a memento that resides on a page which is below the age mark. Those pages could've been moved within new space and the containing mementos could point to already collected allocation sites. BUG= ========== to ========== [heap] Fix finding mementos on pages below age mark Don't use a memento that resides on a page which is below the age mark. Those pages could've been moved within new space and the containing mementos could point to already collected allocation sites. BUG=chromium:631050,chromium:581412 R=hpayer@chromium.org ==========
mlippautz@chromium.org changed reviewers: + hpayer@chromium.org
Description was changed from ========== [heap] Fix finding mementos on pages below age mark Don't use a memento that resides on a page which is below the age mark. Those pages could've been moved within new space and the containing mementos could point to already collected allocation sites. BUG=chromium:631050,chromium:581412 R=hpayer@chromium.org ========== to ========== [heap] Don't consider mementos on pages below age mark Don't use a memento that resides on a page which is below the age mark. Those pages could've been moved within new space and the containing mementos could point to already collected allocation sites. BUG=chromium:631050,chromium:581412 R=hpayer@chromium.org ==========
ptal
The CQ bit was checked by mlippautz@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/2179033005/diff/1/src/heap/heap-inl.h File src/heap/heap-inl.h (right): https://codereview.chromium.org/2179033005/diff/1/src/heap/heap-inl.h#newcode487 src/heap/heap-inl.h:487: if ((object_page != Page::FromAddress(last_memento_word_address))) { Can you leave the original check unchanged?
The CQ bit was checked by mlippautz@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 ========== [heap] Don't consider mementos on pages below age mark Don't use a memento that resides on a page which is below the age mark. Those pages could've been moved within new space and the containing mementos could point to already collected allocation sites. BUG=chromium:631050,chromium:581412 R=hpayer@chromium.org ========== to ========== [heap] Only consider mementos on 'from' space pages Don't use a memento that resides on a page which is below the age mark. Those pages could've been moved within new space and the containing mementos could point to already collected allocation sites. BUG=chromium:631050,chromium:581412 R=hpayer@chromium.org ==========
Description was changed from ========== [heap] Only consider mementos on 'from' space pages Don't use a memento that resides on a page which is below the age mark. Those pages could've been moved within new space and the containing mementos could point to already collected allocation sites. BUG=chromium:631050,chromium:581412 R=hpayer@chromium.org ========== to ========== [heap] Only consider mementos on 'from' space pages Don't consider mementos in 'to' space as we cannot tell the difference between moved pages and non-moved pages. Moved pages can contain mementos that point to already collected allocation sites. BUG=chromium:631050,chromium:581412 R=hpayer@chromium.org ==========
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...)
Description was changed from ========== [heap] Only consider mementos on 'from' space pages Don't consider mementos in 'to' space as we cannot tell the difference between moved pages and non-moved pages. Moved pages can contain mementos that point to already collected allocation sites. BUG=chromium:631050,chromium:581412 R=hpayer@chromium.org ========== to ========== [heap] Don't consider mementos on pages below age mark Objects that reside below the age mark could be on pages that have been moved within new space. In this case mementos survived which can actually point to already-collected allocation sites. BUG=chromium:631050,chromium:581412 R=hpayer@chromium.org ==========
The CQ bit was checked by mlippautz@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 ========== [heap] Don't consider mementos on pages below age mark Objects that reside below the age mark could be on pages that have been moved within new space. In this case mementos survived which can actually point to already-collected allocation sites. BUG=chromium:631050,chromium:581412 R=hpayer@chromium.org ========== to ========== [heap] Don't consider mementos on pages below age mark Objects that reside below the age mark could be on pages that have been moved within new space. In this case mementos survived which can actually point to already-collected allocation sites. BUG=chromium:631050,chromium:581412 R=hpayer@chromium.org ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/5743) 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...)
The CQ bit was checked by mlippautz@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 https://codereview.chromium.org/2179033005/diff/1/src/heap/heap-inl.h File src/heap/heap-inl.h (right): https://codereview.chromium.org/2179033005/diff/1/src/heap/heap-inl.h#newcode487 src/heap/heap-inl.h:487: if ((object_page != Page::FromAddress(last_memento_word_address))) { On 2016/07/27 08:34:34, Hannes Payer wrote: > Can you leave the original check unchanged? Done. https://codereview.chromium.org/2179033005/diff/80001/src/heap/heap-inl.h File src/heap/heap-inl.h (right): https://codereview.chromium.org/2179033005/diff/80001/src/heap/heap-inl.h#new... src/heap/heap-inl.h:504: reinterpret_cast<SemiSpace*>(object_page->owner())->age_mark(); This is tricky: We need to pick the right age_mark: During the GC we are interested in from space (the space that is currently evacuated), during runtime we are interest in to space.
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 mlippautz@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.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== [heap] Don't consider mementos on pages below age mark Objects that reside below the age mark could be on pages that have been moved within new space. In this case mementos survived which can actually point to already-collected allocation sites. BUG=chromium:631050,chromium:581412 R=hpayer@chromium.org ========== to ========== [heap] Don't consider mementos on pages below age mark Objects that reside below the age mark could be on pages that have been moved within new space. In this case mementos survived which can actually point to already-collected allocation sites. BUG=chromium:631050,chromium:581412 R=hpayer@chromium.org Committed: https://crrev.com/e97b8686f234c4f49bec8f31bd1702d6908901cc Cr-Commit-Position: refs/heads/master@{#38094} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/e97b8686f234c4f49bec8f31bd1702d6908901cc Cr-Commit-Position: refs/heads/master@{#38094}
Message was sent while issue was closed.
lgtm |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
