Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
623033002:
Oilpan: Add support of pre-finalization callback to Oilpan infrastructure. (Closed)
|
Created:
6 years, 2 months ago by tkent Modified:
6 years, 2 months ago CC:
blink-reviews, Mads Ager (chromium), mkwst+moarreviews_chromium.org, dgrogan, cmumford, jsbell+idb_chromium.org, kouhei+heap_chromium.org Base URL:
svn://svn.chromium.org/blink/trunk Project:
blink Visibility:
Public. |
DescriptionOilpan: Add support of pre-finalization callback to Oilpan infrastructure.
This CL introduces:
- ThreadState::registerPreFinalizer(T&)
- ThreadState::unregisterPreFinalizer(T&)
- USING_PRE_FINALIZER(Class, method)
They are used like the following:
class Foo : GarbageCollected<Foo> {
USING_PRE_FINALIZER(Foo, dispose);
public:
Foo() { ThreadState::current()->registerPreFinalizer(*this); }
void trace(Visitor*);
...
private:
void dispose();
Member<Bar> m_bar;
};
void Foo::dispose()
{
m_bar->doesSomething();
}
BUG=420515
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=183253
Patch Set 1 #
Total comments: 25
Patch Set 2 : apply review comments #
Total comments: 14
Patch Set 3 : typos, add an aasertion #
Total comments: 8
Patch Set 4 : Fix a wrong example in a comment #Patch Set 5 : Update a comment #Patch Set 6 : Add a FIXME #
Total comments: 5
Messages
Total messages: 32 (6 generated)
tkent@chromium.org changed reviewers: + oilpan-reviews@chromium.org
Please review this. https://codereview.chromium.org/623033002/diff/1/Source/platform/heap/ThreadS... File Source/platform/heap/ThreadState.h (right): https://codereview.chromium.org/623033002/diff/1/Source/platform/heap/ThreadS... Source/platform/heap/ThreadState.h:153: static bool willFinalizeDeadGarbageCollectedObject(void* object, Visitor& visitor) \ wrong indentation. will fix.
haraken@chromium.org changed reviewers: + haraken@chromium.org
The approach looks good. https://codereview.chromium.org/623033002/diff/1/Source/platform/heap/ThreadS... File Source/platform/heap/ThreadState.h (right): https://codereview.chromium.org/623033002/diff/1/Source/platform/heap/ThreadS... Source/platform/heap/ThreadState.h:132: // other garbarge-collected members. However we must not allocate Slightly clearer: (a) The function can access garbage-collected objects allocated on the thread heap. (b) The function cannot access garbage-collected objects allocated on other thread heaps. (c) The function cannot allocate garbage-collected objects. (d) The function cannot change an object graph. (Note related to CL: We already have an assert for (c). Would it be possible to add an assert for (b) and (d)? I think we can hook Member::operator->, Member::operator= etc, but I'm a bit afraid that adding an assert to Member::operator-> etc will unacceptably slow down performance of Debug builds.) https://codereview.chromium.org/623033002/diff/1/Source/platform/heap/ThreadS... Source/platform/heap/ThreadState.h:151: #define USING_PRE_FINALIZATION_CALLBACK(Class, method) \ USING_PRE_FINALIZATION_CALLBACK => USING_PRE_FINALIZER ? (Another possible name would be USING_NEAR_DEATH_CALLBACK. I don't have a strong opinion here. V8 is using "near-death" notifications.) https://codereview.chromium.org/623033002/diff/1/Source/platform/heap/ThreadS... Source/platform/heap/ThreadState.h:153: static bool willFinalizeDeadGarbageCollectedObject(void* object, Visitor& visitor) \ willFinalizeDeadGarbageCollectedObject => invokePreFinalizer ? https://codereview.chromium.org/623033002/diff/1/Source/platform/heap/ThreadS... Source/platform/heap/ThreadState.h:662: // Request to call a pref-finalize function of the target object before the pre-finalizer https://codereview.chromium.org/623033002/diff/1/Source/platform/heap/ThreadS... Source/platform/heap/ThreadState.h:669: void registerObjectWithPreFinalizer(T& target) registerObjectWithPreFinalizer => registerPreFinalizer ? https://codereview.chromium.org/623033002/diff/1/Source/platform/heap/ThreadS... Source/platform/heap/ThreadState.h:671: ASSERT(!m_objectsWithPreFinalizer.contains(&target)); Shall we add ASSERT(!isSweepInProgress()) ? https://codereview.chromium.org/623033002/diff/1/Source/platform/heap/ThreadS... Source/platform/heap/ThreadState.h:677: void unregisterObjectWithPreFinalizer(T& target) unregisterObjectWithPreFinalizer => unregisterPreFinalizer ? https://codereview.chromium.org/623033002/diff/1/Source/platform/heap/ThreadS... Source/platform/heap/ThreadState.h:722: void invokePreFinalizer(Visitor&); invokePreFinalizer => invokePreFinalizers ? https://codereview.chromium.org/623033002/diff/1/Source/platform/heap/ThreadS... Source/platform/heap/ThreadState.h:774: HashMap<void*, bool (*)(void*, Visitor&)> m_objectsWithPreFinalizer; m_objectsWithPreFinalizer => m_preFinalizers ?
https://codereview.chromium.org/623033002/diff/1/Source/platform/heap/ThreadS... File Source/platform/heap/ThreadState.h (right): https://codereview.chromium.org/623033002/diff/1/Source/platform/heap/ThreadS... Source/platform/heap/ThreadState.h:132: // other garbarge-collected members. However we must not allocate On 2014/10/03 06:00:32, haraken wrote: > > Slightly clearer: > > (a) The function can access garbage-collected objects allocated on the thread > heap. > (b) The function cannot access garbage-collected objects allocated on other > thread heaps. > (c) The function cannot allocate garbage-collected objects. > (d) The function cannot change an object graph. > > (Note related to CL: We already have an assert for (c). Would it be possible to > add an assert for (b) and (d)? I think we can hook Member::operator->, > Member::operator= etc, but I'm a bit afraid that adding an assert to > Member::operator-> etc will unacceptably slow down performance of Debug builds.) (A), (b), (c), and (d) are correct. I should add them to the code comment. I also think complexity to detect (b) and (d) would be unacceptable. These restrictions are same as the existing HashMap<WeakMember<>> pattern, which is working well now. So documentation would be enough. https://codereview.chromium.org/623033002/diff/1/Source/platform/heap/ThreadS... Source/platform/heap/ThreadState.h:153: static bool willFinalizeDeadGarbageCollectedObject(void* object, Visitor& visitor) \ On 2014/10/03 06:00:31, haraken wrote: > > willFinalizeDeadGarbageCollectedObject => invokePreFinalizer ? Yeah, it's inconsistent with other names in this patch though I wanted to make this consistent with GarbageCollectedFinalized::finalizeGarbageCollectedObject. invokePreFinalizerOfGarbageCollectedObject?
https://codereview.chromium.org/623033002/diff/1/Source/platform/heap/ThreadS... File Source/platform/heap/ThreadState.h (right): https://codereview.chromium.org/623033002/diff/1/Source/platform/heap/ThreadS... Source/platform/heap/ThreadState.h:132: // other garbarge-collected members. However we must not allocate On 2014/10/03 06:34:01, tkent (high review load) wrote: > On 2014/10/03 06:00:32, haraken wrote: > > > > Slightly clearer: > > > > (a) The function can access garbage-collected objects allocated on the thread > > heap. > > (b) The function cannot access garbage-collected objects allocated on other > > thread heaps. > > (c) The function cannot allocate garbage-collected objects. > > (d) The function cannot change an object graph. > > > > (Note related to CL: We already have an assert for (c). Would it be possible > to > > add an assert for (b) and (d)? I think we can hook Member::operator->, > > Member::operator= etc, but I'm a bit afraid that adding an assert to > > Member::operator-> etc will unacceptably slow down performance of Debug > builds.) > > (A), (b), (c), and (d) are correct. I should add them to the code comment. > > I also think complexity to detect (b) and (d) would be unacceptable. These > restrictions are same as the existing HashMap<WeakMember<>> pattern, which is > working well now. So documentation would be enough. Agreed. https://codereview.chromium.org/623033002/diff/1/Source/platform/heap/ThreadS... Source/platform/heap/ThreadState.h:153: static bool willFinalizeDeadGarbageCollectedObject(void* object, Visitor& visitor) \ On 2014/10/03 06:34:01, tkent (high review load) wrote: > On 2014/10/03 06:00:31, haraken wrote: > > > > willFinalizeDeadGarbageCollectedObject => invokePreFinalizer ? > > Yeah, it's inconsistent with other names in this patch though I wanted to make > this consistent with GarbageCollectedFinalized::finalizeGarbageCollectedObject. > invokePreFinalizerOfGarbageCollectedObject? I don't have a strong opinion here, but I guess invokePreFinalizer would be enough and consistent with invokePreFinalizers.
erik.corry@gmail.com changed reviewers: + erik.corry@gmail.com
I'm not seeing any tests here. Are the prefinalization callbacks run on the thread to which the object belongs, or on the GC thread?
https://codereview.chromium.org/623033002/diff/1/Source/modules/indexeddb/IDB... File Source/modules/indexeddb/IDBRequest.h (left): https://codereview.chromium.org/623033002/diff/1/Source/modules/indexeddb/IDB... Source/modules/indexeddb/IDBRequest.h:65: void dispose(); I'd prefer to keep this https://codereview.chromium.org/623033002/diff/1/Source/platform/heap/ThreadS... File Source/platform/heap/ThreadState.h (right): https://codereview.chromium.org/623033002/diff/1/Source/platform/heap/ThreadS... Source/platform/heap/ThreadState.h:162: void method() To my mind, declaring the method in the macro like this puts more magic in the macro than it needs to have. It's not much work to declare the method explicitly, and makes the code easier to navigate. Otherwise the reader is left wondering where this 'dispose' method comes from. If you were willing to fix the name of the 'dispose' method you could probably remove this ugly macro completely, and replace it with a single templatized static method outside the classes. That would mean the dispose method needed to be public, which I think is OK, since it is essentially called from outside the object (by the GC infrastructure).
Updated the patch. - Consistent and simplified names. - Add more comments - The macro don't add a declaration of the specified function. - Add unit test Erik wrote: > I'm not seeing any tests here. Done. > Are the prefinalization callbacks run on the thread to which the object belongs, > or on the GC thread? They should be run on the object owner thread, not sweeping thread. I have added a code comment. https://codereview.chromium.org/623033002/diff/1/Source/platform/heap/ThreadS... File Source/platform/heap/ThreadState.h (right): https://codereview.chromium.org/623033002/diff/1/Source/platform/heap/ThreadS... Source/platform/heap/ThreadState.h:151: #define USING_PRE_FINALIZATION_CALLBACK(Class, method) \ On 2014/10/03 06:00:31, haraken wrote: > > USING_PRE_FINALIZATION_CALLBACK => USING_PRE_FINALIZER ? > > (Another possible name would be USING_NEAR_DEATH_CALLBACK. I don't have a strong > opinion here. V8 is using "near-death" notifications.) Done. https://codereview.chromium.org/623033002/diff/1/Source/platform/heap/ThreadS... Source/platform/heap/ThreadState.h:153: static bool willFinalizeDeadGarbageCollectedObject(void* object, Visitor& visitor) \ On 2014/10/03 06:00:31, haraken wrote: > > willFinalizeDeadGarbageCollectedObject => invokePreFinalizer ? Done. https://codereview.chromium.org/623033002/diff/1/Source/platform/heap/ThreadS... Source/platform/heap/ThreadState.h:162: void method() On 2014/10/03 09:13:26, Erik Corry wrote: > To my mind, declaring the method in the macro like this puts more magic in the > macro than it needs to have. It's not much work to declare the method > explicitly, and makes the code easier to navigate. Otherwise the reader is left > wondering where this 'dispose' method comes from. > > If you were willing to fix the name of the 'dispose' method you could probably > remove this ugly macro completely, and replace it with a single templatized > static method outside the classes. That would mean the dispose method needed to > be public, which I think is OK, since it is essentially called from outside the > object (by the GC infrastructure). Removed the hidden declaration. https://codereview.chromium.org/623033002/diff/1/Source/platform/heap/ThreadS... Source/platform/heap/ThreadState.h:662: // Request to call a pref-finalize function of the target object before the On 2014/10/03 06:00:31, haraken wrote: > > pre-finalizer Done. https://codereview.chromium.org/623033002/diff/1/Source/platform/heap/ThreadS... Source/platform/heap/ThreadState.h:669: void registerObjectWithPreFinalizer(T& target) On 2014/10/03 06:00:31, haraken wrote: > > registerObjectWithPreFinalizer => registerPreFinalizer ? Done. https://codereview.chromium.org/623033002/diff/1/Source/platform/heap/ThreadS... Source/platform/heap/ThreadState.h:671: ASSERT(!m_objectsWithPreFinalizer.contains(&target)); On 2014/10/03 06:00:31, haraken wrote: > > Shall we add ASSERT(!isSweepInProgress()) ? Done. https://codereview.chromium.org/623033002/diff/1/Source/platform/heap/ThreadS... Source/platform/heap/ThreadState.h:677: void unregisterObjectWithPreFinalizer(T& target) On 2014/10/03 06:00:32, haraken wrote: > > unregisterObjectWithPreFinalizer => unregisterPreFinalizer ? Done. https://codereview.chromium.org/623033002/diff/1/Source/platform/heap/ThreadS... Source/platform/heap/ThreadState.h:722: void invokePreFinalizer(Visitor&); On 2014/10/03 06:00:32, haraken wrote: > > invokePreFinalizer => invokePreFinalizers ? Done. https://codereview.chromium.org/623033002/diff/1/Source/platform/heap/ThreadS... Source/platform/heap/ThreadState.h:774: HashMap<void*, bool (*)(void*, Visitor&)> m_objectsWithPreFinalizer; On 2014/10/03 06:00:32, haraken wrote: > > m_objectsWithPreFinalizer => m_preFinalizers ? Done.
On 2014/10/06 01:25:53, tkent wrote: > Updated the patch. > - Consistent and simplified names. > - Add more comments > - The macro don't add a declaration of the specified function. > - Add unit test Also, I added TRACE_EVENT0() only for invokePreFinalizers().
Mostly looks good. https://codereview.chromium.org/623033002/diff/20001/Source/platform/heap/Thr... File Source/platform/heap/ThreadState.cpp (right): https://codereview.chromium.org/623033002/diff/20001/Source/platform/heap/Thr... Source/platform/heap/ThreadState.cpp:1119: invokePreFinalizers(*Heap::s_markingVisitor); Just to confirm: The main difference between weak callbacks and pre-finalizers is that pre-finalizers are always called when the target objects die whereas weak callbacks are not called if the target objects die together with the observing objects, right? Probably we might want to make the point clearer in the comment. https://codereview.chromium.org/623033002/diff/20001/Source/platform/heap/Thr... File Source/platform/heap/ThreadState.h (right): https://codereview.chromium.org/623033002/diff/20001/Source/platform/heap/Thr... Source/platform/heap/ThreadState.h:134: // allocate new garbage-collected objects, and must not update Member<> and , and => nor https://codereview.chromium.org/623033002/diff/20001/Source/platform/heap/Thr... Source/platform/heap/ThreadState.h:156: static bool invokePreFinalier(void* object, Visitor& visitor) \ invokePreFinalier => invokePreFinalizer https://codereview.chromium.org/623033002/diff/20001/Source/platform/heap/Thr... Source/platform/heap/ThreadState.h:164: typedef char UsingPreFinazlizerMacroNeedsTrailingSemiColon What's this? https://codereview.chromium.org/623033002/diff/20001/Source/platform/heap/Thr... Source/platform/heap/ThreadState.h:664: // Request to call a pref-finalizer function of the target object before the a pref-finalizer function => a pre-finalizer https://codereview.chromium.org/623033002/diff/20001/Source/platform/heap/Thr... Source/platform/heap/ThreadState.h:666: // argument should be |*this|. Registering a lot of objects affects GC Would it be possible to define registerPreFinalizer/unregisterPreFinalizer on a GarbageCollected class so that the caller side can avoid passing |*this|? https://codereview.chromium.org/623033002/diff/20001/Source/platform/heap/Thr... Source/platform/heap/ThreadState.h:777: HashMap<void*, bool (*)(void*, Visitor&)> m_preFinalizers; Can we add an assertion to verify that m_preFinalizers is empty when the thread shuts down?
https://codereview.chromium.org/623033002/diff/20001/Source/platform/heap/Thr... File Source/platform/heap/ThreadState.h (right): https://codereview.chromium.org/623033002/diff/20001/Source/platform/heap/Thr... Source/platform/heap/ThreadState.h:134: // allocate new garbage-collected objects, and must not update Member<> and On 2014/10/06 01:55:27, haraken wrote: > > , and => nor Done. https://codereview.chromium.org/623033002/diff/20001/Source/platform/heap/Thr... Source/platform/heap/ThreadState.h:156: static bool invokePreFinalier(void* object, Visitor& visitor) \ On 2014/10/06 01:55:27, haraken wrote: > > invokePreFinalier => invokePreFinalizer Done. https://codereview.chromium.org/623033002/diff/20001/Source/platform/heap/Thr... Source/platform/heap/ThreadState.h:164: typedef char UsingPreFinazlizerMacroNeedsTrailingSemiColon On 2014/10/06 01:55:27, haraken wrote: > > What's this? This is an idiom to make ';' after USING_PRE_FINALIZER() mandatory. With this line, the following will cause a compile error USING_PRE_FINALIZER(Foo, dispose) This line helps to avoid to have both of |USING_PRE_FINALIZER();| and |USING_PRE_FINALIZER()|. https://codereview.chromium.org/623033002/diff/20001/Source/platform/heap/Thr... Source/platform/heap/ThreadState.h:664: // Request to call a pref-finalizer function of the target object before the On 2014/10/06 01:55:27, haraken wrote: > > a pref-finalizer function => a pre-finalizer Done. https://codereview.chromium.org/623033002/diff/20001/Source/platform/heap/Thr... Source/platform/heap/ThreadState.h:666: // argument should be |*this|. Registering a lot of objects affects GC On 2014/10/06 01:55:27, haraken wrote: > > Would it be possible to define registerPreFinalizer/unregisterPreFinalizer on a > GarbageCollected class so that the caller side can avoid passing |*this|? It's not impossible, however I think it's not an improvement. ThreadState needs to know a specific type information to get T::invokePreFinalizer address. If we added these functions to GarbageCollected, we would need to pass address of T::invokePreFinalizer like: registerPreFinalizer(&IDBRequest::invokePreFinalizer); This looks worse than |*this|. https://codereview.chromium.org/623033002/diff/20001/Source/platform/heap/Thr... Source/platform/heap/ThreadState.h:777: HashMap<void*, bool (*)(void*, Visitor&)> m_preFinalizers; On 2014/10/06 01:55:28, haraken wrote: > > Can we add an assertion to verify that m_preFinalizers is empty when the thread > shuts down? Done.
LGTM https://codereview.chromium.org/623033002/diff/20001/Source/platform/heap/Thr... File Source/platform/heap/ThreadState.h (right): https://codereview.chromium.org/623033002/diff/20001/Source/platform/heap/Thr... Source/platform/heap/ThreadState.h:666: // argument should be |*this|. Registering a lot of objects affects GC On 2014/10/06 05:01:43, tkent wrote: > On 2014/10/06 01:55:27, haraken wrote: > > > > Would it be possible to define registerPreFinalizer/unregisterPreFinalizer on > a > > GarbageCollected class so that the caller side can avoid passing |*this|? > > It's not impossible, however I think it's not an improvement. > > ThreadState needs to know a specific type information to get > T::invokePreFinalizer address. > If we added these functions to GarbageCollected, we would need to pass address > of T::invokePreFinalizer like: > registerPreFinalizer(&IDBRequest::invokePreFinalizer); > This looks worse than |*this|. Makes sense. https://codereview.chromium.org/623033002/diff/40001/Source/platform/heap/Thr... File Source/platform/heap/ThreadState.h (right): https://codereview.chromium.org/623033002/diff/40001/Source/platform/heap/Thr... Source/platform/heap/ThreadState.h:135: // Persistent<> pointers. nor must update => nor update (I'm not familiar with the usage of 'nor', but the safest way to say this would be: we must not allocate ... nor update ....) https://codereview.chromium.org/623033002/diff/40001/Source/platform/heap/Thr... Source/platform/heap/ThreadState.h:142: // HeapHashMap are collected at same time. I'm a bit confused with this description. class Bar : public GarbageCollected<Bar> { HeapHashMap<WeakMember<Foo>, OwnPtr<Disposer>> m_map; }; If the Bar object and the Foo objects become unreachable at the same time, the Disposer's destructors won't be called at all, will it? More accurately, if the Bar object becomes unreachable, the Disposer's destructors won't be called at all. Thus we cannot use the idiom if we want to have Foo::dispose called whenever the Foo object becomes unreachable. If we want to do that, we need to use the pre-finalizer. Am I understanding correctly? https://codereview.chromium.org/623033002/diff/40001/Source/platform/heap/Thr... Source/platform/heap/ThreadState.h:153: // registerPreFinalizer(); registerPreFinalizer(*this);
https://codereview.chromium.org/623033002/diff/40001/Source/platform/heap/Thr... File Source/platform/heap/ThreadState.h (right): https://codereview.chromium.org/623033002/diff/40001/Source/platform/heap/Thr... Source/platform/heap/ThreadState.h:142: // HeapHashMap are collected at same time. On 2014/10/06 05:26:42, haraken wrote: > > I'm a bit confused with this description. > > class Bar : public GarbageCollected<Bar> { > HeapHashMap<WeakMember<Foo>, OwnPtr<Disposer>> m_map; > }; > > If the Bar object and the Foo objects become unreachable at the same time, the > Disposer's destructors won't be called at all, will it? More accurately, if the > Bar object becomes unreachable, the Disposer's destructors won't be called at > all. It's not correct. ~Disposer is called when a backing store of HeapHashMap is swept. If ~Foo is called before ~Disposer, this causes a use-after-free. In order to avoid UAF, we need an assumption, or need to make it PersistentHeapHashMap. e.g. AudioContext::m_liveNodes is safe because AudioContext always outlives any AudioNodes. > Thus we cannot use the idiom if we want to have Foo::dispose called whenever the > Foo object becomes unreachable. If we want to do that, we need to use the > pre-finalizer. Right.
https://codereview.chromium.org/623033002/diff/40001/Source/platform/heap/Thr... File Source/platform/heap/ThreadState.h (right): https://codereview.chromium.org/623033002/diff/40001/Source/platform/heap/Thr... Source/platform/heap/ThreadState.h:142: // HeapHashMap are collected at same time. On 2014/10/06 06:02:24, tkent wrote: > On 2014/10/06 05:26:42, haraken wrote: > > > > I'm a bit confused with this description. > > > > class Bar : public GarbageCollected<Bar> { > > HeapHashMap<WeakMember<Foo>, OwnPtr<Disposer>> m_map; > > }; > > > > If the Bar object and the Foo objects become unreachable at the same time, the > > Disposer's destructors won't be called at all, will it? More accurately, if > the > > Bar object becomes unreachable, the Disposer's destructors won't be called at > > all. > > It's not correct. ~Disposer is called when a backing store of HeapHashMap is > swept. If ~Foo is called before ~Disposer, this causes a use-after-free. > In order to avoid UAF, we need an assumption, or need to make it > PersistentHeapHashMap. e.g. AudioContext::m_liveNodes is safe because > AudioContext always outlives any AudioNodes. Thanks, understood. Then is the only difference whether it is guaranteed that ~Disposer() gets called before ~Foo()? I begin to think that from the perspective of GC overhead, there is no difference between the HeapHashMap<WeakMember...> idiom and the pre-finalizer. If so, we should probably aim at replacing all the HeapHashMap<WeakMember...> idioms with the pre-finalizers. What do you think?
https://codereview.chromium.org/623033002/diff/40001/Source/platform/heap/Thr... File Source/platform/heap/ThreadState.h (right): https://codereview.chromium.org/623033002/diff/40001/Source/platform/heap/Thr... Source/platform/heap/ThreadState.h:142: // HeapHashMap are collected at same time. On 2014/10/06 06:28:09, haraken wrote: > On 2014/10/06 06:02:24, tkent wrote: > > On 2014/10/06 05:26:42, haraken wrote: > > > > > > I'm a bit confused with this description. > > > > > > class Bar : public GarbageCollected<Bar> { > > > HeapHashMap<WeakMember<Foo>, OwnPtr<Disposer>> m_map; > > > }; > > > > > > If the Bar object and the Foo objects become unreachable at the same time, > the > > > Disposer's destructors won't be called at all, will it? More accurately, if > > the > > > Bar object becomes unreachable, the Disposer's destructors won't be called > at > > > all. > > > > It's not correct. ~Disposer is called when a backing store of HeapHashMap is > > swept. If ~Foo is called before ~Disposer, this causes a use-after-free. > > In order to avoid UAF, we need an assumption, or need to make it > > PersistentHeapHashMap. e.g. AudioContext::m_liveNodes is safe because > > AudioContext always outlives any AudioNodes. > > Thanks, understood. > > Then is the only difference whether it is guaranteed that ~Disposer() gets > called before ~Foo()? > > I begin to think that from the perspective of GC overhead, there is no > difference between the HeapHashMap<WeakMember...> idiom and the pre-finalizer. > If so, we should probably aim at replacing all the HeapHashMap<WeakMember...> > idioms with the pre-finalizers. What do you think? Right. Both of them have O(n) cost, and the pre-finalizer is much easier to use. We can replace existing usage of HeapHashMap<WeakMember<>, OwnPtr<Disposer>> idiom.
https://codereview.chromium.org/623033002/diff/40001/Source/platform/heap/Thr... File Source/platform/heap/ThreadState.h (right): https://codereview.chromium.org/623033002/diff/40001/Source/platform/heap/Thr... Source/platform/heap/ThreadState.h:142: // HeapHashMap are collected at same time. On 2014/10/06 07:00:52, tkent wrote: > On 2014/10/06 06:28:09, haraken wrote: > > On 2014/10/06 06:02:24, tkent wrote: > > > On 2014/10/06 05:26:42, haraken wrote: > > > > > > > > I'm a bit confused with this description. > > > > > > > > class Bar : public GarbageCollected<Bar> { > > > > HeapHashMap<WeakMember<Foo>, OwnPtr<Disposer>> m_map; > > > > }; > > > > > > > > If the Bar object and the Foo objects become unreachable at the same time, > > the > > > > Disposer's destructors won't be called at all, will it? More accurately, > if > > > the > > > > Bar object becomes unreachable, the Disposer's destructors won't be called > > at > > > > all. > > > > > > It's not correct. ~Disposer is called when a backing store of HeapHashMap > is > > > swept. If ~Foo is called before ~Disposer, this causes a use-after-free. > > > In order to avoid UAF, we need an assumption, or need to make it > > > PersistentHeapHashMap. e.g. AudioContext::m_liveNodes is safe because > > > AudioContext always outlives any AudioNodes. > > > > Thanks, understood. > > > > Then is the only difference whether it is guaranteed that ~Disposer() gets > > called before ~Foo()? > > > > I begin to think that from the perspective of GC overhead, there is no > > difference between the HeapHashMap<WeakMember...> idiom and the pre-finalizer. > > If so, we should probably aim at replacing all the HeapHashMap<WeakMember...> > > idioms with the pre-finalizers. What do you think? > > Right. Both of them have O(n) cost, and the pre-finalizer is much easier to > use. We can replace existing usage of HeapHashMap<WeakMember<>, > OwnPtr<Disposer>> idiom. Sounds great. Then let's replace this comment with a FIXME and mention that the HeapHashMap<WeakMember<>, ...> idiom should be replaced with the pre-finalizer idiom?
https://codereview.chromium.org/623033002/diff/40001/Source/platform/heap/Thr... File Source/platform/heap/ThreadState.h (right): https://codereview.chromium.org/623033002/diff/40001/Source/platform/heap/Thr... Source/platform/heap/ThreadState.h:142: // HeapHashMap are collected at same time. > Sounds great. Then let's replace this comment with a FIXME and mention that the > HeapHashMap<WeakMember<>, ...> idiom should be replaced with the pre-finalizer > idiom? Done. Added a FIXME.
Still LGTM.
The CQ bit was checked by tkent@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/623033002/100001
The CQ bit was unchecked by tkent@chromium.org
The CQ bit was checked by tkent@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/623033002/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as 183253
Message was sent while issue was closed.
https://codereview.chromium.org/623033002/diff/100001/Source/platform/heap/Th... File Source/platform/heap/ThreadState.h (right): https://codereview.chromium.org/623033002/diff/100001/Source/platform/heap/Th... Source/platform/heap/ThreadState.h:133: // garbarge-collected objects allocated in the thread. However we must not garbarge -> garbage https://codereview.chromium.org/623033002/diff/100001/Source/platform/heap/Th... Source/platform/heap/ThreadState.h:141: // outlives objects pointed by WeakMembers. This does not make sense to me. I'm also not happy with the dots in "HeapHashMap<WeakMember...>". Are you suggesting removing all weak heap hash maps, or only the ones with OwnPtr on the right? You only need the assumption that the map outlives the Foo objects if you are going to access the map from the destructor of the Disposer. Otherwise there is no issue. As for overhead, if you are going to remove items from the map one at a time from the pre-finalizer function, then that can be quite a big overhead. With a weak map solution, when the map dies, the items are not removed, we just call the Disposer destuctors.
Message was sent while issue was closed.
https://codereview.chromium.org/623033002/diff/100001/Source/platform/heap/Th... File Source/platform/heap/ThreadState.h (right): https://codereview.chromium.org/623033002/diff/100001/Source/platform/heap/Th... Source/platform/heap/ThreadState.h:141: // outlives objects pointed by WeakMembers. On 2014/10/06 12:39:18, Erik Corry wrote: > This does not make sense to me. I'm also not happy with the dots in > "HeapHashMap<WeakMember...>". Are you suggesting removing all weak heap hash > maps, or only the ones with OwnPtr on the right? We're just planning to remove the weak hash maps that have Disposers on the right. The Disposer pattern looks complicated, so we don't have two ways (i.e., the Disposer pattern and the pre-finalizer idiom) to achieve the same thing. > As for overhead, if you are going to remove items from the map one at a time > from the pre-finalizer function, then that can be quite a big overhead. With a > weak map solution, when the map dies, the items are not removed, we just call > the Disposer destuctors. Yeah. That being said, there are ~10 Disposers in the code base, so I hope the overhead won't become an issue.
Message was sent while issue was closed.
https://codereview.chromium.org/623033002/diff/100001/Source/platform/heap/Th... File Source/platform/heap/ThreadState.h (right): https://codereview.chromium.org/623033002/diff/100001/Source/platform/heap/Th... Source/platform/heap/ThreadState.h:133: // garbarge-collected objects allocated in the thread. However we must not On 2014/10/06 12:39:18, Erik Corry wrote: > garbarge -> garbage will fix. https://codereview.chromium.org/623033002/diff/100001/Source/platform/heap/Th... Source/platform/heap/ThreadState.h:141: // outlives objects pointed by WeakMembers. On 2014/10/06 12:39:18, Erik Corry wrote: > This does not make sense to me. I'm also not happy with the dots in > "HeapHashMap<WeakMember...>". Are you suggesting removing all weak heap hash > maps, or only the ones with OwnPtr on the right? The latter. I'll update the comment. > You only need the assumption that the map outlives the Foo objects if you are > going to access the map from the destructor of the Disposer. Otherwise there is > no issue. Right. > As for overhead, if you are going to remove items from the map one at a time > from the pre-finalizer function, then that can be quite a big overhead. With a > weak map solution, when the map dies, the items are not removed, we just call > the Disposer destuctors. ThreadState::invokePreFinalizers() might have a performance issue because HashMap::removeAll() can shrink repeatedly. We should optimize this if this is significant.
Message was sent while issue was closed.
> > As for overhead, if you are going to remove items from the map one at a time > > from the pre-finalizer function, then that can be quite a big overhead. With > a > > weak map solution, when the map dies, the items are not removed, we just call > > the Disposer destuctors. > > ThreadState::invokePreFinalizers() might have a performance issue because > HashMap::removeAll() can shrink repeatedly. > We should optimize this if this is significant. I guess the HashMap::removeAll() won't cause shrink because shrink is disabled during weak processing.
Message was sent while issue was closed.
On 2014/10/07 05:16:56, haraken wrote: > > ThreadState::invokePreFinalizers() might have a performance issue because > > HashMap::removeAll() can shrink repeatedly. > > We should optimize this if this is significant. > > I guess the HashMap::removeAll() won't cause shrink because shrink is disabled > during weak processing. HashMap knows nothing about weak processing. More concretely, Allocator::isAllocationAllowed() in HashTable::shouldShrink() always returns true for off-heap HashMap.
Message was sent while issue was closed.
On 2014/10/07 05:21:00, tkent wrote: > On 2014/10/07 05:16:56, haraken wrote: > > > ThreadState::invokePreFinalizers() might have a performance issue because > > > HashMap::removeAll() can shrink repeatedly. > > > We should optimize this if this is significant. > > > > I guess the HashMap::removeAll() won't cause shrink because shrink is disabled > > during weak processing. > > HashMap knows nothing about weak processing. More concretely, > Allocator::isAllocationAllowed() in HashTable::shouldShrink() always returns > true for off-heap HashMap. oh, you're right. This is an off-heap HashMap, not an on-heap HeapHashMap. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
