Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1224323003:
Oilpan: simplify ImageLoaderClient handling. (Closed)
DescriptionOilpan: simplify ImageLoaderClient handling.
Simplify the unregistration of ImageLoaderClient objects. The weak client
references are removed as clients as part of weak processing or
when running the ImageLoader's prefinalizer.
R=haraken
BUG=383742
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=198790
Patch Set 1 #Patch Set 2 : add unregistration of weakly held clients #
Total comments: 2
Messages
Total messages: 17 (3 generated)
sigbjornf@opera.com changed reviewers: + oilpan-reviews@chromium.org, tkent@chromium.org
please take a look. I don't see a legitimate use for this any longer, but Resource handling does throw surprises at times :) i.e., checking with the bots.
On 2015/07/13 13:29:38, sof wrote: > please take a look. > > I don't see a legitimate use for this any longer, but Resource handling does > throw surprises at times :) i.e., checking with the bots. And that it did. Adjusted somewhat, adding weak reference handling. Updated description accordingly.
haraken@chromium.org changed reviewers: + haraken@chromium.org
https://codereview.chromium.org/1224323003/diff/20001/Source/core/loader/Imag... File Source/core/loader/ImageLoader.h (right): https://codereview.chromium.org/1224323003/diff/20001/Source/core/loader/Imag... Source/core/loader/ImageLoader.h:173: HashSet<ImageLoaderClient*> m_clients; I'm wondering if we could just use HeapHashSet<WeakMember<ImageLoaderClient>> but doesn't it work for some reason?
https://codereview.chromium.org/1224323003/diff/20001/Source/core/loader/Imag... File Source/core/loader/ImageLoader.h (right): https://codereview.chromium.org/1224323003/diff/20001/Source/core/loader/Imag... Source/core/loader/ImageLoader.h:173: HashSet<ImageLoaderClient*> m_clients; On 2015/07/13 14:46:56, haraken wrote: > > I'm wondering if we could just use HeapHashSet<WeakMember<ImageLoaderClient>> > but doesn't it work for some reason? > We need to explicitly unregister (==willRemoveClient()) upon them falling away, so a custom weak callback is needed. This representation mirrors how LocalFrame::m_pluginElements are kept.
On 2015/07/13 14:49:44, sof wrote: > https://codereview.chromium.org/1224323003/diff/20001/Source/core/loader/Imag... > File Source/core/loader/ImageLoader.h (right): > > https://codereview.chromium.org/1224323003/diff/20001/Source/core/loader/Imag... > Source/core/loader/ImageLoader.h:173: HashSet<ImageLoaderClient*> m_clients; > On 2015/07/13 14:46:56, haraken wrote: > > > > I'm wondering if we could just use HeapHashSet<WeakMember<ImageLoaderClient>> > > but doesn't it work for some reason? > > > > We need to explicitly unregister (==willRemoveClient()) upon them falling away, > so a custom weak callback is needed. > > This representation mirrors how LocalFrame::m_pluginElements are kept. ah, makes sense. Would it be an option that we add a pre-finalizer to ImageLoaderClient and call willRemoveClient(*client) in the pre-finalizer?
On 2015/07/13 14:51:55, haraken wrote: > On 2015/07/13 14:49:44, sof wrote: > > > https://codereview.chromium.org/1224323003/diff/20001/Source/core/loader/Imag... > > File Source/core/loader/ImageLoader.h (right): > > > > > https://codereview.chromium.org/1224323003/diff/20001/Source/core/loader/Imag... > > Source/core/loader/ImageLoader.h:173: HashSet<ImageLoaderClient*> m_clients; > > On 2015/07/13 14:46:56, haraken wrote: > > > > > > I'm wondering if we could just use > HeapHashSet<WeakMember<ImageLoaderClient>> > > > but doesn't it work for some reason? > > > > > > > We need to explicitly unregister (==willRemoveClient()) upon them falling > away, > > so a custom weak callback is needed. > > > > This representation mirrors how LocalFrame::m_pluginElements are kept. > > ah, makes sense. Would it be an option that we add a pre-finalizer to > ImageLoaderClient and call willRemoveClient(*client) in the pre-finalizer? hmm, I think that's pushing the use of prefinalizers too far.
On 2015/07/13 14:54:56, sof wrote: > On 2015/07/13 14:51:55, haraken wrote: > > On 2015/07/13 14:49:44, sof wrote: > > > > > > https://codereview.chromium.org/1224323003/diff/20001/Source/core/loader/Imag... > > > File Source/core/loader/ImageLoader.h (right): > > > > > > > > > https://codereview.chromium.org/1224323003/diff/20001/Source/core/loader/Imag... > > > Source/core/loader/ImageLoader.h:173: HashSet<ImageLoaderClient*> m_clients; > > > On 2015/07/13 14:46:56, haraken wrote: > > > > > > > > I'm wondering if we could just use > > HeapHashSet<WeakMember<ImageLoaderClient>> > > > > but doesn't it work for some reason? > > > > > > > > > > We need to explicitly unregister (==willRemoveClient()) upon them falling > > away, > > > so a custom weak callback is needed. > > > > > > This representation mirrors how LocalFrame::m_pluginElements are kept. > > > > ah, makes sense. Would it be an option that we add a pre-finalizer to > > ImageLoaderClient and call willRemoveClient(*client) in the pre-finalizer? > > hmm, I think that's pushing the use of prefinalizers too far. Conceptually I think it should be a pre-finalizer, but if that's not nice from the performance perspective, I'm fine with the current CL. BTW, in the current CL, willRemoveClient won't be called if the ImageLoader and its clients die together. Will it be OK?
On 2015/07/13 15:15:19, haraken wrote: > On 2015/07/13 14:54:56, sof wrote: > > On 2015/07/13 14:51:55, haraken wrote: > > > On 2015/07/13 14:49:44, sof wrote: > > > > > > > > > > https://codereview.chromium.org/1224323003/diff/20001/Source/core/loader/Imag... > > > > File Source/core/loader/ImageLoader.h (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1224323003/diff/20001/Source/core/loader/Imag... > > > > Source/core/loader/ImageLoader.h:173: HashSet<ImageLoaderClient*> > m_clients; > > > > On 2015/07/13 14:46:56, haraken wrote: > > > > > > > > > > I'm wondering if we could just use > > > HeapHashSet<WeakMember<ImageLoaderClient>> > > > > > but doesn't it work for some reason? > > > > > > > > > > > > > We need to explicitly unregister (==willRemoveClient()) upon them falling > > > away, > > > > so a custom weak callback is needed. > > > > > > > > This representation mirrors how LocalFrame::m_pluginElements are kept. > > > > > > ah, makes sense. Would it be an option that we add a pre-finalizer to > > > ImageLoaderClient and call willRemoveClient(*client) in the pre-finalizer? > > > > hmm, I think that's pushing the use of prefinalizers too far. > > Conceptually I think it should be a pre-finalizer, but if that's not nice from > the performance perspective, I'm fine with the current CL. > > BTW, in the current CL, willRemoveClient won't be called if the ImageLoader and > its clients die together. Will it be OK? Isn't that done in dispose()?
On 2015/07/13 15:15:19, haraken wrote: > On 2015/07/13 14:54:56, sof wrote: > > On 2015/07/13 14:51:55, haraken wrote: > > > On 2015/07/13 14:49:44, sof wrote: > > > > > > > > > > https://codereview.chromium.org/1224323003/diff/20001/Source/core/loader/Imag... > > > > File Source/core/loader/ImageLoader.h (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1224323003/diff/20001/Source/core/loader/Imag... > > > > Source/core/loader/ImageLoader.h:173: HashSet<ImageLoaderClient*> > m_clients; > > > > On 2015/07/13 14:46:56, haraken wrote: > > > > > > > > > > I'm wondering if we could just use > > > HeapHashSet<WeakMember<ImageLoaderClient>> > > > > > but doesn't it work for some reason? > > > > > > > > > > > > > We need to explicitly unregister (==willRemoveClient()) upon them falling > > > away, > > > > so a custom weak callback is needed. > > > > > > > > This representation mirrors how LocalFrame::m_pluginElements are kept. > > > > > > ah, makes sense. Would it be an option that we add a pre-finalizer to > > > ImageLoaderClient and call willRemoveClient(*client) in the pre-finalizer? > > > > hmm, I think that's pushing the use of prefinalizers too far. > > Conceptually I think it should be a pre-finalizer, but if that's not nice from > the performance perspective, I'm fine with the current CL. > I don't want to spend per-GC cycles iterating a hash set containing mostly live objects, so that's one aspect of it (along with other overhead of the mechanism.) Prefinalizers (and eager finalization) should be used sparingly, if at all. The underlying assumption that all ImageLoaderClients are created after the ImageLoader they attach themselves to, is another reason why I think this is not appropriate.
On 2015/07/13 15:16:30, sof wrote: > On 2015/07/13 15:15:19, haraken wrote: > > On 2015/07/13 14:54:56, sof wrote: > > > On 2015/07/13 14:51:55, haraken wrote: > > > > On 2015/07/13 14:49:44, sof wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/1224323003/diff/20001/Source/core/loader/Imag... > > > > > File Source/core/loader/ImageLoader.h (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1224323003/diff/20001/Source/core/loader/Imag... > > > > > Source/core/loader/ImageLoader.h:173: HashSet<ImageLoaderClient*> > > m_clients; > > > > > On 2015/07/13 14:46:56, haraken wrote: > > > > > > > > > > > > I'm wondering if we could just use > > > > HeapHashSet<WeakMember<ImageLoaderClient>> > > > > > > but doesn't it work for some reason? > > > > > > > > > > > > > > > > We need to explicitly unregister (==willRemoveClient()) upon them > falling > > > > away, > > > > > so a custom weak callback is needed. > > > > > > > > > > This representation mirrors how LocalFrame::m_pluginElements are kept. > > > > > > > > ah, makes sense. Would it be an option that we add a pre-finalizer to > > > > ImageLoaderClient and call willRemoveClient(*client) in the pre-finalizer? > > > > > > hmm, I think that's pushing the use of prefinalizers too far. > > > > Conceptually I think it should be a pre-finalizer, but if that's not nice from > > the performance perspective, I'm fine with the current CL. > > > > BTW, in the current CL, willRemoveClient won't be called if the ImageLoader > and > > its clients die together. Will it be OK? > > Isn't that done in dispose()? Thanks, I'm getting to understand; hmm, the situation looks complicated... What I'm concerned about is the following scenario: 1) A GC runs. The ImageLoader survives but a client dies. 2) The next GC hits before the thread handles the thread-local weak processing (i.e., before calling the clearWeakMember). The ImageLoader dies in the GC. 3) The ImageLoader's pre-finalizer gets called. Then the clearWeakMember gets called. Consequently, willRemoveClient gets called twice. In this particular case, it might be safe to call willRemoveClient twice, but I'm considering a way to handle this scenario in general.
On 2015/07/13 15:29:48, haraken wrote: > On 2015/07/13 15:16:30, sof wrote: > > On 2015/07/13 15:15:19, haraken wrote: > > > On 2015/07/13 14:54:56, sof wrote: > > > > On 2015/07/13 14:51:55, haraken wrote: > > > > > On 2015/07/13 14:49:44, sof wrote: > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1224323003/diff/20001/Source/core/loader/Imag... > > > > > > File Source/core/loader/ImageLoader.h (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1224323003/diff/20001/Source/core/loader/Imag... > > > > > > Source/core/loader/ImageLoader.h:173: HashSet<ImageLoaderClient*> > > > m_clients; > > > > > > On 2015/07/13 14:46:56, haraken wrote: > > > > > > > > > > > > > > I'm wondering if we could just use > > > > > HeapHashSet<WeakMember<ImageLoaderClient>> > > > > > > > but doesn't it work for some reason? > > > > > > > > > > > > > > > > > > > We need to explicitly unregister (==willRemoveClient()) upon them > > falling > > > > > away, > > > > > > so a custom weak callback is needed. > > > > > > > > > > > > This representation mirrors how LocalFrame::m_pluginElements are kept. > > > > > > > > > > ah, makes sense. Would it be an option that we add a pre-finalizer to > > > > > ImageLoaderClient and call willRemoveClient(*client) in the > pre-finalizer? > > > > > > > > hmm, I think that's pushing the use of prefinalizers too far. > > > > > > Conceptually I think it should be a pre-finalizer, but if that's not nice > from > > > the performance perspective, I'm fine with the current CL. > > > > > > BTW, in the current CL, willRemoveClient won't be called if the ImageLoader > > and > > > its clients die together. Will it be OK? > > > > Isn't that done in dispose()? > > Thanks, I'm getting to understand; hmm, the situation looks complicated... > > What I'm concerned about is the following scenario: > > 1) A GC runs. The ImageLoader survives but a client dies. > 2) The next GC hits before the thread handles the thread-local weak processing > (i.e., before calling the clearWeakMember). The ImageLoader dies in the GC. > 3) The ImageLoader's pre-finalizer gets called. Then the clearWeakMember gets > called. Consequently, willRemoveClient gets called twice. > > In this particular case, it might be safe to call willRemoveClient twice, but > I'm considering a way to handle this scenario in general. How can a weak callback be called after the prefinalizer has run for that object? Isn't thread local processing done first in preSweep() ?
On 2015/07/13 15:34:54, sof wrote: > On 2015/07/13 15:29:48, haraken wrote: > > On 2015/07/13 15:16:30, sof wrote: > > > On 2015/07/13 15:15:19, haraken wrote: > > > > On 2015/07/13 14:54:56, sof wrote: > > > > > On 2015/07/13 14:51:55, haraken wrote: > > > > > > On 2015/07/13 14:49:44, sof wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1224323003/diff/20001/Source/core/loader/Imag... > > > > > > > File Source/core/loader/ImageLoader.h (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1224323003/diff/20001/Source/core/loader/Imag... > > > > > > > Source/core/loader/ImageLoader.h:173: HashSet<ImageLoaderClient*> > > > > m_clients; > > > > > > > On 2015/07/13 14:46:56, haraken wrote: > > > > > > > > > > > > > > > > I'm wondering if we could just use > > > > > > HeapHashSet<WeakMember<ImageLoaderClient>> > > > > > > > > but doesn't it work for some reason? > > > > > > > > > > > > > > > > > > > > > > We need to explicitly unregister (==willRemoveClient()) upon them > > > falling > > > > > > away, > > > > > > > so a custom weak callback is needed. > > > > > > > > > > > > > > This representation mirrors how LocalFrame::m_pluginElements are > kept. > > > > > > > > > > > > ah, makes sense. Would it be an option that we add a pre-finalizer to > > > > > > ImageLoaderClient and call willRemoveClient(*client) in the > > pre-finalizer? > > > > > > > > > > hmm, I think that's pushing the use of prefinalizers too far. > > > > > > > > Conceptually I think it should be a pre-finalizer, but if that's not nice > > from > > > > the performance perspective, I'm fine with the current CL. > > > > > > > > BTW, in the current CL, willRemoveClient won't be called if the > ImageLoader > > > and > > > > its clients die together. Will it be OK? > > > > > > Isn't that done in dispose()? > > > > Thanks, I'm getting to understand; hmm, the situation looks complicated... > > > > What I'm concerned about is the following scenario: > > > > 1) A GC runs. The ImageLoader survives but a client dies. > > 2) The next GC hits before the thread handles the thread-local weak processing > > (i.e., before calling the clearWeakMember). The ImageLoader dies in the GC. > > 3) The ImageLoader's pre-finalizer gets called. Then the clearWeakMember gets > > called. Consequently, willRemoveClient gets called twice. > > > > In this particular case, it might be safe to call willRemoveClient twice, but > > I'm considering a way to handle this scenario in general. > > How can a weak callback be called after the prefinalizer has run for that > object? Isn't thread local processing done first in preSweep() ? ah, I was confused. clearWeakMembers should get called first in any case (i.e., even if the pre-finalizer follows), so there should be no issue. LGTM.
The CQ bit was checked by sigbjornf@opera.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1224323003/20001
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://src.chromium.org/viewvc/blink?view=rev&revision=198790 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
