Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
7031005:
Extend Handle API with MarkIndependent. (Closed)
|
Created:
9 years, 7 months ago by Vyacheslav Egorov (Chromium) Modified:
9 years, 7 months ago CC:
v8-dev Visibility:
Public. |
DescriptionExtend Handle API with MarkIndependent.
Garbage collector is free to ignore object groups for independent handles and can collect then in minor collections.
Committed: http://code.google.com/p/v8/source/detail?r=7915
Patch Set 1 #
Total comments: 30
Patch Set 2 : partially fixed Vitaly's and Anton's comments #Patch Set 3 : remove default clause #
Total comments: 3
Patch Set 4 : do not process PENDING dependent handles after SCAVENGE #
Total comments: 7
Messages
Total messages: 14 (0 generated)
fyi: +iposva, +vitalyr
Almost LG, but we should be more careful with nasty callbacks that can trigger GC. http://codereview.chromium.org/7031005/diff/1/include/v8.h File include/v8.h (right): http://codereview.chromium.org/7031005/diff/1/include/v8.h#newcode392 include/v8.h:392: * is free to ignore any object groups containing this object. I think we should also mention that the gc prologue/epilogue callbacks are not guaranteed to be called here. More important issue is whether it's safe to enter V8 in the associated weak callback. I don't think we can survive nested scavenges. http://codereview.chromium.org/7031005/diff/1/src/global-handles.h File src/global-handles.h (right): http://codereview.chromium.org/7031005/diff/1/src/global-handles.h#newcode184 src/global-handles.h:184: // Iterates over weak independent roots that are bound to a given callback. The comment is wrong, but we should most likely refactor this function. http://codereview.chromium.org/7031005/diff/1/src/heap.cc File src/heap.cc (right): http://codereview.chromium.org/7031005/diff/1/src/heap.cc#newcode777 src/heap.cc:777: isolate_->global_handles()->PostGarbageCollectionProcessing(); This can invoke callbacks for pending dependent handles left over from previous major GCs. (Because some of them can trigger GCs.) Given that the prologue/epilogue callbacks won't be called it might be a problem. http://codereview.chromium.org/7031005/diff/1/src/heap.cc#newcode1037 src/heap.cc:1037: isolate_->global_handles()->IdentifyWeakIndependentHandles( Scanning the linked list of handles was really slow in some benchmarks. We should merge these two passes because the scavenges should be as fast as possible. http://codereview.chromium.org/7031005/diff/1/src/heap.cc#newcode4560 src/heap.cc:4560: default: Gcc should be able to statically catch an unhanded case error if default is omitted.
DBC Overall, independent doesn't look like a very good name to me. Something like early collectable feels somewhat better, but YMMV. http://codereview.chromium.org/7031005/diff/1/include/v8.h File include/v8.h (right): http://codereview.chromium.org/7031005/diff/1/include/v8.h#newcode391 include/v8.h:391: * Mark the reference to this object independent. Garbage collector nit: Mark[s] http://codereview.chromium.org/7031005/diff/1/src/global-handles.cc File src/global-handles.cc (right): http://codereview.chromium.org/7031005/diff/1/src/global-handles.cc#newcode330 src/global-handles.cc:330: Handle<Object> GlobalHandles::Create(Object* value) { I think you should reset independent flag here. http://codereview.chromium.org/7031005/diff/1/src/global-handles.cc#newcode412 src/global-handles.cc:412: // WEAK or PENDING. comment is wrong. http://codereview.chromium.org/7031005/diff/1/test/cctest/test-api.cc File test/cctest/test-api.cc (right): http://codereview.chromium.org/7031005/diff/1/test/cctest/test-api.cc#newcode... test/cctest/test-api.cc:4455: object_a.MarkIndependent(); please, add tests cases for weak callbacks forcing various GCs (both scavenge and major GCs)
Thanks for looking at this Vitaly. My replies inline. http://codereview.chromium.org/7031005/diff/1/include/v8.h File include/v8.h (right): http://codereview.chromium.org/7031005/diff/1/include/v8.h#newcode392 include/v8.h:392: * is free to ignore any object groups containing this object. On 2011/05/16 13:42:11, Vitaly Repeshko wrote: > I think we should also mention that the gc prologue/epilogue callbacks are not > guaranteed to be called here. > > More important issue is whether it's safe to enter V8 in the associated weak > callback. I don't think we can survive nested scavenges. Prologue/epilogue will be called with kGCTypeScavenge. See Heap::PerformGarbageCollection. There is also nothing nested about the GC that will happen in callback (no difference between MS and Scavenge in this respect). Callbacks are called when GC is complete and the state of heap is "operational". http://codereview.chromium.org/7031005/diff/1/src/global-handles.h File src/global-handles.h (right): http://codereview.chromium.org/7031005/diff/1/src/global-handles.h#newcode184 src/global-handles.h:184: // Iterates over weak independent roots that are bound to a given callback. On 2011/05/16 13:42:11, Vitaly Repeshko wrote: > The comment is wrong, but we should most likely refactor this function. Yep. Thanks for catching it. Copy/paste with wrong direction vector (should have c/p from below, but did c&p from above). http://codereview.chromium.org/7031005/diff/1/src/heap.cc File src/heap.cc (right): http://codereview.chromium.org/7031005/diff/1/src/heap.cc#newcode777 src/heap.cc:777: isolate_->global_handles()->PostGarbageCollectionProcessing(); On 2011/05/16 13:42:11, Vitaly Repeshko wrote: > This can invoke callbacks for pending dependent handles left over from previous > major GCs. (Because some of them can trigger GCs.) Given that the > prologue/epilogue callbacks won't be called it might be a problem. Yes it can and I am failing to see how it is a problem. Once the handle enters a PENDING state his callback _will_ be called no matter what happens to object in subsequent collections. The object e.g. can become strongly reachable, but this would not affect anything. If you know places in bindings where this can be a problem --- we should fix them. http://codereview.chromium.org/7031005/diff/1/src/heap.cc#newcode1037 src/heap.cc:1037: isolate_->global_handles()->IdentifyWeakIndependentHandles( On 2011/05/16 13:42:11, Vitaly Repeshko wrote: > Scanning the linked list of handles was really slow in some benchmarks. We > should merge these two passes because the scavenges should be as fast as > possible. If there are two weak handles that point to the same object them both should have their callbacks called. Thus merging these two passes together would require additional mark somewhere on the object to distinguish objects evacuated by normal scavenging and objects evacuated by scavenging of weak independent handles. We probably can use the second bit from map word but this would complicate things. http://codereview.chromium.org/7031005/diff/1/src/heap.cc#newcode4560 src/heap.cc:4560: default: On 2011/05/16 13:42:11, Vitaly Repeshko wrote: > Gcc should be able to statically catch an unhanded case error if default is > omitted. Ok.
http://codereview.chromium.org/7031005/diff/1/include/v8.h File include/v8.h (right): http://codereview.chromium.org/7031005/diff/1/include/v8.h#newcode392 include/v8.h:392: * is free to ignore any object groups containing this object. On 2011/05/16 14:15:46, Vyacheslav Egorov wrote: > On 2011/05/16 13:42:11, Vitaly Repeshko wrote: > > I think we should also mention that the gc prologue/epilogue callbacks are not > > guaranteed to be called here. > > > > More important issue is whether it's safe to enter V8 in the associated weak > > callback. I don't think we can survive nested scavenges. > > Prologue/epilogue will be called with kGCTypeScavenge. See > Heap::PerformGarbageCollection. The ones taking the type argument are only used for stats, the "real" callbacks are named global_gc_{prologue,epilogue}_callback_ and I think are only called during major GCs. > There is also nothing nested about the GC that will happen in callback (no > difference between MS and Scavenge in this respect). Callbacks are called when > GC is complete and the state of heap is "operational". Right, it's most likely not a problem. I assumed the callbacks are being called early. Still I agree with Anton that more testing of this kind of corner cases would be nice. http://codereview.chromium.org/7031005/diff/1/src/heap.cc File src/heap.cc (right): http://codereview.chromium.org/7031005/diff/1/src/heap.cc#newcode777 src/heap.cc:777: isolate_->global_handles()->PostGarbageCollectionProcessing(); On 2011/05/16 13:42:11, Vitaly Repeshko wrote: > This can invoke callbacks for pending dependent handles left over from previous > major GCs. (Because some of them can trigger GCs.) Given that the > prologue/epilogue callbacks won't be called it might be a problem. If I'm right about the prologue/epilogue callbacks not being called, it could be a problem because weak callbacks used to be called between the prologue/epilogue pair. http://codereview.chromium.org/7031005/diff/1/src/heap.cc#newcode1037 src/heap.cc:1037: isolate_->global_handles()->IdentifyWeakIndependentHandles( On 2011/05/16 14:15:46, Vyacheslav Egorov wrote: > On 2011/05/16 13:42:11, Vitaly Repeshko wrote: > > Scanning the linked list of handles was really slow in some benchmarks. We > > should merge these two passes because the scavenges should be as fast as > > possible. > > If there are two weak handles that point to the same object them both should > have their callbacks called. > > Thus merging these two passes together would require additional mark somewhere > on the object to distinguish objects evacuated by normal scavenging and objects > evacuated by scavenging of weak independent handles. We probably can use the > second bit from map word but this would complicate things. Ahh, this is annoying. Can we at least inline the "identify" callback?
Anton thanks for driving by. I have addressed some of your and Vitaly's comments. http://codereview.chromium.org/7031005/diff/1/include/v8.h File include/v8.h (right): http://codereview.chromium.org/7031005/diff/1/include/v8.h#newcode391 include/v8.h:391: * Mark the reference to this object independent. Garbage collector On 2011/05/16 14:13:19, antonm wrote: > nit: Mark[s] I will fix it, but it seems that we have two style of comments: one talks about the function (the function marks) and the other talks about the action (mark). See for example MakeWeak above. http://codereview.chromium.org/7031005/diff/1/include/v8.h#newcode392 include/v8.h:392: * is free to ignore any object groups containing this object. On 2011/05/16 14:31:22, Vitaly Repeshko wrote: > On 2011/05/16 14:15:46, Vyacheslav Egorov wrote: > > On 2011/05/16 13:42:11, Vitaly Repeshko wrote: > > > I think we should also mention that the gc prologue/epilogue callbacks are > not > > > guaranteed to be called here. > > > > > > More important issue is whether it's safe to enter V8 in the associated weak > > > callback. I don't think we can survive nested scavenges. > > > > Prologue/epilogue will be called with kGCTypeScavenge. See > > Heap::PerformGarbageCollection. > > The ones taking the type argument are only used for stats, the "real" callbacks > are named global_gc_{prologue,epilogue}_callback_ and I think are only called > during major GCs. > > > There is also nothing nested about the GC that will happen in callback (no > > difference between MS and Scavenge in this respect). Callbacks are called when > > GC is complete and the state of heap is "operational". > > Right, it's most likely not a problem. I assumed the callbacks are being called > early. Still I agree with Anton that more testing of this kind of corner cases > would be nice. Yep you are right about callback. Global one will only be invoked for full collection. Test added. But frankly speaking I do not think that V8 has to guarantee any ordering between prologue/epilogue callbacks and weak handle callbacks. http://codereview.chromium.org/7031005/diff/1/src/global-handles.cc File src/global-handles.cc (right): http://codereview.chromium.org/7031005/diff/1/src/global-handles.cc#newcode330 src/global-handles.cc:330: Handle<Object> GlobalHandles::Create(Object* value) { On 2011/05/16 14:13:19, antonm wrote: > I think you should reset independent flag here. It's done by result->Initialize(value) below. Am I missing some code path? http://codereview.chromium.org/7031005/diff/1/src/global-handles.cc#newcode412 src/global-handles.cc:412: // WEAK or PENDING. On 2011/05/16 14:13:19, antonm wrote: > comment is wrong. Thanks! Will fix. http://codereview.chromium.org/7031005/diff/1/src/heap.cc File src/heap.cc (right): http://codereview.chromium.org/7031005/diff/1/src/heap.cc#newcode777 src/heap.cc:777: isolate_->global_handles()->PostGarbageCollectionProcessing(); On 2011/05/16 14:31:22, Vitaly Repeshko wrote: > On 2011/05/16 13:42:11, Vitaly Repeshko wrote: > > This can invoke callbacks for pending dependent handles left over from > previous > > major GCs. (Because some of them can trigger GCs.) Given that the > > prologue/epilogue callbacks won't be called it might be a problem. > > If I'm right about the prologue/epilogue callbacks not being called, it could be > a problem because weak callbacks used to be called between the prologue/epilogue > pair. We should not mark those handles that depend on prologue/epilogue callbacks as independent. The whole idea of independent handle is that it, well, does not depend on anything. http://codereview.chromium.org/7031005/diff/1/src/heap.cc#newcode1037 src/heap.cc:1037: isolate_->global_handles()->IdentifyWeakIndependentHandles( On 2011/05/16 14:31:22, Vitaly Repeshko wrote: > On 2011/05/16 14:15:46, Vyacheslav Egorov wrote: > > On 2011/05/16 13:42:11, Vitaly Repeshko wrote: > > > Scanning the linked list of handles was really slow in some benchmarks. We > > > should merge these two passes because the scavenges should be as fast as > > > possible. > > > > If there are two weak handles that point to the same object them both should > > have their callbacks called. > > > > Thus merging these two passes together would require additional mark somewhere > > on the object to distinguish objects evacuated by normal scavenging and > objects > > evacuated by scavenging of weak independent handles. We probably can use the > > second bit from map word but this would complicate things. > > Ahh, this is annoying. Can we at least inline the "identify" callback? To inline identify callback we need to move IdentifyWeakIndependentHandles into global-handles-inl.h. I can do that but I really think it's premature optimization. I have not seen any significant degradation on a test that had dozens of thousands of independent handles created and destroyed every second. Alternatively we can have separate lists for independent and dependent handles so we will not pay anything for dependent handles when we don't need to iterate over them. http://codereview.chromium.org/7031005/diff/1/test/cctest/test-api.cc File test/cctest/test-api.cc (right): http://codereview.chromium.org/7031005/diff/1/test/cctest/test-api.cc#newcode... test/cctest/test-api.cc:4455: object_a.MarkIndependent(); On 2011/05/16 14:13:19, antonm wrote: > please, add tests cases for weak callbacks forcing various GCs (both scavenge > and major GCs) Done.
http://codereview.chromium.org/7031005/diff/1/include/v8.h File include/v8.h (right): http://codereview.chromium.org/7031005/diff/1/include/v8.h#newcode392 include/v8.h:392: * is free to ignore any object groups containing this object. On 2011/05/16 14:55:34, Vyacheslav Egorov wrote: > On 2011/05/16 14:31:22, Vitaly Repeshko wrote: > > On 2011/05/16 14:15:46, Vyacheslav Egorov wrote: > > > On 2011/05/16 13:42:11, Vitaly Repeshko wrote: > > > > I think we should also mention that the gc prologue/epilogue callbacks are > > not > > > > guaranteed to be called here. > > > > > > > > More important issue is whether it's safe to enter V8 in the associated > weak > > > > callback. I don't think we can survive nested scavenges. > > > > > > Prologue/epilogue will be called with kGCTypeScavenge. See > > > Heap::PerformGarbageCollection. > > > > The ones taking the type argument are only used for stats, the "real" > callbacks > > are named global_gc_{prologue,epilogue}_callback_ and I think are only called > > during major GCs. > > > > > There is also nothing nested about the GC that will happen in callback (no > > > difference between MS and Scavenge in this respect). Callbacks are called > when > > > GC is complete and the state of heap is "operational". > > > > Right, it's most likely not a problem. I assumed the callbacks are being > called > > early. Still I agree with Anton that more testing of this kind of corner cases > > would be nice. > > Yep you are right about callback. Global one will only be invoked for full > collection. > > Test added. > > But frankly speaking I do not think that V8 has to guarantee any ordering > between prologue/epilogue callbacks and weak handle callbacks. There are e.g. active DOM objects with weak handles that are temporarily made strong by the prologue callback and weak again by the epilogue callback. So we can't mark them as independent even though they don't participate in object grouping. http://codereview.chromium.org/7031005/diff/1/src/heap.cc File src/heap.cc (right): http://codereview.chromium.org/7031005/diff/1/src/heap.cc#newcode777 src/heap.cc:777: isolate_->global_handles()->PostGarbageCollectionProcessing(); On 2011/05/16 14:55:34, Vyacheslav Egorov wrote: > On 2011/05/16 14:31:22, Vitaly Repeshko wrote: > > On 2011/05/16 13:42:11, Vitaly Repeshko wrote: > > > This can invoke callbacks for pending dependent handles left over from > > previous > > > major GCs. (Because some of them can trigger GCs.) Given that the > > > prologue/epilogue callbacks won't be called it might be a problem. > > > > If I'm right about the prologue/epilogue callbacks not being called, it could > be > > a problem because weak callbacks used to be called between the > prologue/epilogue > > pair. > > We should not mark those handles that depend on prologue/epilogue callbacks as > independent. The whole idea of independent handle is that it, well, does not > depend on anything. Right, but please not that there can be _dependent_ callbacks still pending. Those can rely on the prologue/epilogue callbacks.
http://codereview.chromium.org/7031005/diff/1/src/heap.cc File src/heap.cc (right): http://codereview.chromium.org/7031005/diff/1/src/heap.cc#newcode777 src/heap.cc:777: isolate_->global_handles()->PostGarbageCollectionProcessing(); On 2011/05/16 15:04:03, Vitaly Repeshko wrote: > On 2011/05/16 14:55:34, Vyacheslav Egorov wrote: > > On 2011/05/16 14:31:22, Vitaly Repeshko wrote: > > > On 2011/05/16 13:42:11, Vitaly Repeshko wrote: > > > > This can invoke callbacks for pending dependent handles left over from > > > previous > > > > major GCs. (Because some of them can trigger GCs.) Given that the > > > > prologue/epilogue callbacks won't be called it might be a problem. > > > > > > If I'm right about the prologue/epilogue callbacks not being called, it > could > > be > > > a problem because weak callbacks used to be called between the > > prologue/epilogue > > > pair. > > > > We should not mark those handles that depend on prologue/epilogue callbacks as > > independent. The whole idea of independent handle is that it, well, does not > > depend on anything. > > Right, but please not that there can be _dependent_ callbacks still pending. > Those can rely on the prologue/epilogue callbacks. Argh. This is true. If those callbacks rely on being called inside "brackets" of callbacks they will blow up. Ok, for the sake of keeping old code up and running I will not invoke any callbacks for dependent handles in scavenge.
http://codereview.chromium.org/7031005/diff/1/src/global-handles.cc File src/global-handles.cc (right): http://codereview.chromium.org/7031005/diff/1/src/global-handles.cc#newcode330 src/global-handles.cc:330: Handle<Object> GlobalHandles::Create(Object* value) { Sorry, missed this one. On 2011/05/16 14:55:34, Vyacheslav Egorov wrote: > On 2011/05/16 14:13:19, antonm wrote: > > I think you should reset independent flag here. > > It's done by result->Initialize(value) below. Am I missing some code path? http://codereview.chromium.org/7031005/diff/2003/test/cctest/test-api.cc File test/cctest/test-api.cc (right): http://codereview.chromium.org/7031005/diff/2003/test/cctest/test-api.cc#newc... test/cctest/test-api.cc:4472: obj.Dispose(); this slightly worries me. Currently we use data read from object itself to do some dispatches (well, maybe not in the case of WebGL arrays, but definitely for nodes). Now I must be careful to fetch those data before object is collected another time. Which is sometimes hard to ensure. The second point. Actually whatever I do in weak callback, object is already collected---I cannot revive it or pass a reference to some other place. Ideally, it should be enforced on API level---Vitaly had a nice idea of passing only custom data in such a callback which solves all this problem. But as the very least, may you update PostGarbage... and assert that independent global handles are disposed in weak callback (normal callbacks may revive them).
http://codereview.chromium.org/7031005/diff/2003/test/cctest/test-api.cc File test/cctest/test-api.cc (right): http://codereview.chromium.org/7031005/diff/2003/test/cctest/test-api.cc#newc... test/cctest/test-api.cc:4472: obj.Dispose(); On 2011/05/16 15:20:44, antonm wrote: > this slightly worries me. Currently we use data read from object itself to do > some dispatches (well, maybe not in the case of WebGL arrays, but definitely for > nodes). Now I must be careful to fetch those data before object is collected > another time. Which is sometimes hard to ensure. > > The second point. Actually whatever I do in weak callback, object is already > collected---I cannot revive it or pass a reference to some other place. > Ideally, it should be enforced on API level---Vitaly had a nice idea of passing > only custom data in such a callback which solves all this problem. > > But as the very least, may you update PostGarbage... and assert that independent > global handles are disposed in weak callback (normal callbacks may revive them). Sorry, I am not following. Could you elaborate your concerns? The behavior of independent handles in this respect is no different from dependent. You _can_ revive object pointed by independent handle. Just ClearWeak and you are done, just like with a normal dependent handle. Scavenger handles independent weak handles just like MarkSweep handles all handles: it preserves objects, it does not leave them in the inactive semispace. I decided not to go for such a drastic API contract change.
http://codereview.chromium.org/7031005/diff/2003/test/cctest/test-api.cc File test/cctest/test-api.cc (right): http://codereview.chromium.org/7031005/diff/2003/test/cctest/test-api.cc#newc... test/cctest/test-api.cc:4472: obj.Dispose(); Sorry, I was wrong in my assumption. On 2011/05/16 15:29:44, Vyacheslav Egorov wrote: > On 2011/05/16 15:20:44, antonm wrote: > > this slightly worries me. Currently we use data read from object itself to > do > > some dispatches (well, maybe not in the case of WebGL arrays, but definitely > for > > nodes). Now I must be careful to fetch those data before object is collected > > another time. Which is sometimes hard to ensure. > > > > The second point. Actually whatever I do in weak callback, object is already > > collected---I cannot revive it or pass a reference to some other place. > > Ideally, it should be enforced on API level---Vitaly had a nice idea of > passing > > only custom data in such a callback which solves all this problem. > > > > But as the very least, may you update PostGarbage... and assert that > independent > > global handles are disposed in weak callback (normal callbacks may revive > them). > > Sorry, I am not following. Could you elaborate your concerns? The behavior of > independent handles in this respect is no different from dependent. You _can_ > revive object pointed by independent handle. Just ClearWeak and you are done, > just like with a normal dependent handle. > > Scavenger handles independent weak handles just like MarkSweep handles all > handles: it preserves objects, it does not leave them in the inactive semispace. > I decided not to go for such a drastic API contract change.
LGTM with a few final comments. http://codereview.chromium.org/7031005/diff/1/src/heap.cc File src/heap.cc (right): http://codereview.chromium.org/7031005/diff/1/src/heap.cc#newcode1037 src/heap.cc:1037: isolate_->global_handles()->IdentifyWeakIndependentHandles( On 2011/05/16 14:55:34, Vyacheslav Egorov wrote: > On 2011/05/16 14:31:22, Vitaly Repeshko wrote: > > On 2011/05/16 14:15:46, Vyacheslav Egorov wrote: > > > On 2011/05/16 13:42:11, Vitaly Repeshko wrote: > > > > Scanning the linked list of handles was really slow in some benchmarks. We > > > > should merge these two passes because the scavenges should be as fast as > > > > possible. > > > > > > If there are two weak handles that point to the same object them both should > > > have their callbacks called. > > > > > > Thus merging these two passes together would require additional mark > somewhere > > > on the object to distinguish objects evacuated by normal scavenging and > > objects > > > evacuated by scavenging of weak independent handles. We probably can use the > > > second bit from map word but this would complicate things. > > > > Ahh, this is annoying. Can we at least inline the "identify" callback? > > To inline identify callback we need to move IdentifyWeakIndependentHandles into > global-handles-inl.h. > > I can do that but I really think it's premature optimization. I have not seen > any significant degradation on a test that had dozens of thousands of > independent handles created and destroyed every second. > > Alternatively we can have separate lists for independent and dependent handles > so we will not pay anything for dependent handles when we don't need to iterate > over them. I do remember seeing scavenge time increase when I experimented with a similar approach for DOM nodes. But it was more than a year ago. I've got an idea on how to merge the passes: we can detect when an object was evacuated by comparing its forwarded address with a saved new space top. Feel free to postpone doing this (if it works at all) or other optimizations. http://codereview.chromium.org/7031005/diff/8001/include/v8.h File include/v8.h (right): http://codereview.chromium.org/7031005/diff/8001/include/v8.h#newcode392 include/v8.h:392: * is free to ignore any object groups containing this object. I still think this comment should be updated. http://codereview.chromium.org/7031005/diff/8001/test/cctest/test-api.cc File test/cctest/test-api.cc (right): http://codereview.chromium.org/7031005/diff/8001/test/cctest/test-api.cc#newc... test/cctest/test-api.cc:4487: THREADED_TEST(GCFromWeakCallbacks) { Please add a test where an independent weak callback makes an object live again. http://codereview.chromium.org/7031005/diff/8001/test/cctest/test-api.cc#newc... test/cctest/test-api.cc:4498: for (int outer_gc = 0; outer_gc < kNumberOfGCTypes; outer_gc++) { Nice! http://codereview.chromium.org/7031005/diff/8001/test/cctest/test-api.cc#newc... test/cctest/test-api.cc:4513: nit: Remove extra blank line.
Thanks, Vitaly & Anton! I'll be landing. http://codereview.chromium.org/7031005/diff/1/src/heap.cc File src/heap.cc (right): http://codereview.chromium.org/7031005/diff/1/src/heap.cc#newcode1037 src/heap.cc:1037: isolate_->global_handles()->IdentifyWeakIndependentHandles( On 2011/05/16 21:40:37, Vitaly Repeshko wrote: > On 2011/05/16 14:55:34, Vyacheslav Egorov wrote: > > On 2011/05/16 14:31:22, Vitaly Repeshko wrote: > > > On 2011/05/16 14:15:46, Vyacheslav Egorov wrote: > > > > On 2011/05/16 13:42:11, Vitaly Repeshko wrote: > > > > > Scanning the linked list of handles was really slow in some benchmarks. > We > > > > > should merge these two passes because the scavenges should be as fast as > > > > > possible. > > > > > > > > If there are two weak handles that point to the same object them both > should > > > > have their callbacks called. > > > > > > > > Thus merging these two passes together would require additional mark > > somewhere > > > > on the object to distinguish objects evacuated by normal scavenging and > > > objects > > > > evacuated by scavenging of weak independent handles. We probably can use > the > > > > second bit from map word but this would complicate things. > > > > > > Ahh, this is annoying. Can we at least inline the "identify" callback? > > > > To inline identify callback we need to move IdentifyWeakIndependentHandles > into > > global-handles-inl.h. > > > > I can do that but I really think it's premature optimization. I have not seen > > any significant degradation on a test that had dozens of thousands of > > independent handles created and destroyed every second. > > > > Alternatively we can have separate lists for independent and dependent handles > > so we will not pay anything for dependent handles when we don't need to > iterate > > over them. > > I do remember seeing scavenge time increase when I experimented with a similar > approach for DOM nodes. But it was more than a year ago. > > I've got an idea on how to merge the passes: we can detect when an object was > evacuated by comparing its forwarded address with a saved new space top. > > Feel free to postpone doing this (if it works at all) or other optimizations. Very neat! But unfortunately will not work for promoted objects as they have no imposed linear order. I will postpone optimization. http://codereview.chromium.org/7031005/diff/8001/include/v8.h File include/v8.h (right): http://codereview.chromium.org/7031005/diff/8001/include/v8.h#newcode392 include/v8.h:392: * is free to ignore any object groups containing this object. On 2011/05/16 21:40:37, Vitaly Repeshko wrote: > I still think this comment should be updated. Done. http://codereview.chromium.org/7031005/diff/8001/test/cctest/test-api.cc File test/cctest/test-api.cc (right): http://codereview.chromium.org/7031005/diff/8001/test/cctest/test-api.cc#newc... test/cctest/test-api.cc:4487: THREADED_TEST(GCFromWeakCallbacks) { On 2011/05/16 21:40:37, Vitaly Repeshko wrote: > Please add a test where an independent weak callback makes an object live again. Done. http://codereview.chromium.org/7031005/diff/8001/test/cctest/test-api.cc#newc... test/cctest/test-api.cc:4513: On 2011/05/16 21:40:37, Vitaly Repeshko wrote: > nit: Remove extra blank line. Done. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
