Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1302423007:
[heap] Fix MemoryChunk::kHeaderSize computation and add some assertions. (Closed)
|
Created:
5 years, 3 months ago by Michael Lippautz Modified:
5 years, 3 months ago Reviewers:
Michael Starzinger CC:
v8-dev Base URL:
https://chromium.googlesource.com/v8/v8.git@master Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[heap] Fix MemoryChunk::kHeaderSize computation and add some assertions.
R=mstarzinger@chromium.org
Committed: https://crrev.com/aeb406849683c1cbf6b09e7fc4503613cbde828c
Cr-Commit-Position: refs/heads/master@{#30630}
Patch Set 1 #
Total comments: 1
Patch Set 2 : Make MemoryChunk a type following standard layout, which should enable offsetof #
Total comments: 6
Patch Set 3 : Addressed comments #Messages
Total messages: 17 (7 generated)
mlippautz@chromium.org changed reviewers: + mstarzinger@chromium.org
https://codereview.chromium.org/1302423007/diff/1/src/heap/spaces.h File src/heap/spaces.h (left): https://codereview.chromium.org/1302423007/diff/1/src/heap/spaces.h#oldcode558 src/heap/spaces.h:558: 5 * kPointerSize + // free list statistics 5 * kPointerSize implicitly included the memory that was needed to amount for alignment requirements. (Maybe the author new this, maybe not.)
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/patch-status/1302423007/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1302423007/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux_gcc_compile_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...)
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/patch-status/1302423007/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1302423007/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM, just nits and a suggestion. https://codereview.chromium.org/1302423007/diff/20001/src/heap/spaces.h File src/heap/spaces.h (right): https://codereview.chromium.org/1302423007/diff/20001/src/heap/spaces.h#newco... src/heap/spaces.h:563: kPointerSize // intptr_t write_barrier_counter_ nit: Shouldn't make a difference, but "kIntptrSize" here. https://codereview.chromium.org/1302423007/diff/20001/src/heap/spaces.h#newco... src/heap/spaces.h:574: static const size_t kHeaderSize = kMinHeaderSize + kPointerSize; As discussed offline: This looks like is makes the "markbits" only be 32bit aligned on 64bit machines. If that's the case then we should try and see if a padding of "kIntSize" here is sufficient, that in turn would make the "markbits" be 64bit aligned again. https://codereview.chromium.org/1302423007/diff/20001/src/heap/spaces.h#newco... src/heap/spaces.h:926: // Computed offsets should match the compiler generates ones. nit: s/generates/generated/
Thanks for clarifying the mark bits alignment. https://codereview.chromium.org/1302423007/diff/20001/src/heap/spaces.h File src/heap/spaces.h (right): https://codereview.chromium.org/1302423007/diff/20001/src/heap/spaces.h#newco... src/heap/spaces.h:563: kPointerSize // intptr_t write_barrier_counter_ On 2015/09/08 07:59:57, Michael Starzinger wrote: > nit: Shouldn't make a difference, but "kIntptrSize" here. Done. https://codereview.chromium.org/1302423007/diff/20001/src/heap/spaces.h#newco... src/heap/spaces.h:574: static const size_t kHeaderSize = kMinHeaderSize + kPointerSize; On 2015/09/08 07:59:57, Michael Starzinger wrote: > As discussed offline: This looks like is makes the "markbits" only be 32bit > aligned on 64bit machines. If that's the case then we should try and see if a > padding of "kIntSize" here is sufficient, that in turn would make the "markbits" > be 64bit aligned again. Done. https://codereview.chromium.org/1302423007/diff/20001/src/heap/spaces.h#newco... src/heap/spaces.h:926: // Computed offsets should match the compiler generates ones. On 2015/09/08 07:59:57, Michael Starzinger wrote: > nit: s/generates/generated/ Done.
The CQ bit was checked by mlippautz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mstarzinger@chromium.org Link to the patchset: https://codereview.chromium.org/1302423007/#ps40001 (title: "Addressed comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1302423007/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1302423007/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/aeb406849683c1cbf6b09e7fc4503613cbde828c Cr-Commit-Position: refs/heads/master@{#30630} |
