Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
850063002:
Do not fire timers for finalizing objects. (Closed)
DescriptionDo not fire timers for finalizing objects.
If a lazy sweep is in progress and a timer fires, do check that
the underlying object is still alive before invoking its method.
R=haraken
BUG=448392
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=188577
Patch Set 1 #
Total comments: 5
Patch Set 2 : Renamed predicate to isFinalizedObjectAlive() #Patch Set 3 : Support non-Oilpan use #Patch Set 4 : Mark pages as lazily swept #
Total comments: 1
Patch Set 5 : Handle mixins #Patch Set 6 : Parameterize isFinalizedObjectAlive() over object type #Patch Set 7 : export Heap::hasBeenLazilySwept() #
Total comments: 3
Patch Set 8 : blind Windows compilation fix #Patch Set 9 : Reset sweep status on pages before sweeping instead #
Total comments: 29
Patch Set 10 : tweak some flag setting #Patch Set 11 : Add some asserts, tweak logic #Patch Set 12 : have objectPayloadSizeForTesting mark pages as swept also #
Total comments: 1
Messages
Total messages: 45 (3 generated)
sigbjornf@opera.com changed reviewers: + oilpan-reviews@chromium.org
Please take a look.
haraken@chromium.org changed reviewers: + haraken@chromium.org
Thanks for looking into the crash! I understand the problem but I'm not sure if this is a right fix, although I don't have any better idea at the moment. Ideally we should clear all timers in pre-finalizers, but it would be too heavy and thus unacceptable. Let me think about a bit more. https://codereview.chromium.org/850063002/diff/1/Source/platform/heap/Heap.cpp File Source/platform/heap/Heap.cpp (right): https://codereview.chromium.org/850063002/diff/1/Source/platform/heap/Heap.cp... Source/platform/heap/Heap.cpp:2133: return s_markingVisitor->isMarked(objectPointer); Hmm, what happens in the following case? (1) Marking marks object A and B. (2) Lazy sweeping unmarks A. (3) isObjectAlive(X) is called. (4) Lazy sweeping unmarks B. (5) Lazy sweeping is complete. In this case, isObjectAlive(X) returns false if X==A, but returns true if X==B. This seems wrong.
https://codereview.chromium.org/850063002/diff/1/Source/platform/heap/Heap.cpp File Source/platform/heap/Heap.cpp (right): https://codereview.chromium.org/850063002/diff/1/Source/platform/heap/Heap.cp... Source/platform/heap/Heap.cpp:2133: return s_markingVisitor->isMarked(objectPointer); On 2015/01/14 14:29:08, haraken wrote: > > Hmm, what happens in the following case? > > (1) Marking marks object A and B. > (2) Lazy sweeping unmarks A. > (3) isObjectAlive(X) is called. > (4) Lazy sweeping unmarks B. > (5) Lazy sweeping is complete. > > In this case, isObjectAlive(X) returns false if X==A, but returns true if X==B. > This seems wrong. I suppose we can look at the page & see if it has yet to be swept. If we can assume no cross thread access here, that would do it.
https://codereview.chromium.org/850063002/diff/1/Source/platform/heap/Heap.cpp File Source/platform/heap/Heap.cpp (right): https://codereview.chromium.org/850063002/diff/1/Source/platform/heap/Heap.cp... Source/platform/heap/Heap.cpp:2133: return s_markingVisitor->isMarked(objectPointer); On 2015/01/14 14:36:19, sof wrote: > On 2015/01/14 14:29:08, haraken wrote: > > > > Hmm, what happens in the following case? > > > > (1) Marking marks object A and B. > > (2) Lazy sweeping unmarks A. > > (3) isObjectAlive(X) is called. > > (4) Lazy sweeping unmarks B. > > (5) Lazy sweeping is complete. > > > > In this case, isObjectAlive(X) returns false if X==A, but returns true if > X==B. > > This seems wrong. > > I suppose we can look at the page & see if it has yet to be swept. If we can > assume no cross thread access here, that would do it. That would work :) Also should we use isMarked or isAlive? (I haven't studied code.)
https://codereview.chromium.org/850063002/diff/1/Source/platform/heap/Heap.cpp File Source/platform/heap/Heap.cpp (right): https://codereview.chromium.org/850063002/diff/1/Source/platform/heap/Heap.cp... Source/platform/heap/Heap.cpp:2133: return s_markingVisitor->isMarked(objectPointer); On 2015/01/14 14:37:44, haraken wrote: > On 2015/01/14 14:36:19, sof wrote: > > On 2015/01/14 14:29:08, haraken wrote: > > > > > > Hmm, what happens in the following case? > > > > > > (1) Marking marks object A and B. > > > (2) Lazy sweeping unmarks A. > > > (3) isObjectAlive(X) is called. > > > (4) Lazy sweeping unmarks B. > > > (5) Lazy sweeping is complete. > > > > > > In this case, isObjectAlive(X) returns false if X==A, but returns true if > > X==B. > > > This seems wrong. > > > > I suppose we can look at the page & see if it has yet to be swept. If we can > > assume no cross thread access here, that would do it. > > That would work :) > > Also should we use isMarked or isAlive? (I haven't studied code.) Actually, if we constrain ourselves to GC objects with finalizers, the CL as is (ps#1) is sufficient. The method could have a better name in that case though. Can be generalized later (but hopefully we won't have to.)
https://codereview.chromium.org/850063002/diff/1/Source/platform/heap/Heap.cpp File Source/platform/heap/Heap.cpp (right): https://codereview.chromium.org/850063002/diff/1/Source/platform/heap/Heap.cp... Source/platform/heap/Heap.cpp:2133: return s_markingVisitor->isMarked(objectPointer); On 2015/01/14 15:53:32, sof wrote: > On 2015/01/14 14:37:44, haraken wrote: > > On 2015/01/14 14:36:19, sof wrote: > > > On 2015/01/14 14:29:08, haraken wrote: > > > > > > > > Hmm, what happens in the following case? > > > > > > > > (1) Marking marks object A and B. > > > > (2) Lazy sweeping unmarks A. > > > > (3) isObjectAlive(X) is called. > > > > (4) Lazy sweeping unmarks B. > > > > (5) Lazy sweeping is complete. > > > > > > > > In this case, isObjectAlive(X) returns false if X==A, but returns true if > > > X==B. > > > > This seems wrong. > > > > > > I suppose we can look at the page & see if it has yet to be swept. If we can > > > assume no cross thread access here, that would do it. > > > > That would work :) > > > > Also should we use isMarked or isAlive? (I haven't studied code.) > > Actually, if we constrain ourselves to GC objects with finalizers, the CL as is > (ps#1) is sufficient. The method could have a better name in that case though. > > Can be generalized later (but hopefully we won't have to.) Renamed to isFinalizedObjectAlive() + clarified that the predicate isn't supported over already finalized references.
Hmm, seeing a bunch of tests time out with this protective measure of not invoking timer methods on not-as-yet finalized objects.
Just to clarify the real issue here -- it isn't directly a problem to invoke the timer object's method if the object is as yet unswept. The problem is what that to-be-swept object might reference and access in that timer-fired method. Some or all of its reachable GCed objects may have been finalized by now.
On 2015/01/14 22:09:47, sof wrote: > Just to clarify the real issue here -- it isn't directly a problem to invoke the > timer object's method if the object is as yet unswept. The problem is what that > to-be-swept object might reference and access in that timer-fired method. Some > or all of its reachable GCed objects may have been finalized by now. Thanks, understood the problem. Recently I begin to think that we should allow destructors (and those timers) to touch other on-heap objects. This is technically doable by sweeping our heap twice -- the first sweep just calls finalizers in our heap without clearing out the memory and the second sweep actually clears out the memory (I guess Boehm GC is doing something like this). The restriction that destructors cannot touch on-heap objects has added a lot of complicated oilpan-specific infrastructures (e.g., a lot of dispose methods, pre-finalizer etc) to the code base and that is one of the points people are complaining about oilpan. Lazy sweeping made the situation worse. We should probably spend some time to experiment if the two-phase sweeping is acceptable in terms of performance. This is something we should consider in long term (but before actually shipping Oilpan for large hierarchies). In the particular case of this bug, I'm fine with any short-term hack to address the issue.
On 2015/01/15 01:27:24, haraken wrote: > On 2015/01/14 22:09:47, sof wrote: > > Just to clarify the real issue here -- it isn't directly a problem to invoke > the > > timer object's method if the object is as yet unswept. The problem is what > that > > to-be-swept object might reference and access in that timer-fired method. Some > > or all of its reachable GCed objects may have been finalized by now. > > Thanks, understood the problem. > > Recently I begin to think that we should allow destructors (and those timers) to > touch other on-heap objects. This is technically doable by sweeping our heap > twice -- the first sweep just calls finalizers in our heap without clearing out > the memory and the second sweep actually clears out the memory (I guess Boehm GC > is doing something like this). The restriction that destructors cannot touch > on-heap objects has added a lot of complicated oilpan-specific infrastructures > (e.g., a lot of dispose methods, pre-finalizer etc) to the code base and that is > one of the points people are complaining about oilpan. Lazy sweeping made the > situation worse. We should probably spend some time to experiment if the > two-phase sweeping is acceptable in terms of performance. This is something we > should consider in long term (but before actually shipping Oilpan for large > hierarchies). Hmm, the two-phase sweep won't work, because it will end up allowing a finalizer to touch another on-heap object whose finalizer is already called...
On 2015/01/15 01:27:24, haraken wrote: > On 2015/01/14 22:09:47, sof wrote: > > Just to clarify the real issue here -- it isn't directly a problem to invoke > the > > timer object's method if the object is as yet unswept. The problem is what > that > > to-be-swept object might reference and access in that timer-fired method. Some > > or all of its reachable GCed objects may have been finalized by now. > > Thanks, understood the problem. > > Recently I begin to think that we should allow destructors (and those timers) to > touch other on-heap objects. This is technically doable by sweeping our heap > twice -- the first sweep just calls finalizers in our heap without clearing out > the memory and the second sweep actually clears out the memory (I guess Boehm GC > is doing something like this). The restriction that destructors cannot touch > on-heap objects has added a lot of complicated oilpan-specific infrastructures > (e.g., a lot of dispose methods, pre-finalizer etc) to the code base and that is > one of the points people are complaining about oilpan. Lazy sweeping made the > situation worse. We should probably spend some time to experiment if the > two-phase sweeping is acceptable in terms of performance. This is something we > should consider in long term (but before actually shipping Oilpan for large > hierarchies). > > In the particular case of this bug, I'm fine with any short-term hack to address > the issue. I'll continue looking; the dual of adding keepalive Persistents while timers are active is too leakprone, i think.
On 2015/01/15 06:16:19, sof wrote: > On 2015/01/15 01:27:24, haraken wrote: > > On 2015/01/14 22:09:47, sof wrote: > > > Just to clarify the real issue here -- it isn't directly a problem to invoke > > the > > > timer object's method if the object is as yet unswept. The problem is what > > that > > > to-be-swept object might reference and access in that timer-fired method. > Some > > > or all of its reachable GCed objects may have been finalized by now. > > > > Thanks, understood the problem. > > > > Recently I begin to think that we should allow destructors (and those timers) > to > > touch other on-heap objects. This is technically doable by sweeping our heap > > twice -- the first sweep just calls finalizers in our heap without clearing > out > > the memory and the second sweep actually clears out the memory (I guess Boehm > GC > > is doing something like this). The restriction that destructors cannot touch > > on-heap objects has added a lot of complicated oilpan-specific infrastructures > > (e.g., a lot of dispose methods, pre-finalizer etc) to the code base and that > is > > one of the points people are complaining about oilpan. Lazy sweeping made the > > situation worse. We should probably spend some time to experiment if the > > two-phase sweeping is acceptable in terms of performance. This is something we > > should consider in long term (but before actually shipping Oilpan for large > > hierarchies). > > > > In the particular case of this bug, I'm fine with any short-term hack to > address > > the issue. > > I'll continue looking; the dual of adding keepalive Persistents while timers are > active is too leakprone, i think. That might be a workable alternative, and a saner option. Checking for regressions/issues via https://codereview.chromium.org/856493002/ (please disregard all changes outside of Timer.*; they will all be gone if this turns out workable & worth the effort.)
> Hmm, seeing a bunch of tests time out with this protective measure of not > invoking timer methods on not-as-yet finalized objects. Would you help me understand why it's not OK to just not invoke timer methods for about-to-be-swept objects? Does it mean that even if we stop timers in pre-finalizers of timer owners, it won't help to fix the problem? (I don't want to add pre-finalizers to all timer owners, but I want to confirm my understanding.)
On 2015/01/15 15:42:26, haraken wrote: > > Hmm, seeing a bunch of tests time out with this protective measure of not > > invoking timer methods on not-as-yet finalized objects. > > Would you help me understand why it's not OK to just not invoke timer methods > for about-to-be-swept objects? Does it mean that even if we stop timers in > pre-finalizers of timer owners, it won't help to fix the problem? (I don't want > to add pre-finalizers to all timer owners, but I want to confirm my > understanding.) I might well be misinterpreting your question, but wasn't that problem explained in https://codereview.chromium.org/850063002/#msg10 ? Prefinalizers are different though, as they run before anything is swept&finalized, but stopping a timer wouldn't involve touching a heap object in any case.
On 2015/01/15 11:01:45, sof wrote: > On 2015/01/15 06:16:19, sof wrote: > > On 2015/01/15 01:27:24, haraken wrote: > > > On 2015/01/14 22:09:47, sof wrote: > > > > Just to clarify the real issue here -- it isn't directly a problem to > invoke > > > the > > > > timer object's method if the object is as yet unswept. The problem is what > > > that > > > > to-be-swept object might reference and access in that timer-fired method. > > Some > > > > or all of its reachable GCed objects may have been finalized by now. > > > > > > Thanks, understood the problem. > > > > > > Recently I begin to think that we should allow destructors (and those > timers) > > to > > > touch other on-heap objects. This is technically doable by sweeping our heap > > > twice -- the first sweep just calls finalizers in our heap without clearing > > out > > > the memory and the second sweep actually clears out the memory (I guess > Boehm > > GC > > > is doing something like this). The restriction that destructors cannot touch > > > on-heap objects has added a lot of complicated oilpan-specific > infrastructures > > > (e.g., a lot of dispose methods, pre-finalizer etc) to the code base and > that > > is > > > one of the points people are complaining about oilpan. Lazy sweeping made > the > > > situation worse. We should probably spend some time to experiment if the > > > two-phase sweeping is acceptable in terms of performance. This is something > we > > > should consider in long term (but before actually shipping Oilpan for large > > > hierarchies). > > > > > > In the particular case of this bug, I'm fine with any short-term hack to > > address > > > the issue. > > > > I'll continue looking; the dual of adding keepalive Persistents while timers > are > > active is too leakprone, i think. > > That might be a workable alternative, and a saner option. > > Checking for regressions/issues via https://codereview.chromium.org/856493002/ > (please disregard all changes outside of Timer.*; they will all be gone if this > turns out workable & worth the effort.) Still waiting for complete test results, but I reckon this one will work out. Seeing a couple of "leak" tests that report slightly values, but that's more of a testing issue.
On 2015/01/15 15:47:31, sof wrote: > On 2015/01/15 15:42:26, haraken wrote: > > > Hmm, seeing a bunch of tests time out with this protective measure of not > > > invoking timer methods on not-as-yet finalized objects. > > > > Would you help me understand why it's not OK to just not invoke timer methods > > for about-to-be-swept objects? Does it mean that even if we stop timers in > > pre-finalizers of timer owners, it won't help to fix the problem? (I don't > want > > to add pre-finalizers to all timer owners, but I want to confirm my > > understanding.) > > I might well be misinterpreting your question, but wasn't that problem explained > in https://codereview.chromium.org/850063002/#msg10 ? I understand the problem is that some timer function can touch some on-heap objects. My question is what happens if we just don't invoke any timer function of a to-be-swept timer owner? If we don't invoke a timer function, then we won't hit the issue. I guess you're saying in #9 that the approach doesn't work and I'm wondering why. > > Prefinalizers are different though, as they run before anything is > swept&finalized, but stopping a timer wouldn't involve touching a heap object in > any case.
On 2015/01/15 16:04:02, haraken wrote: > On 2015/01/15 15:47:31, sof wrote: > > On 2015/01/15 15:42:26, haraken wrote: > > > > Hmm, seeing a bunch of tests time out with this protective measure of not > > > > invoking timer methods on not-as-yet finalized objects. > > > > > > Would you help me understand why it's not OK to just not invoke timer > methods > > > for about-to-be-swept objects? Does it mean that even if we stop timers in > > > pre-finalizers of timer owners, it won't help to fix the problem? (I don't > > want > > > to add pre-finalizers to all timer owners, but I want to confirm my > > > understanding.) > > > > I might well be misinterpreting your question, but wasn't that problem > explained > > in https://codereview.chromium.org/850063002/#msg10 ? > > I understand the problem is that some timer function can touch some on-heap > objects. My question is what happens if we just don't invoke any timer function > of a to-be-swept timer owner? If we don't invoke a timer function, then we won't > hit the issue. > > I guess you're saying in #9 that the approach doesn't work and I'm wondering > why. > My theory was that ongoing timers fails to notify off-heap objects of the outcome. Calling stop() rather than ignoring might be an alternative, but I haven't investigated. This is time consuming enough as it is.
On 2015/01/15 16:09:49, sof wrote: > On 2015/01/15 16:04:02, haraken wrote: > > On 2015/01/15 15:47:31, sof wrote: > > > On 2015/01/15 15:42:26, haraken wrote: > > > > > Hmm, seeing a bunch of tests time out with this protective measure of > not > > > > > invoking timer methods on not-as-yet finalized objects. > > > > > > > > Would you help me understand why it's not OK to just not invoke timer > > methods > > > > for about-to-be-swept objects? Does it mean that even if we stop timers in > > > > pre-finalizers of timer owners, it won't help to fix the problem? (I don't > > > want > > > > to add pre-finalizers to all timer owners, but I want to confirm my > > > > understanding.) > > > > > > I might well be misinterpreting your question, but wasn't that problem > > explained > > > in https://codereview.chromium.org/850063002/#msg10 ? > > > > I understand the problem is that some timer function can touch some on-heap > > objects. My question is what happens if we just don't invoke any timer > function > > of a to-be-swept timer owner? If we don't invoke a timer function, then we > won't > > hit the issue. > > > > I guess you're saying in #9 that the approach doesn't work and I'm wondering > > why. > > > > My theory was that ongoing timers fails to notify off-heap objects of the > outcome. > > Calling stop() rather than ignoring might be an alternative, but I haven't > investigated. This is time consuming enough as it is. Hmm, I'm not sure how calling stop() is different from ignoring. stop() just does "please don't invoke the timer any more", which seems to be equivalent with just ignoring the timers. (So my question is still why just ignoring timers doesn't work.) The persistent approach looks ok but I'm a bit afraid of the complexity (so want to convinced about the complexity).
On 2015/01/15 16:30:27, haraken wrote: > On 2015/01/15 16:09:49, sof wrote: > > On 2015/01/15 16:04:02, haraken wrote: > > > On 2015/01/15 15:47:31, sof wrote: > > > > On 2015/01/15 15:42:26, haraken wrote: > > > > > > Hmm, seeing a bunch of tests time out with this protective measure of > > not > > > > > > invoking timer methods on not-as-yet finalized objects. > > > > > > > > > > Would you help me understand why it's not OK to just not invoke timer > > > methods > > > > > for about-to-be-swept objects? Does it mean that even if we stop timers > in > > > > > pre-finalizers of timer owners, it won't help to fix the problem? (I > don't > > > > want > > > > > to add pre-finalizers to all timer owners, but I want to confirm my > > > > > understanding.) > > > > > > > > I might well be misinterpreting your question, but wasn't that problem > > > explained > > > > in https://codereview.chromium.org/850063002/#msg10 ? > > > > > > I understand the problem is that some timer function can touch some on-heap > > > objects. My question is what happens if we just don't invoke any timer > > function > > > of a to-be-swept timer owner? If we don't invoke a timer function, then we > > won't > > > hit the issue. > > > > > > I guess you're saying in #9 that the approach doesn't work and I'm wondering > > > why. > > > > > > > My theory was that ongoing timers fails to notify off-heap objects of the > > outcome. > > > > Calling stop() rather than ignoring might be an alternative, but I haven't > > investigated. This is time consuming enough as it is. > > Hmm, I'm not sure how calling stop() is different from ignoring. stop() just > does "please don't invoke the timer any more", which seems to be equivalent with > just ignoring the timers. (So my question is still why just ignoring timers > doesn't work.) > > The persistent approach looks ok but I'm a bit afraid of the complexity (so want > to convinced about the complexity). Hmm. I'm not going to spend yet another day investigating details.
The reasoning behind https://codereview.chromium.org/850063002/#msg7 is somewhat subtle, but I realized it was wrong/incomplete while going over the details here again. i.e., pages need to be marked as they're lazily swept so as to be able to correctly interpret the mark bit in isFinalizedObjectAlive(). The latest patchset does that. There are two test failures to contend with; will try to have a look at those tomorrow. So, either CL is on offer here now to address timer handling, I think.
I'll take a detailed look after the remaining issues are fixed. I think this CL is a good way to go. https://codereview.chromium.org/850063002/diff/60001/Source/platform/heap/Hea... File Source/platform/heap/Heap.cpp (right): https://codereview.chromium.org/850063002/diff/60001/Source/platform/heap/Hea... Source/platform/heap/Heap.cpp:2156: bool Heap::isFinalizedObjectAlive(const void* objectPointer) I like having this method in our heap infrastructure -- it would be useful to catch various errors. For example, we can add ASSERT(!isFinalizedObjectAlive(m_raw)) to Member::operator-> to detect code that is accessing on-heap objects in destructors etc.
On 2015/01/16 00:57:10, haraken wrote: > I'll take a detailed look after the remaining issues are fixed. I think this CL > is a good way to go. > Fixed now; hadn't accounted for mixins.
On 2015/01/16 09:04:45, sof wrote: > On 2015/01/16 00:57:10, haraken wrote: > > I'll take a detailed look after the remaining issues are fixed. I think this > CL > > is a good way to go. > > > > Fixed now; hadn't accounted for mixins. Parameterized the predicate over the GC class type, easier to use (and no issues with supporting this in a component build, which is why I held off initially.)
Thanks for working on this. The approach looks good to me. https://codereview.chromium.org/850063002/diff/120001/Source/platform/heap/He... File Source/platform/heap/Heap.cpp (right): https://codereview.chromium.org/850063002/diff/120001/Source/platform/heap/He... Source/platform/heap/Heap.cpp:1266: return currentToken == (m_sweepStatus == 2); The token looks a bit tricky to me. Can we just define BaseHeapPage::m_unswept and set the flag to true in prepareForSweeping()? (We'll need to scan the list of pages in prepareForSweeping(), but the overhead would be ignorable.) The invariant is that BaseHeapPage::m_unswept is true if and only if the BaseHeapPage is in a linked list of m_unsweptPages or m_unsweptLargeObjects.
https://codereview.chromium.org/850063002/diff/120001/Source/platform/heap/He... File Source/platform/heap/Heap.cpp (right): https://codereview.chromium.org/850063002/diff/120001/Source/platform/heap/He... Source/platform/heap/Heap.cpp:1266: return currentToken == (m_sweepStatus == 2); On 2015/01/16 13:40:01, haraken wrote: > > The token looks a bit tricky to me. Can we just define BaseHeapPage::m_unswept > and set the flag to true in prepareForSweeping()? (We'll need to scan the list > of pages in prepareForSweeping(), but the overhead would be ignorable.) The > invariant is that BaseHeapPage::m_unswept is true if and only if the > BaseHeapPage is in a linked list of m_unsweptPages or m_unsweptLargeObjects. I elected not to scan all pages (previously swept and unswept) and update this field. That's why I did it this way.
https://codereview.chromium.org/850063002/diff/120001/Source/platform/heap/He... File Source/platform/heap/Heap.cpp (right): https://codereview.chromium.org/850063002/diff/120001/Source/platform/heap/He... Source/platform/heap/Heap.cpp:1266: return currentToken == (m_sweepStatus == 2); On 2015/01/16 13:43:10, sof wrote: > On 2015/01/16 13:40:01, haraken wrote: > > > > The token looks a bit tricky to me. Can we just define BaseHeapPage::m_unswept > > and set the flag to true in prepareForSweeping()? (We'll need to scan the list > > of pages in prepareForSweeping(), but the overhead would be ignorable.) The > > invariant is that BaseHeapPage::m_unswept is true if and only if the > > BaseHeapPage is in a linked list of m_unsweptPages or m_unsweptLargeObjects. > > I elected not to scan all pages (previously swept and unswept) and update this > field. That's why I did it this way. If you need to go with the flip bit, I guess you'll also need to re-flip the bit in markUnmarkedObjectsDead(). (That said, it seems a bit complicated, so if it doesn't matter for performance, I'd prefer just scanning all pages in prepareForSweep.)
On 2015/01/16 13:54:55, haraken wrote: > https://codereview.chromium.org/850063002/diff/120001/Source/platform/heap/He... > File Source/platform/heap/Heap.cpp (right): > > https://codereview.chromium.org/850063002/diff/120001/Source/platform/heap/He... > Source/platform/heap/Heap.cpp:1266: return currentToken == (m_sweepStatus == 2); > On 2015/01/16 13:43:10, sof wrote: > > On 2015/01/16 13:40:01, haraken wrote: > > > > > > The token looks a bit tricky to me. Can we just define > BaseHeapPage::m_unswept > > > and set the flag to true in prepareForSweeping()? (We'll need to scan the > list > > > of pages in prepareForSweeping(), but the overhead would be ignorable.) The > > > invariant is that BaseHeapPage::m_unswept is true if and only if the > > > BaseHeapPage is in a linked list of m_unsweptPages or m_unsweptLargeObjects. > > > > I elected not to scan all pages (previously swept and unswept) and update this > > field. That's why I did it this way. > > If you need to go with the flip bit, I guess you'll also need to re-flip the bit > in markUnmarkedObjectsDead(). (That said, it seems a bit complicated, so if it > doesn't matter for performance, I'd prefer just scanning all pages in > prepareForSweep.) Don't think so, why would you have to flip it there?
On 2015/01/16 14:07:19, sof wrote: > On 2015/01/16 13:54:55, haraken wrote: > > > https://codereview.chromium.org/850063002/diff/120001/Source/platform/heap/He... > > File Source/platform/heap/Heap.cpp (right): > > > > > https://codereview.chromium.org/850063002/diff/120001/Source/platform/heap/He... > > Source/platform/heap/Heap.cpp:1266: return currentToken == (m_sweepStatus == > 2); > > On 2015/01/16 13:43:10, sof wrote: > > > On 2015/01/16 13:40:01, haraken wrote: > > > > > > > > The token looks a bit tricky to me. Can we just define > > BaseHeapPage::m_unswept > > > > and set the flag to true in prepareForSweeping()? (We'll need to scan the > > list > > > > of pages in prepareForSweeping(), but the overhead would be ignorable.) > The > > > > invariant is that BaseHeapPage::m_unswept is true if and only if the > > > > BaseHeapPage is in a linked list of m_unsweptPages or > m_unsweptLargeObjects. > > > > > > I elected not to scan all pages (previously swept and unswept) and update > this > > > field. That's why I did it this way. > > > > If you need to go with the flip bit, I guess you'll also need to re-flip the > bit > > in markUnmarkedObjectsDead(). (That said, it seems a bit complicated, so if it > > doesn't matter for performance, I'd prefer just scanning all pages in > > prepareForSweep.) > > Don't think so, why would you have to flip it there? Probably I'm a bit confused. What's an invariant about the m_sweepStatus? I wonder why we need to maintain 3 states (0 or 1 or 2). I guess what we need is a way to know whether a given page is in a list of m_firstPage or m_unsweptFirstPage.
On 2015/01/16 14:30:45, haraken wrote: > On 2015/01/16 14:07:19, sof wrote: > > On 2015/01/16 13:54:55, haraken wrote: > > > > > > https://codereview.chromium.org/850063002/diff/120001/Source/platform/heap/He... > > > File Source/platform/heap/Heap.cpp (right): > > > > > > > > > https://codereview.chromium.org/850063002/diff/120001/Source/platform/heap/He... > > > Source/platform/heap/Heap.cpp:1266: return currentToken == (m_sweepStatus == > > 2); > > > On 2015/01/16 13:43:10, sof wrote: > > > > On 2015/01/16 13:40:01, haraken wrote: > > > > > > > > > > The token looks a bit tricky to me. Can we just define > > > BaseHeapPage::m_unswept > > > > > and set the flag to true in prepareForSweeping()? (We'll need to scan > the > > > list > > > > > of pages in prepareForSweeping(), but the overhead would be ignorable.) > > The > > > > > invariant is that BaseHeapPage::m_unswept is true if and only if the > > > > > BaseHeapPage is in a linked list of m_unsweptPages or > > m_unsweptLargeObjects. > > > > > > > > I elected not to scan all pages (previously swept and unswept) and update > > this > > > > field. That's why I did it this way. > > > > > > If you need to go with the flip bit, I guess you'll also need to re-flip the > > bit > > > in markUnmarkedObjectsDead(). (That said, it seems a bit complicated, so if > it > > > doesn't matter for performance, I'd prefer just scanning all pages in > > > prepareForSweep.) > > > > Don't think so, why would you have to flip it there? > > Probably I'm a bit confused. What's an invariant about the m_sweepStatus? I > wonder why we need to maintain 3 states (0 or 1 or 2). I guess what we need is a > way to know whether a given page is in a list of m_firstPage or > m_unsweptFirstPage. ^^^ And that's why I think you need to flip the bit in markUnmarkedObjectsDead(), which migrates pages from m_unsweptFirstPage to m_firstPage.
On 2015/01/16 14:30:45, haraken wrote: > On 2015/01/16 14:07:19, sof wrote: > > On 2015/01/16 13:54:55, haraken wrote: > > > > > > https://codereview.chromium.org/850063002/diff/120001/Source/platform/heap/He... > > > File Source/platform/heap/Heap.cpp (right): > > > > > > > > > https://codereview.chromium.org/850063002/diff/120001/Source/platform/heap/He... > > > Source/platform/heap/Heap.cpp:1266: return currentToken == (m_sweepStatus == > > 2); > > > On 2015/01/16 13:43:10, sof wrote: > > > > On 2015/01/16 13:40:01, haraken wrote: > > > > > > > > > > The token looks a bit tricky to me. Can we just define > > > BaseHeapPage::m_unswept > > > > > and set the flag to true in prepareForSweeping()? (We'll need to scan > the > > > list > > > > > of pages in prepareForSweeping(), but the overhead would be ignorable.) > > The > > > > > invariant is that BaseHeapPage::m_unswept is true if and only if the > > > > > BaseHeapPage is in a linked list of m_unsweptPages or > > m_unsweptLargeObjects. > > > > > > > > I elected not to scan all pages (previously swept and unswept) and update > > this > > > > field. That's why I did it this way. > > > > > > If you need to go with the flip bit, I guess you'll also need to re-flip the > > bit > > > in markUnmarkedObjectsDead(). (That said, it seems a bit complicated, so if > it > > > doesn't matter for performance, I'd prefer just scanning all pages in > > > prepareForSweep.) > > > > Don't think so, why would you have to flip it there? > > Probably I'm a bit confused. What's an invariant about the m_sweepStatus? I > wonder why we need to maintain 3 states (0 or 1 or 2). I guess what we need is a > way to know whether a given page is in a list of m_firstPage or > m_unsweptFirstPage. We alternate between two values across page sweeps, checking against the current value to determine if the lazy sweep has promoted a page to a swept page. Any (base) page that's freed (or been allocated) while a lazy sweep is ongoing, gets a sweep status of 0.
On 2015/01/16 14:34:21, sof wrote: > On 2015/01/16 14:30:45, haraken wrote: > > On 2015/01/16 14:07:19, sof wrote: > > > On 2015/01/16 13:54:55, haraken wrote: > > > > > > > > > > https://codereview.chromium.org/850063002/diff/120001/Source/platform/heap/He... > > > > File Source/platform/heap/Heap.cpp (right): > > > > > > > > > > > > > > https://codereview.chromium.org/850063002/diff/120001/Source/platform/heap/He... > > > > Source/platform/heap/Heap.cpp:1266: return currentToken == (m_sweepStatus > == > > > 2); > > > > On 2015/01/16 13:43:10, sof wrote: > > > > > On 2015/01/16 13:40:01, haraken wrote: > > > > > > > > > > > > The token looks a bit tricky to me. Can we just define > > > > BaseHeapPage::m_unswept > > > > > > and set the flag to true in prepareForSweeping()? (We'll need to scan > > the > > > > list > > > > > > of pages in prepareForSweeping(), but the overhead would be > ignorable.) > > > The > > > > > > invariant is that BaseHeapPage::m_unswept is true if and only if the > > > > > > BaseHeapPage is in a linked list of m_unsweptPages or > > > m_unsweptLargeObjects. > > > > > > > > > > I elected not to scan all pages (previously swept and unswept) and > update > > > this > > > > > field. That's why I did it this way. > > > > > > > > If you need to go with the flip bit, I guess you'll also need to re-flip > the > > > bit > > > > in markUnmarkedObjectsDead(). (That said, it seems a bit complicated, so > if > > it > > > > doesn't matter for performance, I'd prefer just scanning all pages in > > > > prepareForSweep.) > > > > > > Don't think so, why would you have to flip it there? > > > > Probably I'm a bit confused. What's an invariant about the m_sweepStatus? I > > wonder why we need to maintain 3 states (0 or 1 or 2). I guess what we need is > a > > way to know whether a given page is in a list of m_firstPage or > > m_unsweptFirstPage. > > We alternate between two values across page sweeps, checking against the current > value to determine if the lazy sweep has promoted a page to a swept page. > > Any (base) page that's freed (or been allocated) while a lazy sweep is ongoing, > gets a sweep status of 0. Then if we use an enum, is it something like NotSweepingTarget, SweepingTargetAndSwept, SweepingTargetAndNotSwept? (Actually I don't think my understanding is correct, because NotSweepingTarget and SweepingTargetAndSwept will be mixed after completeSweep().) I guess the current algorithm would work correctly but the invariant is a bit hard to verify.
On 2015/01/16 14:42:16, haraken wrote: > On 2015/01/16 14:34:21, sof wrote: > > On 2015/01/16 14:30:45, haraken wrote: > > > On 2015/01/16 14:07:19, sof wrote: > > > > On 2015/01/16 13:54:55, haraken wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/850063002/diff/120001/Source/platform/heap/He... > > > > > File Source/platform/heap/Heap.cpp (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/850063002/diff/120001/Source/platform/heap/He... > > > > > Source/platform/heap/Heap.cpp:1266: return currentToken == > (m_sweepStatus > > == > > > > 2); > > > > > On 2015/01/16 13:43:10, sof wrote: > > > > > > On 2015/01/16 13:40:01, haraken wrote: > > > > > > > > > > > > > > The token looks a bit tricky to me. Can we just define > > > > > BaseHeapPage::m_unswept > > > > > > > and set the flag to true in prepareForSweeping()? (We'll need to > scan > > > the > > > > > list > > > > > > > of pages in prepareForSweeping(), but the overhead would be > > ignorable.) > > > > The > > > > > > > invariant is that BaseHeapPage::m_unswept is true if and only if the > > > > > > > BaseHeapPage is in a linked list of m_unsweptPages or > > > > m_unsweptLargeObjects. > > > > > > > > > > > > I elected not to scan all pages (previously swept and unswept) and > > update > > > > this > > > > > > field. That's why I did it this way. > > > > > > > > > > If you need to go with the flip bit, I guess you'll also need to re-flip > > the > > > > bit > > > > > in markUnmarkedObjectsDead(). (That said, it seems a bit complicated, so > > if > > > it > > > > > doesn't matter for performance, I'd prefer just scanning all pages in > > > > > prepareForSweep.) > > > > > > > > Don't think so, why would you have to flip it there? > > > > > > Probably I'm a bit confused. What's an invariant about the m_sweepStatus? I > > > wonder why we need to maintain 3 states (0 or 1 or 2). I guess what we need > is > > a > > > way to know whether a given page is in a list of m_firstPage or > > > m_unsweptFirstPage. > > > > We alternate between two values across page sweeps, checking against the > current > > value to determine if the lazy sweep has promoted a page to a swept page. > > > > Any (base) page that's freed (or been allocated) while a lazy sweep is > ongoing, > > gets a sweep status of 0. > > Then if we use an enum, is it something like NotSweepingTarget, > SweepingTargetAndSwept, SweepingTargetAndNotSwept? (Actually I don't think my > understanding is correct, because NotSweepingTarget and SweepingTargetAndSwept > will be mixed after completeSweep().) > > I guess the current algorithm would work correctly but the invariant is a bit > hard to verify. As the value representing "has-been-swept" alternates, a fixed-value enum wouldn't quite fit, but those are the three states.
On 2015/01/16 14:46:53, sof wrote: > On 2015/01/16 14:42:16, haraken wrote: > > On 2015/01/16 14:34:21, sof wrote: > > > On 2015/01/16 14:30:45, haraken wrote: > > > > On 2015/01/16 14:07:19, sof wrote: > > > > > On 2015/01/16 13:54:55, haraken wrote: > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/850063002/diff/120001/Source/platform/heap/He... > > > > > > File Source/platform/heap/Heap.cpp (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/850063002/diff/120001/Source/platform/heap/He... > > > > > > Source/platform/heap/Heap.cpp:1266: return currentToken == > > (m_sweepStatus > > > == > > > > > 2); > > > > > > On 2015/01/16 13:43:10, sof wrote: > > > > > > > On 2015/01/16 13:40:01, haraken wrote: > > > > > > > > > > > > > > > > The token looks a bit tricky to me. Can we just define > > > > > > BaseHeapPage::m_unswept > > > > > > > > and set the flag to true in prepareForSweeping()? (We'll need to > > scan > > > > the > > > > > > list > > > > > > > > of pages in prepareForSweeping(), but the overhead would be > > > ignorable.) > > > > > The > > > > > > > > invariant is that BaseHeapPage::m_unswept is true if and only if > the > > > > > > > > BaseHeapPage is in a linked list of m_unsweptPages or > > > > > m_unsweptLargeObjects. > > > > > > > > > > > > > > I elected not to scan all pages (previously swept and unswept) and > > > update > > > > > this > > > > > > > field. That's why I did it this way. > > > > > > > > > > > > If you need to go with the flip bit, I guess you'll also need to > re-flip > > > the > > > > > bit > > > > > > in markUnmarkedObjectsDead(). (That said, it seems a bit complicated, > so > > > if > > > > it > > > > > > doesn't matter for performance, I'd prefer just scanning all pages in > > > > > > prepareForSweep.) > > > > > > > > > > Don't think so, why would you have to flip it there? > > > > > > > > Probably I'm a bit confused. What's an invariant about the m_sweepStatus? > I > > > > wonder why we need to maintain 3 states (0 or 1 or 2). I guess what we > need > > is > > > a > > > > way to know whether a given page is in a list of m_firstPage or > > > > m_unsweptFirstPage. > > > > > > We alternate between two values across page sweeps, checking against the > > current > > > value to determine if the lazy sweep has promoted a page to a swept page. > > > > > > Any (base) page that's freed (or been allocated) while a lazy sweep is > > ongoing, > > > gets a sweep status of 0. > > > > Then if we use an enum, is it something like NotSweepingTarget, > > SweepingTargetAndSwept, SweepingTargetAndNotSwept? (Actually I don't think my > > understanding is correct, because NotSweepingTarget and SweepingTargetAndSwept > > will be mixed after completeSweep().) > > > > I guess the current algorithm would work correctly but the invariant is a bit > > hard to verify. > > As the value representing "has-been-swept" alternates, a fixed-value enum > wouldn't quite fit, but those are the three states. It would perhaps be an improvement to rename hasBeenLazilySwept() to hasBeenSwept() ?
On 2015/01/16 14:50:58, sof wrote: > On 2015/01/16 14:46:53, sof wrote: > > On 2015/01/16 14:42:16, haraken wrote: > > > On 2015/01/16 14:34:21, sof wrote: > > > > On 2015/01/16 14:30:45, haraken wrote: > > > > > On 2015/01/16 14:07:19, sof wrote: > > > > > > On 2015/01/16 13:54:55, haraken wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/850063002/diff/120001/Source/platform/heap/He... > > > > > > > File Source/platform/heap/Heap.cpp (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/850063002/diff/120001/Source/platform/heap/He... > > > > > > > Source/platform/heap/Heap.cpp:1266: return currentToken == > > > (m_sweepStatus > > > > == > > > > > > 2); > > > > > > > On 2015/01/16 13:43:10, sof wrote: > > > > > > > > On 2015/01/16 13:40:01, haraken wrote: > > > > > > > > > > > > > > > > > > The token looks a bit tricky to me. Can we just define > > > > > > > BaseHeapPage::m_unswept > > > > > > > > > and set the flag to true in prepareForSweeping()? (We'll need to > > > scan > > > > > the > > > > > > > list > > > > > > > > > of pages in prepareForSweeping(), but the overhead would be > > > > ignorable.) > > > > > > The > > > > > > > > > invariant is that BaseHeapPage::m_unswept is true if and only if > > the > > > > > > > > > BaseHeapPage is in a linked list of m_unsweptPages or > > > > > > m_unsweptLargeObjects. > > > > > > > > > > > > > > > > I elected not to scan all pages (previously swept and unswept) and > > > > update > > > > > > this > > > > > > > > field. That's why I did it this way. > > > > > > > > > > > > > > If you need to go with the flip bit, I guess you'll also need to > > re-flip > > > > the > > > > > > bit > > > > > > > in markUnmarkedObjectsDead(). (That said, it seems a bit > complicated, > > so > > > > if > > > > > it > > > > > > > doesn't matter for performance, I'd prefer just scanning all pages > in > > > > > > > prepareForSweep.) > > > > > > > > > > > > Don't think so, why would you have to flip it there? > > > > > > > > > > Probably I'm a bit confused. What's an invariant about the > m_sweepStatus? > > I > > > > > wonder why we need to maintain 3 states (0 or 1 or 2). I guess what we > > need > > > is > > > > a > > > > > way to know whether a given page is in a list of m_firstPage or > > > > > m_unsweptFirstPage. > > > > > > > > We alternate between two values across page sweeps, checking against the > > > current > > > > value to determine if the lazy sweep has promoted a page to a swept page. > > > > > > > > Any (base) page that's freed (or been allocated) while a lazy sweep is > > > ongoing, > > > > gets a sweep status of 0. > > > > > > Then if we use an enum, is it something like NotSweepingTarget, > > > SweepingTargetAndSwept, SweepingTargetAndNotSwept? (Actually I don't think > my > > > understanding is correct, because NotSweepingTarget and > SweepingTargetAndSwept > > > will be mixed after completeSweep().) > > > > > > I guess the current algorithm would work correctly but the invariant is a > bit > > > hard to verify. > > > > As the value representing "has-been-swept" alternates, a fixed-value enum > > wouldn't quite fit, but those are the three states. Ah, I now understand we need three states... (a) Pages that have been swept in the latest lazy sweeping. (b) Pages that are going to be swept in the lazy sweeping. (c) Other pages. m_firstPage contains (a) or (c). m_firstUnsweptPage contains (b). Then can we manage the states more straightforwardly? I mean: - Introduce an enum: HasBeenSwept, WillBeSwept, NotSweepingTarget. - In prepareForSweeping, scan all pages and set WillBeSwept on them. - When migrating a page from m_firstUnsweptPage to m_firstPage, change WillBeSwept to HasBeenSwept. - When allocating a new page, set NotSweepingTarget. - Add an ASSERT somewhere to verify that m_firstPage contains only HasBeenSwept or NotSweepingTarget and that m_firstUnsweptPage contains only WillBeSwept.
On 2015/01/16 15:12:27, haraken wrote: > On 2015/01/16 14:50:58, sof wrote: > > On 2015/01/16 14:46:53, sof wrote: > > > On 2015/01/16 14:42:16, haraken wrote: > > > > On 2015/01/16 14:34:21, sof wrote: > > > > > On 2015/01/16 14:30:45, haraken wrote: > > > > > > On 2015/01/16 14:07:19, sof wrote: > > > > > > > On 2015/01/16 13:54:55, haraken wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/850063002/diff/120001/Source/platform/heap/He... > > > > > > > > File Source/platform/heap/Heap.cpp (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/850063002/diff/120001/Source/platform/heap/He... > > > > > > > > Source/platform/heap/Heap.cpp:1266: return currentToken == > > > > (m_sweepStatus > > > > > == > > > > > > > 2); > > > > > > > > On 2015/01/16 13:43:10, sof wrote: > > > > > > > > > On 2015/01/16 13:40:01, haraken wrote: > > > > > > > > > > > > > > > > > > > > The token looks a bit tricky to me. Can we just define > > > > > > > > BaseHeapPage::m_unswept > > > > > > > > > > and set the flag to true in prepareForSweeping()? (We'll need > to > > > > scan > > > > > > the > > > > > > > > list > > > > > > > > > > of pages in prepareForSweeping(), but the overhead would be > > > > > ignorable.) > > > > > > > The > > > > > > > > > > invariant is that BaseHeapPage::m_unswept is true if and only > if > > > the > > > > > > > > > > BaseHeapPage is in a linked list of m_unsweptPages or > > > > > > > m_unsweptLargeObjects. > > > > > > > > > > > > > > > > > > I elected not to scan all pages (previously swept and unswept) > and > > > > > update > > > > > > > this > > > > > > > > > field. That's why I did it this way. > > > > > > > > > > > > > > > > If you need to go with the flip bit, I guess you'll also need to > > > re-flip > > > > > the > > > > > > > bit > > > > > > > > in markUnmarkedObjectsDead(). (That said, it seems a bit > > complicated, > > > so > > > > > if > > > > > > it > > > > > > > > doesn't matter for performance, I'd prefer just scanning all pages > > in > > > > > > > > prepareForSweep.) > > > > > > > > > > > > > > Don't think so, why would you have to flip it there? > > > > > > > > > > > > Probably I'm a bit confused. What's an invariant about the > > m_sweepStatus? > > > I > > > > > > wonder why we need to maintain 3 states (0 or 1 or 2). I guess what we > > > need > > > > is > > > > > a > > > > > > way to know whether a given page is in a list of m_firstPage or > > > > > > m_unsweptFirstPage. > > > > > > > > > > We alternate between two values across page sweeps, checking against the > > > > current > > > > > value to determine if the lazy sweep has promoted a page to a swept > page. > > > > > > > > > > Any (base) page that's freed (or been allocated) while a lazy sweep is > > > > ongoing, > > > > > gets a sweep status of 0. > > > > > > > > Then if we use an enum, is it something like NotSweepingTarget, > > > > SweepingTargetAndSwept, SweepingTargetAndNotSwept? (Actually I don't think > > my > > > > understanding is correct, because NotSweepingTarget and > > SweepingTargetAndSwept > > > > will be mixed after completeSweep().) > > > > > > > > I guess the current algorithm would work correctly but the invariant is a > > bit > > > > hard to verify. > > > > > > As the value representing "has-been-swept" alternates, a fixed-value enum > > > wouldn't quite fit, but those are the three states. > > Ah, I now understand we need three states... > > (a) Pages that have been swept in the latest lazy sweeping. > (b) Pages that are going to be swept in the lazy sweeping. > (c) Other pages. > > m_firstPage contains (a) or (c). m_firstUnsweptPage contains (b). > > Then can we manage the states more straightforwardly? I mean: > > - Introduce an enum: HasBeenSwept, WillBeSwept, NotSweepingTarget. > - In prepareForSweeping, scan all pages and set WillBeSwept on them. > - When migrating a page from m_firstUnsweptPage to m_firstPage, change > WillBeSwept to HasBeenSwept. > - When allocating a new page, set NotSweepingTarget. > - Add an ASSERT somewhere to verify that m_firstPage contains only HasBeenSwept > or NotSweepingTarget and that m_firstUnsweptPage contains only WillBeSwept. You're not going to let this pass it seems, but will insist on all pages being marked/cleared on initiation of a sweep. ok, then. You can make do without an enum.
On 2015/01/16 15:17:40, sof wrote: > On 2015/01/16 15:12:27, haraken wrote: > > On 2015/01/16 14:50:58, sof wrote: > > > On 2015/01/16 14:46:53, sof wrote: > > > > On 2015/01/16 14:42:16, haraken wrote: > > > > > On 2015/01/16 14:34:21, sof wrote: > > > > > > On 2015/01/16 14:30:45, haraken wrote: > > > > > > > On 2015/01/16 14:07:19, sof wrote: > > > > > > > > On 2015/01/16 13:54:55, haraken wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/850063002/diff/120001/Source/platform/heap/He... > > > > > > > > > File Source/platform/heap/Heap.cpp (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/850063002/diff/120001/Source/platform/heap/He... > > > > > > > > > Source/platform/heap/Heap.cpp:1266: return currentToken == > > > > > (m_sweepStatus > > > > > > == > > > > > > > > 2); > > > > > > > > > On 2015/01/16 13:43:10, sof wrote: > > > > > > > > > > On 2015/01/16 13:40:01, haraken wrote: > > > > > > > > > > > > > > > > > > > > > > The token looks a bit tricky to me. Can we just define > > > > > > > > > BaseHeapPage::m_unswept > > > > > > > > > > > and set the flag to true in prepareForSweeping()? (We'll > need > > to > > > > > scan > > > > > > > the > > > > > > > > > list > > > > > > > > > > > of pages in prepareForSweeping(), but the overhead would be > > > > > > ignorable.) > > > > > > > > The > > > > > > > > > > > invariant is that BaseHeapPage::m_unswept is true if and > only > > if > > > > the > > > > > > > > > > > BaseHeapPage is in a linked list of m_unsweptPages or > > > > > > > > m_unsweptLargeObjects. > > > > > > > > > > > > > > > > > > > > I elected not to scan all pages (previously swept and unswept) > > and > > > > > > update > > > > > > > > this > > > > > > > > > > field. That's why I did it this way. > > > > > > > > > > > > > > > > > > If you need to go with the flip bit, I guess you'll also need to > > > > re-flip > > > > > > the > > > > > > > > bit > > > > > > > > > in markUnmarkedObjectsDead(). (That said, it seems a bit > > > complicated, > > > > so > > > > > > if > > > > > > > it > > > > > > > > > doesn't matter for performance, I'd prefer just scanning all > pages > > > in > > > > > > > > > prepareForSweep.) > > > > > > > > > > > > > > > > Don't think so, why would you have to flip it there? > > > > > > > > > > > > > > Probably I'm a bit confused. What's an invariant about the > > > m_sweepStatus? > > > > I > > > > > > > wonder why we need to maintain 3 states (0 or 1 or 2). I guess what > we > > > > need > > > > > is > > > > > > a > > > > > > > way to know whether a given page is in a list of m_firstPage or > > > > > > > m_unsweptFirstPage. > > > > > > > > > > > > We alternate between two values across page sweeps, checking against > the > > > > > current > > > > > > value to determine if the lazy sweep has promoted a page to a swept > > page. > > > > > > > > > > > > Any (base) page that's freed (or been allocated) while a lazy sweep is > > > > > ongoing, > > > > > > gets a sweep status of 0. > > > > > > > > > > Then if we use an enum, is it something like NotSweepingTarget, > > > > > SweepingTargetAndSwept, SweepingTargetAndNotSwept? (Actually I don't > think > > > my > > > > > understanding is correct, because NotSweepingTarget and > > > SweepingTargetAndSwept > > > > > will be mixed after completeSweep().) > > > > > > > > > > I guess the current algorithm would work correctly but the invariant is > a > > > bit > > > > > hard to verify. > > > > > > > > As the value representing "has-been-swept" alternates, a fixed-value enum > > > > wouldn't quite fit, but those are the three states. > > > > Ah, I now understand we need three states... > > > > (a) Pages that have been swept in the latest lazy sweeping. > > (b) Pages that are going to be swept in the lazy sweeping. > > (c) Other pages. > > > > m_firstPage contains (a) or (c). m_firstUnsweptPage contains (b). > > > > Then can we manage the states more straightforwardly? I mean: > > > > - Introduce an enum: HasBeenSwept, WillBeSwept, NotSweepingTarget. > > - In prepareForSweeping, scan all pages and set WillBeSwept on them. > > - When migrating a page from m_firstUnsweptPage to m_firstPage, change > > WillBeSwept to HasBeenSwept. > > - When allocating a new page, set NotSweepingTarget. > > - Add an ASSERT somewhere to verify that m_firstPage contains only > HasBeenSwept > > or NotSweepingTarget and that m_firstUnsweptPage contains only WillBeSwept. > > You're not going to let this pass it seems, but will insist on all pages being > marked/cleared on initiation of a sweep. > > ok, then. You can make do without an enum. Done.
Mostly looks good to me. https://codereview.chromium.org/850063002/diff/160001/Source/platform/heap/He... File Source/platform/heap/Heap.cpp (right): https://codereview.chromium.org/850063002/diff/160001/Source/platform/heap/He... Source/platform/heap/Heap.cpp:546: markAsUnswept(); Isn't it guaranteed that the page is already marked as unswept at this point? (because removeFromHeap is called when the page is swept.) We can probably add ASSERT(!hasBeenSwept()) instead? https://codereview.chromium.org/850063002/diff/160001/Source/platform/heap/He... Source/platform/heap/Heap.cpp:1154: largeObject->markAsSwept(); It's a bit inconsistent that a page allocated during sweeping is marked as swept but a page allocated not during sweeping is marked as unswept. You might want to set m_swept(true) in BaseHeapPage's constructor. Then you can remove these two lines, I think. https://codereview.chromium.org/850063002/diff/160001/Source/platform/heap/He... Source/platform/heap/Heap.cpp:1243: , m_swept(false) As commented above, m_swept(true) will make the code a bit tidier. https://codereview.chromium.org/850063002/diff/160001/Source/platform/heap/He... Source/platform/heap/Heap.cpp:1403: page->markAsSwept(); Ditto. I think we can remove this code. https://codereview.chromium.org/850063002/diff/160001/Source/platform/heap/He... Source/platform/heap/Heap.cpp:1463: for (HeapPage* page = m_firstPage; page; page = page->next()) You can move this loop to after line 1484. https://codereview.chromium.org/850063002/diff/160001/Source/platform/heap/He... Source/platform/heap/Heap.cpp:1476: page->markAsUnswept(); Then you won't need this. https://codereview.chromium.org/850063002/diff/160001/Source/platform/heap/He... Source/platform/heap/Heap.cpp:1486: for (LargeObject* largeObject = m_firstLargeObject; largeObject; largeObject = largeObject->next()) Ditto. https://codereview.chromium.org/850063002/diff/160001/Source/platform/heap/He... Source/platform/heap/Heap.cpp:1638: markAsUnswept(); I think the page is already marked as unswept. ASSERT(!hasBeenSwept()) instead? https://codereview.chromium.org/850063002/diff/160001/Source/platform/heap/He... File Source/platform/heap/Heap.h (right): https://codereview.chromium.org/850063002/diff/160001/Source/platform/heap/He... Source/platform/heap/Heap.h:404: // Returns true if this page has been swept by the ongoing lazy/incremental sweep. Nit: lazy/incremental => lazy https://codereview.chromium.org/850063002/diff/160001/Source/platform/heap/He... Source/platform/heap/Heap.h:407: void markAsSwept() { m_swept = true; } Can we add ASSERT(!m_swept)? https://codereview.chromium.org/850063002/diff/160001/Source/platform/heap/He... Source/platform/heap/Heap.h:408: void markAsUnswept() { m_swept = false; } Can we add ASSERT(m_swept)? https://codereview.chromium.org/850063002/diff/160001/Source/platform/heap/He... Source/platform/heap/Heap.h:416: protected: Need to be protected? https://codereview.chromium.org/850063002/diff/160001/Source/platform/heap/He... Source/platform/heap/Heap.h:420: // Cleared at the start of a sweep. Set to false at the start of a sweep and set to true when the lazy sweeping is complete. https://codereview.chromium.org/850063002/diff/160001/Source/platform/heap/He... Source/platform/heap/Heap.h:860: if (!page->heap()->threadState()->isSweepingInProgress()) Do we need this check? I guess that the following page->hasBeenSwept() would be enough. We can probably add ASSERT(page->heap()->threadState()->isSweepingInProgress()) to line 865. https://codereview.chromium.org/850063002/diff/160001/Source/platform/heap/He... Source/platform/heap/Heap.h:868: // FIXME: remove when incremental sweeping is always on Nit: incremental => lazy
https://codereview.chromium.org/850063002/diff/160001/Source/platform/heap/He... File Source/platform/heap/Heap.cpp (right): https://codereview.chromium.org/850063002/diff/160001/Source/platform/heap/He... Source/platform/heap/Heap.cpp:546: markAsUnswept(); On 2015/01/16 17:29:49, haraken wrote: > > Isn't it guaranteed that the page is already marked as unswept at this point? > (because removeFromHeap is called when the page is swept.) > > We can probably add ASSERT(!hasBeenSwept()) instead? Removed. https://codereview.chromium.org/850063002/diff/160001/Source/platform/heap/He... Source/platform/heap/Heap.cpp:1154: largeObject->markAsSwept(); On 2015/01/16 17:29:49, haraken wrote: > > It's a bit inconsistent that a page allocated during sweeping is marked as swept > but a page allocated not during sweeping is marked as unswept. > > You might want to set m_swept(true) in BaseHeapPage's constructor. Then you can > remove these two lines, I think. We can flip defaults that way. https://codereview.chromium.org/850063002/diff/160001/Source/platform/heap/He... Source/platform/heap/Heap.cpp:1243: , m_swept(false) On 2015/01/16 17:29:49, haraken wrote: > > As commented above, m_swept(true) will make the code a bit tidier. Done. https://codereview.chromium.org/850063002/diff/160001/Source/platform/heap/He... Source/platform/heap/Heap.cpp:1403: page->markAsSwept(); On 2015/01/16 17:29:49, haraken wrote: > > Ditto. I think we can remove this code. Done. https://codereview.chromium.org/850063002/diff/160001/Source/platform/heap/He... Source/platform/heap/Heap.cpp:1463: for (HeapPage* page = m_firstPage; page; page = page->next()) On 2015/01/16 17:29:49, haraken wrote: > > You can move this loop to after line 1484. It's preferable to have this loop here & over these pages only; there's actually no need to clear the sweep flag on the as-yet-unswept heap pages. By construction these will be cleared/false already. https://codereview.chromium.org/850063002/diff/160001/Source/platform/heap/He... Source/platform/heap/Heap.cpp:1486: for (LargeObject* largeObject = m_firstLargeObject; largeObject; largeObject = largeObject->next()) On 2015/01/16 17:29:49, haraken wrote: > > Ditto. See above. https://codereview.chromium.org/850063002/diff/160001/Source/platform/heap/He... Source/platform/heap/Heap.cpp:1638: markAsUnswept(); On 2015/01/16 17:29:49, haraken wrote: > > I think the page is already marked as unswept. ASSERT(!hasBeenSwept()) instead? Removed. https://codereview.chromium.org/850063002/diff/160001/Source/platform/heap/He... File Source/platform/heap/Heap.h (right): https://codereview.chromium.org/850063002/diff/160001/Source/platform/heap/He... Source/platform/heap/Heap.h:404: // Returns true if this page has been swept by the ongoing lazy/incremental sweep. On 2015/01/16 17:29:49, haraken wrote: > > Nit: lazy/incremental => lazy done. https://codereview.chromium.org/850063002/diff/160001/Source/platform/heap/He... Source/platform/heap/Heap.h:407: void markAsSwept() { m_swept = true; } On 2015/01/16 17:29:49, haraken wrote: > > Can we add ASSERT(!m_swept)? Done. https://codereview.chromium.org/850063002/diff/160001/Source/platform/heap/He... Source/platform/heap/Heap.h:408: void markAsUnswept() { m_swept = false; } On 2015/01/16 17:29:49, haraken wrote: > > Can we add ASSERT(m_swept)? Done. https://codereview.chromium.org/850063002/diff/160001/Source/platform/heap/He... Source/platform/heap/Heap.h:416: protected: On 2015/01/16 17:29:49, haraken wrote: > > Need to be protected? Done. https://codereview.chromium.org/850063002/diff/160001/Source/platform/heap/He... Source/platform/heap/Heap.h:420: // Cleared at the start of a sweep. On 2015/01/16 17:29:49, haraken wrote: > > Set to false at the start of a sweep and set to true when the lazy sweeping is > complete. Done. https://codereview.chromium.org/850063002/diff/160001/Source/platform/heap/He... Source/platform/heap/Heap.h:860: if (!page->heap()->threadState()->isSweepingInProgress()) On 2015/01/16 17:29:49, haraken wrote: > > Do we need this check? I guess that the following page->hasBeenSwept() would be > enough. We can probably add > ASSERT(page->heap()->threadState()->isSweepingInProgress()) to line 865. It depends on having all the page mark bits set up correctly at all times, which they now are, so this is a vestige of the earlier scheme. Adjusted. https://codereview.chromium.org/850063002/diff/160001/Source/platform/heap/He... Source/platform/heap/Heap.h:868: // FIXME: remove when incremental sweeping is always on On 2015/01/16 17:29:49, haraken wrote: > > Nit: incremental => lazy Done.
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/850063002/210004
Message was sent while issue was closed.
Committed patchset #12 (id:210004) as https://src.chromium.org/viewvc/blink?view=rev&revision=188577
Message was sent while issue was closed.
https://codereview.chromium.org/850063002/diff/210004/Source/platform/heap/He... File Source/platform/heap/Heap.h (right): https://codereview.chromium.org/850063002/diff/210004/Source/platform/heap/He... Source/platform/heap/Heap.h:862: // A couple of suggestions to clean up the method and the comment: - Rename isFinalizedObjectAlive to willBeSweptInLazySweeping. Flip true/false of the return value. ("isFinalizedObjectAlive" is confusing because we're not allowed to pass an already-finalized object.) - Update the comment to: // Returns true if the |objectPointer| is going to be swept in the on-going // lazy sweeping. If no lazy sweeping is in progress, it returns true. // If lazy sweeping is in progress, it returns true iff the |objectPointer| // hasn't yet been swept and it's unmarked. You should not pass an already-finalized // object to |objectPointer|. - Add an ASSERT to verify that |objectPointer| is not an already-finalized object: ASSERT(!objectPointer->isFree()); objectPointer->checkHeader(); |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
