Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2015173003:
Address ThreadHeap::willObjectBeLazilySwept() corner case. (Closed)
|
Created:
4 years, 6 months ago by sof Modified:
4 years, 6 months ago CC:
chromium-reviews, blink-reviews, kouhei+heap_chromium.org, oilpan-reviews, Mads Ager (chromium) Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAddress ThreadHeap::willObjectBeLazilySwept() corner case.
If willObjectBeLazilySwept(object) was used when finalizing an object on a
lazily swept page, and |object| happened to reside on the same heap page,
the predicate would return the wrong result if the object had been swept
past (and it had been deemed to be alive.)
Addressed by adding a special case for querying objects on the same page,
making willObjectBeLazilySwept() precisely determine liveness in the
face of lazy sweeping.
R=
BUG=
Committed: https://crrev.com/197454b5a3762b4d303a16261f0e68a955070919
Cr-Commit-Position: refs/heads/master@{#396798}
Patch Set 1 #Patch Set 2 : comment + whitespace #Patch Set 3 : rebased #
Total comments: 11
Messages
Total messages: 24 (8 generated)
Description was changed from ========== Address ThreadHeap::willObjectBeLazilySwept() corner case. If willObjectBeLazilySwept(object) was used when finalizing an object on a lazily swept page, and |object| happened to reside on the same heap page, the predicate would return the wrong result if the object had been swept past (and it had been deemed to be alive.) Address this rare case by adding a special case for querying objects on the same page. Not a common case. R= BUG= ========== to ========== Address ThreadHeap::willObjectBeLazilySwept() corner case. If willObjectBeLazilySwept(object) was used when finalizing an object on a lazily swept page, and |object| happened to reside on the same heap page, the predicate would return the wrong result if the object had been swept past (and it had been deemed to be alive.) Addressed by adding a special case for querying objects on the same page, making willObjectBeLazilySwept() precisely determine liveness in the face of lazy sweeping. R= BUG= ==========
sigbjornf@opera.com changed reviewers: + oilpan-reviews@chromium.org, tkent@chromium.org
please take a look.
The CQ bit was checked by sigbjornf@opera.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2015173003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2015173003/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...) ios-simulator-gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)
haraken@chromium.org changed reviewers: + haraken@chromium.org
https://codereview.chromium.org/2015173003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/heap/Heap.h (right): https://codereview.chromium.org/2015173003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/heap/Heap.h:273: // such a reference. To check this, can we add objectPointer->checkHeader()? https://codereview.chromium.org/2015173003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/heap/HeapPage.cpp (right): https://codereview.chromium.org/2015173003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/heap/HeapPage.cpp:403: for (Address headerAddress = normalPage->payload(); headerAddress < pageEnd; ) { Instead of scanning the entire page, it would be better to remember m_headerAddressCurrentlyBeingLazilySwept and start scanning the page from m_headerAddressCurrentlyBeingLazilySwept. If objectPointer < m_headerAddressCurrentlyBeingLazilySwept, you can unconditionally return false. Otherwise, you can start scanning from m_headerAddressCurrentlyBeingLazilySwept. We can replace the m_isLazySweeping flag with m_headerAddressCurrentlyBeingLazilySwept. https://codereview.chromium.org/2015173003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/heap/HeapPage.cpp:415: if (!header->isFree() && header->isMarked()) { Hmm, I don't fully understand why this check is doing. I guess what we need is: if (headerAddress <= objectPointer && objectPointer < headerAddress) { return !header->isMarked(); }
On 2016/05/30 23:54:49, haraken wrote: > https://codereview.chromium.org/2015173003/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/platform/heap/Heap.h (right): > > https://codereview.chromium.org/2015173003/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/platform/heap/Heap.h:273: // such a reference. > > To check this, can we add objectPointer->checkHeader()? > > https://codereview.chromium.org/2015173003/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/platform/heap/HeapPage.cpp (right): > > https://codereview.chromium.org/2015173003/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/platform/heap/HeapPage.cpp:403: for (Address > headerAddress = normalPage->payload(); headerAddress < pageEnd; ) { > > Instead of scanning the entire page, it would be better to remember > m_headerAddressCurrentlyBeingLazilySwept and start scanning the page from > m_headerAddressCurrentlyBeingLazilySwept. > > If objectPointer < m_headerAddressCurrentlyBeingLazilySwept, you can > unconditionally return false. Otherwise, you can start scanning from > m_headerAddressCurrentlyBeingLazilySwept. > > We can replace the m_isLazySweeping flag with > m_headerAddressCurrentlyBeingLazilySwept. > > https://codereview.chromium.org/2015173003/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/platform/heap/HeapPage.cpp:415: if (!header->isFree() > && header->isMarked()) { > > Hmm, I don't fully understand why this check is doing. s/why/what/ > > I guess what we need is: > > if (headerAddress <= objectPointer && objectPointer < headerAddress) { > return !header->isMarked(); > }
https://codereview.chromium.org/2015173003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/heap/HeapPage.cpp (right): https://codereview.chromium.org/2015173003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/heap/HeapPage.cpp:403: for (Address headerAddress = normalPage->payload(); headerAddress < pageEnd; ) { On 2016/05/30 23:54:49, haraken wrote: > > Instead of scanning the entire page, it would be better to remember > m_headerAddressCurrentlyBeingLazilySwept and start scanning the page from > m_headerAddressCurrentlyBeingLazilySwept. > > If objectPointer < m_headerAddressCurrentlyBeingLazilySwept, you can > unconditionally return false. Otherwise, you can start scanning from > m_headerAddressCurrentlyBeingLazilySwept. > > We can replace the m_isLazySweeping flag with > m_headerAddressCurrentlyBeingLazilySwept. You can make trade offs like that, i.e., take the cost of maintaining a per-arena pointer value that's written to and kept up-to-date across all finalizations. It will not be read and used except in this rather very unusual case; which is why I elected not to. That was how I reasoned. https://codereview.chromium.org/2015173003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/heap/HeapPage.cpp:415: if (!header->isFree() && header->isMarked()) { On 2016/05/30 23:54:49, haraken wrote: > > Hmm, I don't fully understand why this check is doing. > > I guess what we need is: > > if (headerAddress <= objectPointer && objectPointer < headerAddress) { > return !header->isMarked(); > } > That won't work as you could be scanning freelist entries where checkHeader() will fail.
https://codereview.chromium.org/2015173003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/heap/HeapPage.cpp (right): https://codereview.chromium.org/2015173003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/heap/HeapPage.cpp:403: for (Address headerAddress = normalPage->payload(); headerAddress < pageEnd; ) { On 2016/05/31 05:13:23, sof wrote: > On 2016/05/30 23:54:49, haraken wrote: > > > > Instead of scanning the entire page, it would be better to remember > > m_headerAddressCurrentlyBeingLazilySwept and start scanning the page from > > m_headerAddressCurrentlyBeingLazilySwept. > > > > If objectPointer < m_headerAddressCurrentlyBeingLazilySwept, you can > > unconditionally return false. Otherwise, you can start scanning from > > m_headerAddressCurrentlyBeingLazilySwept. > > > > We can replace the m_isLazySweeping flag with > > m_headerAddressCurrentlyBeingLazilySwept. > > You can make trade offs like that, i.e., take the cost of maintaining a > per-arena pointer value that's written to and kept up-to-date across all > finalizations. It will not be read and used except in this rather very unusual > case; which is why I elected not to. That was how I reasoned. Yeah, that makes sense. The reason I didn't want to scan the area before m_headerAddressCurrentlyBeingLazilySwept is that the area may already be reused for new objects. But I realized that the current algorithm will just work for the area as well. https://codereview.chromium.org/2015173003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/heap/HeapPage.cpp:415: if (!header->isFree() && header->isMarked()) { On 2016/05/31 05:13:23, sof wrote: > On 2016/05/30 23:54:49, haraken wrote: > > > > Hmm, I don't fully understand why this check is doing. > > > > I guess what we need is: > > > > if (headerAddress <= objectPointer && objectPointer < headerAddress) { > > return !header->isMarked(); > > } > > > > That won't work as you could be scanning freelist entries where checkHeader() > will fail. Sorry, I meant: for (/* scan the page */) { if (headerAddress <= objectPointer && objectPointer < headerAddress + objectSize) { return !header->isFree() && !header->isMarked(); } } Why do you return true when '!header->isFree() && header->isMarked()'? I think that you need to return false in that case because the object is alive.
https://codereview.chromium.org/2015173003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/heap/HeapPage.cpp (right): https://codereview.chromium.org/2015173003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/heap/HeapPage.cpp:415: if (!header->isFree() && header->isMarked()) { On 2016/05/31 05:26:38, haraken wrote: > On 2016/05/31 05:13:23, sof wrote: > > On 2016/05/30 23:54:49, haraken wrote: > > > > > > Hmm, I don't fully understand why this check is doing. > > > > > > I guess what we need is: > > > > > > if (headerAddress <= objectPointer && objectPointer < headerAddress) { > > > return !header->isMarked(); > > > } > > > > > > > That won't work as you could be scanning freelist entries where checkHeader() > > will fail. > > Sorry, I meant: > > for (/* scan the page */) { > if (headerAddress <= objectPointer && objectPointer < headerAddress + > objectSize) { > return !header->isFree() && !header->isMarked(); > } > } > > Why do you return true when '!header->isFree() && header->isMarked()'? I think > that you need to return false in that case because the object is alive. No, we already know that objectPointer is unmarked. But if we hit a marked object before it, we do know that it is unmarked due to not having been marked (and not having been lazy-sweep processed already.) Hence it should return true.
https://codereview.chromium.org/2015173003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/heap/HeapPage.cpp (right): https://codereview.chromium.org/2015173003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/heap/HeapPage.cpp:320: if (m_firstUnsweptPage && !m_firstUnsweptPage->isLargeObjectPage()) { Why is it enough to check if the first unswept page is a large object page or not? The second unswept page can be a normal page. https://codereview.chromium.org/2015173003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/heap/HeapPage.cpp:415: if (!header->isFree() && header->isMarked()) { On 2016/05/31 05:32:25, sof wrote: > On 2016/05/31 05:26:38, haraken wrote: > > On 2016/05/31 05:13:23, sof wrote: > > > On 2016/05/30 23:54:49, haraken wrote: > > > > > > > > Hmm, I don't fully understand why this check is doing. > > > > > > > > I guess what we need is: > > > > > > > > if (headerAddress <= objectPointer && objectPointer < headerAddress) { > > > > return !header->isMarked(); > > > > } > > > > > > > > > > That won't work as you could be scanning freelist entries where > checkHeader() > > > will fail. > > > > Sorry, I meant: > > > > for (/* scan the page */) { > > if (headerAddress <= objectPointer && objectPointer < headerAddress + > > objectSize) { > > return !header->isFree() && !header->isMarked(); > > } > > } > > > > Why do you return true when '!header->isFree() && header->isMarked()'? I think > > that you need to return false in that case because the object is alive. > > No, we already know that objectPointer is unmarked. But if we hit a marked > object before it, we do know that it is unmarked due to not having been marked > (and not having been lazy-sweep processed already.) Hence it should return true. OK. Do you need to check !header->isFree()? If header->isMarked() returns true, header->isFree() should return false.
On 2016/05/31 05:41:16, haraken wrote: > https://codereview.chromium.org/2015173003/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/platform/heap/HeapPage.cpp (right): > > https://codereview.chromium.org/2015173003/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/platform/heap/HeapPage.cpp:320: if (m_firstUnsweptPage > && !m_firstUnsweptPage->isLargeObjectPage()) { > > Why is it enough to check if the first unswept page is a large object page or > not? The second unswept page can be a normal page. > > https://codereview.chromium.org/2015173003/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/platform/heap/HeapPage.cpp:415: if (!header->isFree() > && header->isMarked()) { > On 2016/05/31 05:32:25, sof wrote: > > On 2016/05/31 05:26:38, haraken wrote: > > > On 2016/05/31 05:13:23, sof wrote: > > > > On 2016/05/30 23:54:49, haraken wrote: > > > > > > > > > > Hmm, I don't fully understand why this check is doing. > > > > > > > > > > I guess what we need is: > > > > > > > > > > if (headerAddress <= objectPointer && objectPointer < headerAddress) { > > > > > return !header->isMarked(); > > > > > } > > > > > > > > > > > > > That won't work as you could be scanning freelist entries where > > checkHeader() > > > > will fail. > > > > > > Sorry, I meant: > > > > > > for (/* scan the page */) { > > > if (headerAddress <= objectPointer && objectPointer < headerAddress + > > > objectSize) { > > > return !header->isFree() && !header->isMarked(); > > > } > > > } > > > > > > Why do you return true when '!header->isFree() && header->isMarked()'? I > think > > > that you need to return false in that case because the object is alive. > > > > No, we already know that objectPointer is unmarked. But if we hit a marked > > object before it, we do know that it is unmarked due to not having been marked > > (and not having been lazy-sweep processed already.) Hence it should return > true. > > OK. Do you need to check !header->isFree()? If header->isMarked() returns true, > header->isFree() should return false. Covered by https://codereview.chromium.org/2015173003/#msg11 ?
On 2016/05/31 05:51:50, sof wrote: > On 2016/05/31 05:41:16, haraken wrote: > > > https://codereview.chromium.org/2015173003/diff/40001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/platform/heap/HeapPage.cpp (right): > > > > > https://codereview.chromium.org/2015173003/diff/40001/third_party/WebKit/Sour... > > third_party/WebKit/Source/platform/heap/HeapPage.cpp:320: if > (m_firstUnsweptPage > > && !m_firstUnsweptPage->isLargeObjectPage()) { > > > > Why is it enough to check if the first unswept page is a large object page or > > not? The second unswept page can be a normal page. > > > > > https://codereview.chromium.org/2015173003/diff/40001/third_party/WebKit/Sour... > > third_party/WebKit/Source/platform/heap/HeapPage.cpp:415: if > (!header->isFree() > > && header->isMarked()) { > > On 2016/05/31 05:32:25, sof wrote: > > > On 2016/05/31 05:26:38, haraken wrote: > > > > On 2016/05/31 05:13:23, sof wrote: > > > > > On 2016/05/30 23:54:49, haraken wrote: > > > > > > > > > > > > Hmm, I don't fully understand why this check is doing. > > > > > > > > > > > > I guess what we need is: > > > > > > > > > > > > if (headerAddress <= objectPointer && objectPointer < headerAddress) > { > > > > > > return !header->isMarked(); > > > > > > } > > > > > > > > > > > > > > > > That won't work as you could be scanning freelist entries where > > > checkHeader() > > > > > will fail. > > > > > > > > Sorry, I meant: > > > > > > > > for (/* scan the page */) { > > > > if (headerAddress <= objectPointer && objectPointer < headerAddress + > > > > objectSize) { > > > > return !header->isFree() && !header->isMarked(); > > > > } > > > > } > > > > > > > > Why do you return true when '!header->isFree() && header->isMarked()'? I > > think > > > > that you need to return false in that case because the object is alive. > > > > > > No, we already know that objectPointer is unmarked. But if we hit a marked > > > object before it, we do know that it is unmarked due to not having been > marked > > > (and not having been lazy-sweep processed already.) Hence it should return > > true. > > > > OK. Do you need to check !header->isFree()? If header->isMarked() returns > true, > > header->isFree() should return false. > > Covered by https://codereview.chromium.org/2015173003/#msg11 ? Makes sense.
https://codereview.chromium.org/2015173003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/heap/HeapPage.cpp (right): https://codereview.chromium.org/2015173003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/heap/HeapPage.cpp:320: if (m_firstUnsweptPage && !m_firstUnsweptPage->isLargeObjectPage()) { On 2016/05/31 05:41:16, haraken wrote: > > Why is it enough to check if the first unswept page is a large object page or > not? The second unswept page can be a normal page. > How can an arena contain a mixture of large and normal pages?
On 2016/05/31 07:24:09, sof wrote: > https://codereview.chromium.org/2015173003/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/platform/heap/HeapPage.cpp (right): > > https://codereview.chromium.org/2015173003/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/platform/heap/HeapPage.cpp:320: if (m_firstUnsweptPage > && !m_firstUnsweptPage->isLargeObjectPage()) { > On 2016/05/31 05:41:16, haraken wrote: > > > > Why is it enough to check if the first unswept page is a large object page or > > not? The second unswept page can be a normal page. > > > > How can an arena contain a mixture of large and normal pages? You're right :) LGTM.
The CQ bit was checked by sigbjornf@opera.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2015173003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2015173003/40001
Message was sent while issue was closed.
Description was changed from ========== Address ThreadHeap::willObjectBeLazilySwept() corner case. If willObjectBeLazilySwept(object) was used when finalizing an object on a lazily swept page, and |object| happened to reside on the same heap page, the predicate would return the wrong result if the object had been swept past (and it had been deemed to be alive.) Addressed by adding a special case for querying objects on the same page, making willObjectBeLazilySwept() precisely determine liveness in the face of lazy sweeping. R= BUG= ========== to ========== Address ThreadHeap::willObjectBeLazilySwept() corner case. If willObjectBeLazilySwept(object) was used when finalizing an object on a lazily swept page, and |object| happened to reside on the same heap page, the predicate would return the wrong result if the object had been swept past (and it had been deemed to be alive.) Addressed by adding a special case for querying objects on the same page, making willObjectBeLazilySwept() precisely determine liveness in the face of lazy sweeping. R= BUG= ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Address ThreadHeap::willObjectBeLazilySwept() corner case. If willObjectBeLazilySwept(object) was used when finalizing an object on a lazily swept page, and |object| happened to reside on the same heap page, the predicate would return the wrong result if the object had been swept past (and it had been deemed to be alive.) Addressed by adding a special case for querying objects on the same page, making willObjectBeLazilySwept() precisely determine liveness in the face of lazy sweeping. R= BUG= ========== to ========== Address ThreadHeap::willObjectBeLazilySwept() corner case. If willObjectBeLazilySwept(object) was used when finalizing an object on a lazily swept page, and |object| happened to reside on the same heap page, the predicate would return the wrong result if the object had been swept past (and it had been deemed to be alive.) Addressed by adding a special case for querying objects on the same page, making willObjectBeLazilySwept() precisely determine liveness in the face of lazy sweeping. R= BUG= Committed: https://crrev.com/197454b5a3762b4d303a16261f0e68a955070919 Cr-Commit-Position: refs/heads/master@{#396798} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/197454b5a3762b4d303a16261f0e68a955070919 Cr-Commit-Position: refs/heads/master@{#396798} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
