Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
190183003:
Oilpan: move ImageBitmap to the oilpan heap. (Closed)
|
Created:
6 years, 9 months ago by sof Modified:
6 years, 9 months ago CC:
blink-reviews, arv+blink, Inactive, watchdog-blink-watchlist_google.com Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Visibility:
Public. |
DescriptionOilpan: move ImageBitmap to the oilpan heap.
R=haraken@chromium.org,erik.corry@gmail.com
BUG=340522
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=169153
Patch Set 1 #Patch Set 2 : Timely removal of image resources from test memory cache #Patch Set 3 : Have ImageLoader keep a persistent set of ImageLoaderClients #
Total comments: 19
Patch Set 4 : Streamlining #Patch Set 5 : Make ImageLoader hold weak references to its clients #Patch Set 6 : Rebase #Patch Set 7 : Switch to using RefPtrWillBeMember<> for stack allocated class. #Patch Set 8 : Control image resource lifetimes in tests using explicit GCs #
Total comments: 2
Messages
Total messages: 36 (0 generated)
Please take a look.
ImageBitmap inherits from ImageLoaderClient, and there is a weak pointer to the ImageLoaderClient, which is cleared on destruction. Now see ImageLoader::sourceImageChanged. It iterates over the ImageLoaderClients, and presumably there is some good reason why things don't disappear from the collection when we are iterating over it. But in the new situation things can disappear if there is a GC and the finalizers get called on the ImageBitmap. There's a call to notifyImageSourceChanged() in the loop, which looks like it might cause a GC.
On 2014/03/10 10:20:24, Erik Corry wrote: > ImageBitmap inherits from ImageLoaderClient, and there is a weak pointer to the > ImageLoaderClient, which is cleared on destruction. > > Now see ImageLoader::sourceImageChanged. It iterates over the > ImageLoaderClients, and presumably there is some good reason why things don't > disappear from the collection when we are iterating over it. But in the new > situation things can disappear if there is a GC and the finalizers get called on > the ImageBitmap. There's a call to notifyImageSourceChanged() in the loop, > which looks like it might cause a GC. Hmm, that might leave invalid references in the HashSet. I'll try switching ImageLoader to use WillBePersistentHeapHashSet.
On 2014/03/10 10:49:55, sof wrote: > On 2014/03/10 10:20:24, Erik Corry wrote: > > ImageBitmap inherits from ImageLoaderClient, and there is a weak pointer to > the > > ImageLoaderClient, which is cleared on destruction. > > > > Now see ImageLoader::sourceImageChanged. It iterates over the > > ImageLoaderClients, and presumably there is some good reason why things don't > > disappear from the collection when we are iterating over it. But in the new > > situation things can disappear if there is a GC and the finalizers get called > on > > the ImageBitmap. There's a call to notifyImageSourceChanged() in the loop, > > which looks like it might cause a GC. > > Hmm, that might leave invalid references in the HashSet. I'll try switching > ImageLoader to use WillBePersistentHeapHashSet. Did that + made ImageLoaderClient a GC mixin.
https://codereview.chromium.org/190183003/diff/40001/Source/core/frame/ImageB... File Source/core/frame/ImageBitmap.h (right): https://codereview.chromium.org/190183003/diff/40001/Source/core/frame/ImageB... Source/core/frame/ImageBitmap.h:44: void detach(); detach => dispose. attach/detach is normally used for attaching/detaching renderers. https://codereview.chromium.org/190183003/diff/40001/Source/core/frame/ImageB... Source/core/frame/ImageBitmap.h:46: virtual void trace(Visitor*); Add OVERRIDE. https://codereview.chromium.org/190183003/diff/40001/Source/core/frame/ImageB... File Source/core/frame/ImageBitmapTest.cpp (right): https://codereview.chromium.org/190183003/diff/40001/Source/core/frame/ImageB... Source/core/frame/ImageBitmapTest.cpp:97: class AutoImageBitmap { I'm not fully convinced why we need AutoImageBitmap. Can we simply use RefPtrWillBeRawPtr<ImageBitmap> and insert Heap::collectGarbage() to proper places to guarantee that the image resources are collected before the memory cache is torn down? https://codereview.chromium.org/190183003/diff/40001/Source/core/frame/ImageB... Source/core/frame/ImageBitmapTest.cpp:116: m_imageBitmap->detach(); This looks dangerous. AutoImageBitmap is on-heap (Here "on-heap" means that "it's managed by oilpan's GC"). ImageBitmap is also on-heap. Thus AutoImageBitmap's destructor shouldn't touch ImageBitmap. https://codereview.chromium.org/190183003/diff/40001/Source/core/frame/ImageB... Source/core/frame/ImageBitmapTest.cpp:123: RefPtrWillBeMember<ImageBitmap> m_imageBitmap; This can be RefPtrWillBeRawPtr because AutoImageBitmap is stack allocated. https://codereview.chromium.org/190183003/diff/40001/Source/core/loader/Image... File Source/core/loader/ImageLoader.h (right): https://codereview.chromium.org/190183003/diff/40001/Source/core/loader/Image... Source/core/loader/ImageLoader.h:42: #if ENABLE(OILPAN) Would it be possible to avoid adding this flag?
https://codereview.chromium.org/190183003/diff/40001/Source/core/frame/ImageB... File Source/core/frame/ImageBitmap.h (right): https://codereview.chromium.org/190183003/diff/40001/Source/core/frame/ImageB... Source/core/frame/ImageBitmap.h:46: virtual void trace(Visitor*); On 2014/03/10 15:25:03, haraken wrote: > > Add OVERRIDE. It's not overriding any other trace(). https://codereview.chromium.org/190183003/diff/40001/Source/core/frame/ImageB... File Source/core/frame/ImageBitmapTest.cpp (right): https://codereview.chromium.org/190183003/diff/40001/Source/core/frame/ImageB... Source/core/frame/ImageBitmapTest.cpp:97: class AutoImageBitmap { On 2014/03/10 15:25:03, haraken wrote: > > I'm not fully convinced why we need AutoImageBitmap. > > Can we simply use RefPtrWillBeRawPtr<ImageBitmap> and insert > Heap::collectGarbage() to proper places to guarantee that the image resources > are collected before the memory cache is torn down? I considered triggering a GC for each test too blunt an instrument. https://codereview.chromium.org/190183003/diff/40001/Source/core/frame/ImageB... Source/core/frame/ImageBitmapTest.cpp:116: m_imageBitmap->detach(); On 2014/03/10 15:25:03, haraken wrote: > > This looks dangerous. AutoImageBitmap is on-heap (Here "on-heap" means that > "it's managed by oilpan's GC"). ImageBitmap is also on-heap. Thus > AutoImageBitmap's destructor shouldn't touch ImageBitmap. It's on the stack and not finalized; what is the concern?
https://codereview.chromium.org/190183003/diff/40001/Source/core/frame/ImageB... File Source/core/frame/ImageBitmapTest.cpp (right): https://codereview.chromium.org/190183003/diff/40001/Source/core/frame/ImageB... Source/core/frame/ImageBitmapTest.cpp:97: class AutoImageBitmap { On 2014/03/10 15:55:57, sof wrote: > On 2014/03/10 15:25:03, haraken wrote: > > > > I'm not fully convinced why we need AutoImageBitmap. > > > > Can we simply use RefPtrWillBeRawPtr<ImageBitmap> and insert > > Heap::collectGarbage() to proper places to guarantee that the image resources > > are collected before the memory cache is torn down? > > I considered triggering a GC for each test too blunt an instrument. We were doing that in the experimental branch. Mads: What do you think? https://codereview.chromium.org/190183003/diff/40001/Source/core/frame/ImageB... Source/core/frame/ImageBitmapTest.cpp:116: m_imageBitmap->detach(); On 2014/03/10 15:55:57, sof wrote: > On 2014/03/10 15:25:03, haraken wrote: > > > > This looks dangerous. AutoImageBitmap is on-heap (Here "on-heap" means that > > "it's managed by oilpan's GC"). ImageBitmap is also on-heap. Thus > > AutoImageBitmap's destructor shouldn't touch ImageBitmap. > > It's on the stack and not finalized; what is the concern? Sorry, you're right.
https://codereview.chromium.org/190183003/diff/40001/Source/core/frame/ImageB... File Source/core/frame/ImageBitmap.h (right): https://codereview.chromium.org/190183003/diff/40001/Source/core/frame/ImageB... Source/core/frame/ImageBitmap.h:44: void detach(); On 2014/03/10 15:25:03, haraken wrote: > > detach => dispose. > > attach/detach is normally used for attaching/detaching renderers. Done. https://codereview.chromium.org/190183003/diff/40001/Source/core/frame/ImageB... File Source/core/frame/ImageBitmapTest.cpp (right): https://codereview.chromium.org/190183003/diff/40001/Source/core/frame/ImageB... Source/core/frame/ImageBitmapTest.cpp:97: class AutoImageBitmap { On 2014/03/10 16:04:55, haraken wrote: > On 2014/03/10 15:55:57, sof wrote: > > On 2014/03/10 15:25:03, haraken wrote: > > > > > > I'm not fully convinced why we need AutoImageBitmap. > > > > > > Can we simply use RefPtrWillBeRawPtr<ImageBitmap> and insert > > > Heap::collectGarbage() to proper places to guarantee that the image > resources > > > are collected before the memory cache is torn down? > > > > I considered triggering a GC for each test too blunt an instrument. > > We were doing that in the experimental branch. > > Mads: What do you think? I see; if preferable, I'll switch over of course. https://codereview.chromium.org/190183003/diff/40001/Source/core/frame/ImageB... Source/core/frame/ImageBitmapTest.cpp:123: RefPtrWillBeMember<ImageBitmap> m_imageBitmap; On 2014/03/10 15:25:03, haraken wrote: > > This can be RefPtrWillBeRawPtr because AutoImageBitmap is stack allocated. I've switched to that representation, but am curious why Member is being avoided. https://codereview.chromium.org/190183003/diff/40001/Source/core/loader/Image... File Source/core/loader/ImageLoader.h (right): https://codereview.chromium.org/190183003/diff/40001/Source/core/loader/Image... Source/core/loader/ImageLoader.h:42: #if ENABLE(OILPAN) On 2014/03/10 15:25:03, haraken wrote: > > Would it be possible to avoid adding this flag? Certainly; removed.
I think this change now makes the ImageLoader have a strong reference to the ImageBitmaps, which I think will be a bad thing. Probably you want to use WeakMember instead of Member in the collection. One of the benefits of the move from CollectionPersistent<HeapHashMap<...> > to PersistentHeapHashMap<...> is that now weakness should work. The semantics are that it turns strong while the iterator is alive and is weak the rest of the time, which is what I think you want. https://codereview.chromium.org/190183003/diff/40001/Source/core/frame/ImageB... File Source/core/frame/ImageBitmapTest.cpp (right): https://codereview.chromium.org/190183003/diff/40001/Source/core/frame/ImageB... Source/core/frame/ImageBitmapTest.cpp:123: RefPtrWillBeMember<ImageBitmap> m_imageBitmap; On 2014/03/10 21:07:21, sof wrote: > On 2014/03/10 15:25:03, haraken wrote: > > > > This can be RefPtrWillBeRawPtr because AutoImageBitmap is stack allocated. > > I've switched to that representation, but am curious why Member is being > avoided. I'm not sure, actually. For bare stack allocated variables I think it's fine to use RawPtr. It turns out that even trivial 'smart' pointers are code generated worse, especially on 32 bit targets. This is to do with the old ABIs where structs/classes are not passed as efficiently as raw pointers. Therefore, and because it is less verbose, we are going to end up with just "ImageBitmap*". RawPtr is what we have now so that it ends up with just the * type. For something inside an on-stack struct the first issue does not apply, and I think there is a nice documenting effect of using Member and I'd be inclined to keep it that way.
On 2014/03/10 21:18:56, Erik Corry wrote: > I think this change now makes the ImageLoader have a strong reference to the > ImageBitmaps, which I think will be a bad thing. Probably you want to use > WeakMember instead of Member in the collection. One of the benefits of the move > from CollectionPersistent<HeapHashMap<...> > to PersistentHeapHashMap<...> is > that now weakness should work. The semantics are that it turns strong while the > iterator is alive and is weak the rest of the time, which is what I think you > want. > > https://codereview.chromium.org/190183003/diff/40001/Source/core/frame/ImageB... > File Source/core/frame/ImageBitmapTest.cpp (right): > > https://codereview.chromium.org/190183003/diff/40001/Source/core/frame/ImageB... > Source/core/frame/ImageBitmapTest.cpp:123: RefPtrWillBeMember<ImageBitmap> > m_imageBitmap; > On 2014/03/10 21:07:21, sof wrote: > > On 2014/03/10 15:25:03, haraken wrote: > > > > > > This can be RefPtrWillBeRawPtr because AutoImageBitmap is stack allocated. > > > > I've switched to that representation, but am curious why Member is being > > avoided. > > I'm not sure, actually. > > For bare stack allocated variables I think it's fine to use RawPtr. It turns > out that even trivial 'smart' pointers are code generated worse, especially on > 32 bit targets. This is to do with the old ABIs where structs/classes are not > passed as efficiently as raw pointers. Therefore, and because it is less > verbose, we are going to end up with just "ImageBitmap*". RawPtr is what we have > now so that it ends up with just the * type. > > For something inside an on-stack struct the first issue does not apply, and I > think there is a nice documenting effect of using Member and I'd be inclined to > keep it that way. Another viewpoint is that all Members in objects should be visited via trace() methods (I think this is an easy-to-understand rule). From that viewpoint, a pointer in stack-allocated objects should be a RawPtr not a Member (because there is no need to add a trace() method). My point is just that we should be clear about the programming rule. As far as the rule is clear, either rule is fine :)
On 2014/03/10 21:18:56, Erik Corry wrote: > I think this change now makes the ImageLoader have a strong reference to the > ImageBitmaps, which I think will be a bad thing. Probably you want to use > WeakMember instead of Member in the collection. One of the benefits of the move > from CollectionPersistent<HeapHashMap<...> > to PersistentHeapHashMap<...> is > that now weakness should work. The semantics are that it turns strong while the > iterator is alive and is weak the rest of the time, which is what I think you > want. > Yes, the loader shouldn't keep an otherwise unused ImageLoaderClient (ImageBitmap) around. Thanks for correcting that; done. (Added a typedef to make uses of the hash set type a bit more readable.)
On 2014/03/11 02:46:56, haraken wrote: > On 2014/03/10 21:18:56, Erik Corry wrote: > > I think this change now makes the ImageLoader have a strong reference to the > > ImageBitmaps, which I think will be a bad thing. Probably you want to use > > WeakMember instead of Member in the collection. One of the benefits of the > move > > from CollectionPersistent<HeapHashMap<...> > to PersistentHeapHashMap<...> is > > that now weakness should work. The semantics are that it turns strong while > the > > iterator is alive and is weak the rest of the time, which is what I think you > > want. > > > > > https://codereview.chromium.org/190183003/diff/40001/Source/core/frame/ImageB... > > File Source/core/frame/ImageBitmapTest.cpp (right): > > > > > https://codereview.chromium.org/190183003/diff/40001/Source/core/frame/ImageB... > > Source/core/frame/ImageBitmapTest.cpp:123: RefPtrWillBeMember<ImageBitmap> > > m_imageBitmap; > > On 2014/03/10 21:07:21, sof wrote: > > > On 2014/03/10 15:25:03, haraken wrote: > > > > > > > > This can be RefPtrWillBeRawPtr because AutoImageBitmap is stack allocated. > > > > > > I've switched to that representation, but am curious why Member is being > > > avoided. > > > > I'm not sure, actually. > > > > For bare stack allocated variables I think it's fine to use RawPtr. It turns > > out that even trivial 'smart' pointers are code generated worse, especially on > > 32 bit targets. This is to do with the old ABIs where structs/classes are not > > passed as efficiently as raw pointers. Therefore, and because it is less > > verbose, we are going to end up with just "ImageBitmap*". RawPtr is what we > have > > now so that it ends up with just the * type. > > > > For something inside an on-stack struct the first issue does not apply, and I > > think there is a nice documenting effect of using Member and I'd be inclined > to > > keep it that way. > > Another viewpoint is that all Members in objects should be visited via trace() > methods (I think this is an easy-to-understand rule). From that viewpoint, a > pointer in stack-allocated objects should be a RawPtr not a Member (because > there is no need to add a trace() method). > > My point is just that we should be clear about the programming rule. As far as > the rule is clear, either rule is fine :) That's a clear rule wrt Member usage, thanks. The dual being that you're only allowed to have a RawPtr<> in a stack-allocated class. ?
> > Another viewpoint is that all Members in objects should be visited via trace() > > methods (I think this is an easy-to-understand rule). From that viewpoint, a > > pointer in stack-allocated objects should be a RawPtr not a Member (because > > there is no need to add a trace() method). > > > > My point is just that we should be clear about the programming rule. As far as > > the rule is clear, either rule is fine :) > > That's a clear rule wrt Member usage, thanks. > > The dual being that you're only allowed to have a RawPtr<> in a stack-allocated > class. ? In my understanding, yes.
We discussed this and the decision was that it's OK (even preferred) to have Member<T> in structs and classes that are only stack allocated. This makes it simpler if the object is later heap allocated, and indicates that we have thought about the pointer. For lone pointers on the stack we still use raw pointers with '*' or XxxWillBeRawPtr<T> in the transition period. On Tue, Mar 11, 2014 at 8:56 AM, <haraken@chromium.org> wrote: > > Another viewpoint is that all Members in objects should be visited via >> > trace() > >> > methods (I think this is an easy-to-understand rule). From that >> viewpoint, a >> > pointer in stack-allocated objects should be a RawPtr not a Member >> (because >> > there is no need to add a trace() method). >> > >> > My point is just that we should be clear about the programming rule. As >> far >> > as > >> > the rule is clear, either rule is fine :) >> > > That's a clear rule wrt Member usage, thanks. >> > > The dual being that you're only allowed to have a RawPtr<> in a >> > stack-allocated > >> class. ? >> > > In my understanding, yes. > > > > https://codereview.chromium.org/190183003/ > > To unsubscribe from this group and stop receiving emails from it, send an > email to blink-reviews+unsubscribe@chromium.org. > -- Erik Corry, Software Engineer Google Denmark ApS - Frederiksborggade 20B, 1 sal, 1360 København K - Denmark - CVR nr. 28 86 69 84 To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
On 2014/03/11 09:14:07, Erik Corry wrote: > We discussed this and the decision was that it's OK (even preferred) to > have Member<T> in structs and classes that are only stack allocated. This > makes it simpler if the object is later heap allocated, and indicates that > we have thought about the pointer. > > For lone pointers on the stack we still use raw pointers with '*' or > XxxWillBeRawPtr<T> in the transition period. > Thanks for going over this & clarifying; updated AutoImageBitmap to follow of that rule.
https://codereview.chromium.org/190183003/diff/40001/Source/core/frame/ImageB... File Source/core/frame/ImageBitmapTest.cpp (right): https://codereview.chromium.org/190183003/diff/40001/Source/core/frame/ImageB... Source/core/frame/ImageBitmapTest.cpp:97: class AutoImageBitmap { On 2014/03/10 21:07:21, sof wrote: > On 2014/03/10 16:04:55, haraken wrote: > > On 2014/03/10 15:55:57, sof wrote: > > > On 2014/03/10 15:25:03, haraken wrote: > > > > > > > > I'm not fully convinced why we need AutoImageBitmap. > > > > > > > > Can we simply use RefPtrWillBeRawPtr<ImageBitmap> and insert > > > > Heap::collectGarbage() to proper places to guarantee that the image > > resources > > > > are collected before the memory cache is torn down? > > > > > > I considered triggering a GC for each test too blunt an instrument. > > > > We were doing that in the experimental branch. > > > > Mads: What do you think? > > I see; if preferable, I'll switch over of course. Need to make a choice here between: - The test TearDown()s perform a garbage collection. - A test explicitly uses AutoImageBitmap to release any ImageBitmaps & have them be evicted from the test's memory cache in a timely manner.
https://codereview.chromium.org/190183003/diff/40001/Source/core/frame/ImageB... File Source/core/frame/ImageBitmapTest.cpp (right): https://codereview.chromium.org/190183003/diff/40001/Source/core/frame/ImageB... Source/core/frame/ImageBitmapTest.cpp:97: class AutoImageBitmap { On 2014/03/12 10:02:25, sof wrote: > On 2014/03/10 21:07:21, sof wrote: > > On 2014/03/10 16:04:55, haraken wrote: > > > On 2014/03/10 15:55:57, sof wrote: > > > > On 2014/03/10 15:25:03, haraken wrote: > > > > > > > > > > I'm not fully convinced why we need AutoImageBitmap. > > > > > > > > > > Can we simply use RefPtrWillBeRawPtr<ImageBitmap> and insert > > > > > Heap::collectGarbage() to proper places to guarantee that the image > > > resources > > > > > are collected before the memory cache is torn down? > > > > > > > > I considered triggering a GC for each test too blunt an instrument. > > > > > > We were doing that in the experimental branch. > > > > > > Mads: What do you think? > > > > I see; if preferable, I'll switch over of course. > > Need to make a choice here between: > > - The test TearDown()s perform a garbage collection. > - A test explicitly uses AutoImageBitmap to release any ImageBitmaps & have > them be evicted from the test's memory cache in a timely manner. My feeling is that an explicit call to GC in the tear-down is closer to the code path in the finished product, so that's the way we want to test it.
https://codereview.chromium.org/190183003/diff/40001/Source/core/frame/ImageB... File Source/core/frame/ImageBitmapTest.cpp (right): https://codereview.chromium.org/190183003/diff/40001/Source/core/frame/ImageB... Source/core/frame/ImageBitmapTest.cpp:97: class AutoImageBitmap { On 2014/03/12 10:06:15, Erik Corry wrote: > On 2014/03/12 10:02:25, sof wrote: > > On 2014/03/10 21:07:21, sof wrote: > > > On 2014/03/10 16:04:55, haraken wrote: > > > > On 2014/03/10 15:55:57, sof wrote: > > > > > On 2014/03/10 15:25:03, haraken wrote: > > > > > > > > > > > > I'm not fully convinced why we need AutoImageBitmap. > > > > > > > > > > > > Can we simply use RefPtrWillBeRawPtr<ImageBitmap> and insert > > > > > > Heap::collectGarbage() to proper places to guarantee that the image > > > > resources > > > > > > are collected before the memory cache is torn down? > > > > > > > > > > I considered triggering a GC for each test too blunt an instrument. > > > > > > > > We were doing that in the experimental branch. > > > > > > > > Mads: What do you think? > > > > > > I see; if preferable, I'll switch over of course. > > > > Need to make a choice here between: > > > > - The test TearDown()s perform a garbage collection. > > - A test explicitly uses AutoImageBitmap to release any ImageBitmaps & have > > them be evicted from the test's memory cache in a timely manner. > > My feeling is that an explicit call to GC in the tear-down is closer to the code > path in the finished product, so that's the way we want to test it. Agreed.
On 2014/03/12 10:34:08, haraken wrote: > https://codereview.chromium.org/190183003/diff/40001/Source/core/frame/ImageB... > File Source/core/frame/ImageBitmapTest.cpp (right): > > https://codereview.chromium.org/190183003/diff/40001/Source/core/frame/ImageB... > Source/core/frame/ImageBitmapTest.cpp:97: class AutoImageBitmap { > On 2014/03/12 10:06:15, Erik Corry wrote: > > On 2014/03/12 10:02:25, sof wrote: > > > On 2014/03/10 21:07:21, sof wrote: > > > > On 2014/03/10 16:04:55, haraken wrote: > > > > > On 2014/03/10 15:55:57, sof wrote: > > > > > > On 2014/03/10 15:25:03, haraken wrote: > > > > > > > > > > > > > > I'm not fully convinced why we need AutoImageBitmap. > > > > > > > > > > > > > > Can we simply use RefPtrWillBeRawPtr<ImageBitmap> and insert > > > > > > > Heap::collectGarbage() to proper places to guarantee that the image > > > > > resources > > > > > > > are collected before the memory cache is torn down? > > > > > > > > > > > > I considered triggering a GC for each test too blunt an instrument. > > > > > > > > > > We were doing that in the experimental branch. > > > > > > > > > > Mads: What do you think? > > > > > > > > I see; if preferable, I'll switch over of course. > > > > > > Need to make a choice here between: > > > > > > - The test TearDown()s perform a garbage collection. > > > - A test explicitly uses AutoImageBitmap to release any ImageBitmaps & have > > > them be evicted from the test's memory cache in a timely manner. > > > > My feeling is that an explicit call to GC in the tear-down is closer to the > code > > path in the finished product, so that's the way we want to test it. > > Agreed. Hmm, if I have WillBePersistentHeapHashSet<RefPtrWillBeWeakMember<ImageBitmap> > m_clients; in ImageBitmap, which isn't GCed, then that persistent "heap hash set" isn't (GC) heap allocated. With the outcome that when the wanted collectGarbage() runs here, the registration of the associated weak-pointer callback fails for the heap hash set, as the object isn't in a known heap. Am I reading this completely wrong and/or has this been addressed somewhere else already?
The HeapHashSet is not heap allocated, but as soon as it is not empty it has a backing store, which _is_ heap allocated. The 'persistent' part makes sure that the backing is subject to weak processing, so you don't need to manually register a weak callback normally. If there's an iterator pointing at the backing store from the stack, then the backing turns strong and no weak processing takes place. That's how it's supposed to work, does that suit your purposes? On Wed, Mar 12, 2014 at 3:14 PM, <sigbjornf@opera.com> wrote: > On 2014/03/12 10:34:08, haraken wrote: > > https://codereview.chromium.org/190183003/diff/40001/Source/core/frame/ > ImageBitmapTest.cpp > >> File Source/core/frame/ImageBitmapTest.cpp (right): >> > > > https://codereview.chromium.org/190183003/diff/40001/Source/core/frame/ > ImageBitmapTest.cpp#newcode97 > >> Source/core/frame/ImageBitmapTest.cpp:97: class AutoImageBitmap { >> On 2014/03/12 10:06:15, Erik Corry wrote: >> > On 2014/03/12 10:02:25, sof wrote: >> > > On 2014/03/10 21:07:21, sof wrote: >> > > > On 2014/03/10 16:04:55, haraken wrote: >> > > > > On 2014/03/10 15:55:57, sof wrote: >> > > > > > On 2014/03/10 15:25:03, haraken wrote: >> > > > > > > >> > > > > > > I'm not fully convinced why we need AutoImageBitmap. >> > > > > > > >> > > > > > > Can we simply use RefPtrWillBeRawPtr<ImageBitmap> and insert >> > > > > > > Heap::collectGarbage() to proper places to guarantee that the >> > image > >> > > > > resources >> > > > > > > are collected before the memory cache is torn down? >> > > > > > >> > > > > > I considered triggering a GC for each test too blunt an >> instrument. >> > > > > >> > > > > We were doing that in the experimental branch. >> > > > > >> > > > > Mads: What do you think? >> > > > >> > > > I see; if preferable, I'll switch over of course. >> > > >> > > Need to make a choice here between: >> > > >> > > - The test TearDown()s perform a garbage collection. >> > > - A test explicitly uses AutoImageBitmap to release any ImageBitmaps >> & >> > have > >> > > them be evicted from the test's memory cache in a timely manner. >> > >> > My feeling is that an explicit call to GC in the tear-down is closer to >> the >> code >> > path in the finished product, so that's the way we want to test it. >> > > Agreed. >> > > Hmm, if I have > > WillBePersistentHeapHashSet<RefPtrWillBeWeakMember<ImageBitmap> > > m_clients; > > in ImageBitmap, which isn't GCed, then that persistent "heap hash set" > isn't > (GC) heap allocated. With the outcome that when the wanted collectGarbage() > runs here, the registration of the associated weak-pointer callback fails > for the heap hash set, as the object isn't in a known heap. > > Am I reading this completely wrong and/or has this been addressed somewhere > else already? > > https://codereview.chromium.org/190183003/ > -- Erik Corry, Software Engineer Google Denmark ApS - Frederiksborggade 20B, 1 sal, 1360 København K - Denmark - CVR nr. 28 86 69 84 To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
On 2014/03/12 14:29:07, Erik Corry wrote: > The HeapHashSet is not heap allocated, but as soon as it is not empty it > has a backing store, which _is_ heap allocated. The 'persistent' part > makes sure that the backing is subject to weak processing, so you don't > need to manually register a weak callback normally. If there's an iterator > pointing at the backing store from the stack, then the backing turns strong > and no weak processing takes place. > > That's how it's supposed to work, does that suit your purposes? > Thanks for the outline, good stuff. The underlying table is heap allocated, but not the m_impl of this HashSet. Which is what HashTable<..>:trace() tries to register its weak members over (via the trace() for HashSet.)
OK that sounds like a wrong assert. It should be OK to register the hash table as the address even though it is off-heap. On Wed, Mar 12, 2014 at 3:54 PM, <sigbjornf@opera.com> wrote: > On 2014/03/12 14:29:07, Erik Corry wrote: > >> The HeapHashSet is not heap allocated, but as soon as it is not empty it >> has a backing store, which _is_ heap allocated. The 'persistent' part >> makes sure that the backing is subject to weak processing, so you don't >> need to manually register a weak callback normally. If there's an >> iterator >> pointing at the backing store from the stack, then the backing turns >> strong >> and no weak processing takes place. >> > > That's how it's supposed to work, does that suit your purposes? >> > > > Thanks for the outline, good stuff. > > The underlying table is heap allocated, but not the m_impl of this HashSet. > Which is what HashTable<..>:trace() tries to register its weak members over > (via the trace() for HashSet.) > > > > https://codereview.chromium.org/190183003/ > > To unsubscribe from this group and stop receiving emails from it, send an > email to blink-reviews+unsubscribe@chromium.org. > -- Erik Corry, Software Engineer Google Denmark ApS - Frederiksborggade 20B, 1 sal, 1360 København K - Denmark - CVR nr. 28 86 69 84 To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
Ah, I see it's not just an assert: We need to change the API a little so we can handle this case. On Wed, Mar 12, 2014 at 3:59 PM, Erik Corry <erik.corry@gmail.com> wrote: > OK that sounds like a wrong assert. It should be OK to register the hash > table as the address even though it is off-heap. > > > On Wed, Mar 12, 2014 at 3:54 PM, <sigbjornf@opera.com> wrote: > >> On 2014/03/12 14:29:07, Erik Corry wrote: >> >>> The HeapHashSet is not heap allocated, but as soon as it is not empty it >>> has a backing store, which _is_ heap allocated. The 'persistent' part >>> makes sure that the backing is subject to weak processing, so you don't >>> need to manually register a weak callback normally. If there's an >>> iterator >>> pointing at the backing store from the stack, then the backing turns >>> strong >>> and no weak processing takes place. >>> >> >> That's how it's supposed to work, does that suit your purposes? >>> >> >> >> Thanks for the outline, good stuff. >> >> The underlying table is heap allocated, but not the m_impl of this >> HashSet. >> Which is what HashTable<..>:trace() tries to register its weak members >> over >> (via the trace() for HashSet.) >> >> >> >> https://codereview.chromium.org/190183003/ >> >> To unsubscribe from this group and stop receiving emails from it, send an >> email to blink-reviews+unsubscribe@chromium.org. >> > > > > -- > Erik Corry, Software Engineer > Google Denmark ApS - Frederiksborggade 20B, 1 sal, > 1360 København K - Denmark - CVR nr. 28 86 69 84 > -- Erik Corry, Software Engineer Google Denmark ApS - Frederiksborggade 20B, 1 sal, 1360 København K - Denmark - CVR nr. 28 86 69 84 To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
On 2014/03/12 15:05:55, Erik Corry wrote: > Ah, I see it's not just an assert: We need to change the API a little so we > can handle this case. > To test, I hackily pushed/registered the weak callback with the main thread if faced with an off-heap object. (The use of collectGarbage() isn't ideal for ImageBitmapTest -- it needs to be manually&additionally run in a test to ensure timely finalization.)
This should help: https://codereview.chromium.org/197343003
On 2014/03/12 15:34:00, Erik Corry wrote: > This should help: https://codereview.chromium.org/197343003 Confirmedly does; awesome, thanks :-)
On 2014/03/12 15:13:45, sof wrote: ... > > (The use of collectGarbage() isn't ideal for ImageBitmapTest -- it needs > to be manually&additionally run in a test to ensure timely finalization.) Uploaded a version that uses collectGarbage() (depends on the above change to PersistentHeapSet<WeakMember<T> > )
LGTM with a comment. https://codereview.chromium.org/190183003/diff/120001/Source/core/loader/Imag... File Source/core/loader/ImageLoader.h (right): https://codereview.chromium.org/190183003/diff/120001/Source/core/loader/Imag... Source/core/loader/ImageLoader.h:112: typedef WillBePersistentHeapHashSet<RawPtrWillBeWeakMember<ImageLoaderClient> > ImageLoaderClientSet; Can't you use WillBeHeapHashSet<RawPtrWillBeWeakMember<ImageLoaderClient>> and trace it? Given that ImageLoaderClient is a GarbageCollectedMixin, you can have on-heap objects in ImageLoaderClient.
https://codereview.chromium.org/190183003/diff/120001/Source/core/loader/Imag... File Source/core/loader/ImageLoader.h (right): https://codereview.chromium.org/190183003/diff/120001/Source/core/loader/Imag... Source/core/loader/ImageLoader.h:112: typedef WillBePersistentHeapHashSet<RawPtrWillBeWeakMember<ImageLoaderClient> > ImageLoaderClientSet; On 2014/03/13 01:22:28, haraken wrote: > > Can't you use WillBeHeapHashSet<RawPtrWillBeWeakMember<ImageLoaderClient>> and > trace it? Given that ImageLoaderClient is a GarbageCollectedMixin, you can have > on-heap objects in ImageLoaderClient. At some point, hopefully. This is in ImageLoader (the diff is misleading), which isn't a garbage collected object.
https://codereview.chromium.org/197343003/ now landed; sending this one along.
The CQ bit was checked by sigbjornf@opera.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sigbjornf@opera.com/190183003/120001
The CQ bit was unchecked by sigbjornf@opera.com
The CQ bit was checked by sigbjornf@opera.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sigbjornf@opera.com/190183003/120001
Message was sent while issue was closed.
Change committed as 169153 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
