Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
53153002:
Pepper: always delete ResourceMessageFilter objects on the creation thread. (Closed)
|
Created:
7 years, 1 month ago by yzshen1 Modified:
7 years, 1 month ago Reviewers:
dmichael (off chromium) CC:
chromium-reviews, extensions-reviews_chromium.org, jam, dcheng, joi+watch-content_chromium.org, darin-cc_chromium.org, chromium-apps-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionPepper: always delete ResourceMessageFilter objects on the creation thread.
BUG=312504
TEST=None
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=235905
Patch Set 1 #
Total comments: 4
Patch Set 2 : #
Total comments: 4
Patch Set 3 : #Patch Set 4 : #Patch Set 5 : #
Messages
Total messages: 14 (0 generated)
Hi, David. Would you please take a look? Thanks!
https://codereview.chromium.org/53153002/diff/1/ppapi/host/resource_message_f... File ppapi/host/resource_message_filter.cc (right): https://codereview.chromium.org/53153002/diff/1/ppapi/host/resource_message_f... ppapi/host/resource_message_filter.cc:105: deletion_message_loop_proxy_->DeleteSoon(FROM_HERE, this); Would there be any harm in just always deleting it on its creation thread? We might even just DCHECK in the constructor that it's the IO thread, and explicitly use DeleteSoon on the IO thread (instead of storing the message_loop_proxy). My *ideal* would be if these acted more like the Hosts, where they aren't ref-counted at all, and the lifetime is very obvious. But I haven't thought hard enough to see if that's at all practical...
https://codereview.chromium.org/53153002/diff/1/ppapi/host/resource_message_f... File ppapi/host/resource_message_filter.cc (right): https://codereview.chromium.org/53153002/diff/1/ppapi/host/resource_message_f... ppapi/host/resource_message_filter.cc:105: deletion_message_loop_proxy_->DeleteSoon(FROM_HERE, this); On 2013/10/30 22:05:42, dmichael wrote: > Would there be any harm in just always deleting it on its creation thread? It is not harmful for all the existing message filters. Just not necessary for some of the message filters. They could be deleted on any thread. I think that saves us some post tasks. But maybe that is not a big gain. :) > We might even just DCHECK in the constructor that it's the IO thread, and > explicitly use DeleteSoon on the IO thread (instead of storing the > message_loop_proxy). If a message filter is in the renderer process, the creation/deletion thread will be the main thread, not the IO thread. That is different than filters in the browser processes. > My *ideal* would be if these acted more like the Hosts, where they aren't > ref-counted at all, and the lifetime is very obvious. But I haven't thought hard > enough to see if that's at all practical... Because we post tasks between threads, I don't know a better way than ref-counting it. :/ Weakptr doesn't work well for multi-threading scenario, IIRC. I will also think harder. Thanks!
https://codereview.chromium.org/53153002/diff/1/ppapi/host/resource_message_f... File ppapi/host/resource_message_filter.cc (right): https://codereview.chromium.org/53153002/diff/1/ppapi/host/resource_message_f... ppapi/host/resource_message_filter.cc:105: deletion_message_loop_proxy_->DeleteSoon(FROM_HERE, this); On 2013/10/30 22:11:55, yzshen1 wrote: > On 2013/10/30 22:05:42, dmichael wrote: > > Would there be any harm in just always deleting it on its creation thread? > It is not harmful for all the existing message filters. Just not necessary for > some of the message filters. They could be deleted on any thread. I think that > saves us some post tasks. But maybe that is not a big gain. :) I see what you mean, but I would lean in favor of keeping the code simpler, so (a) the individual filters don't need to specify the deletion thread, and (b) we don't have think hard to make sure a given filter really can be deleted on any thread. I think it's just easier to understand if we say they're always created and deleted on the same thread. I don't think the performance difference is going to really be noticeable. WDYT? > > > We might even just DCHECK in the constructor that it's the IO thread, and > > explicitly use DeleteSoon on the IO thread (instead of storing the > > message_loop_proxy). > > If a message filter is in the renderer process, the creation/deletion thread > will be the main thread, not the IO thread. That is different than filters in > the browser processes. Okay, I forgot about that. Thanks for explaining. > > > My *ideal* would be if these acted more like the Hosts, where they aren't > > ref-counted at all, and the lifetime is very obvious. But I haven't thought > hard > > enough to see if that's at all practical... > Because we post tasks between threads, I don't know a better way than > ref-counting it. :/ > Weakptr doesn't work well for multi-threading scenario, IIRC. > > I will also think harder. > > Thanks! > Yeah, I don't think it would be very easy to do this. We accomplish it sometimes by having a nested sort of "core" class which is RefCountedThreadSafe, while the outer class has a more clearly defined lifetime. It matters more for classes that are providing an API in the process. Since these Filters mostly only respond to IPC, there probably isn't much to be gained by doing all that work.
https://codereview.chromium.org/53153002/diff/1/ppapi/host/resource_message_f... File ppapi/host/resource_message_filter.cc (right): https://codereview.chromium.org/53153002/diff/1/ppapi/host/resource_message_f... ppapi/host/resource_message_filter.cc:105: deletion_message_loop_proxy_->DeleteSoon(FROM_HERE, this); > I see what you mean, but I would lean in favor of keeping the code simpler, so > (a) the individual filters don't need to specify the deletion thread, and (b) we > don't have think hard to make sure a given filter really can be deleted on any > thread. I think it's just easier to understand if we say they're always created > and deleted on the same thread. I don't think the performance difference is > going to really be noticeable. WDYT? Yeah. Sounds good. I have made the change. > Yeah, I don't think it would be very easy to do this. We accomplish it sometimes > by having a nested sort of "core" class which is RefCountedThreadSafe, while the > outer class has a more clearly defined lifetime. It matters more for classes > that are providing an API in the process. Since these Filters mostly only > respond to IPC, there probably isn't much to be gained by doing all that work. I think it is actually similar to the solution you mentioned above: ResourceHost is acting as the outer, non-ref-counted class, while the ResourceMessageFilter it contains is the inner, ref-counted class. Thanks!
On 2013/10/31 21:28:04, yzshen1 wrote: > https://codereview.chromium.org/53153002/diff/1/ppapi/host/resource_message_f... > File ppapi/host/resource_message_filter.cc (right): > > https://codereview.chromium.org/53153002/diff/1/ppapi/host/resource_message_f... > ppapi/host/resource_message_filter.cc:105: > deletion_message_loop_proxy_->DeleteSoon(FROM_HERE, this); > > I see what you mean, but I would lean in favor of keeping the code simpler, so > > (a) the individual filters don't need to specify the deletion thread, and (b) > we > > don't have think hard to make sure a given filter really can be deleted on any > > thread. I think it's just easier to understand if we say they're always > created > > and deleted on the same thread. I don't think the performance difference is > > going to really be noticeable. WDYT? > > Yeah. Sounds good. I have made the change. > > > Yeah, I don't think it would be very easy to do this. We accomplish it > sometimes > > by having a nested sort of "core" class which is RefCountedThreadSafe, while > the > > outer class has a more clearly defined lifetime. It matters more for classes > > that are providing an API in the process. Since these Filters mostly only > > respond to IPC, there probably isn't much to be gained by doing all that work. > > I think it is actually similar to the solution you mentioned above: ResourceHost > is acting as the outer, non-ref-counted class, while the ResourceMessageFilter > it contains is the inner, ref-counted class. > > Thanks! Friendly ping. David: would you please take another look? I followed your advice and changed to always delete ResourceMessageFilter on the creation thread. It should be much simpler now. Thanks!
Thanks for reminding me; I was doing the halloween thing last night and forgot all about this. lgtm https://codereview.chromium.org/53153002/diff/210001/ppapi/host/resource_mess... File ppapi/host/resource_message_filter.h (right): https://codereview.chromium.org/53153002/diff/210001/ppapi/host/resource_mess... ppapi/host/resource_message_filter.h:117: ResourceMessageFilter, internal::ResourceMessageFilterDeleteTraits>; Are these friend decls both still necessary? https://codereview.chromium.org/53153002/diff/210001/ppapi/host/resource_mess... ppapi/host/resource_message_filter.h:118: friend struct internal::ResourceMessageFilterDeleteTraits; Optional: strictly speaking, you could do the work of InternalDestruct inside ResourceMessageFilterDeleteTraits, since it's a friend. Then you wouldn't need InternalDestruct, and wouldn't have to do "delete this", which always looks a little scary (even when it's right :-) ).
Thanks, David! https://codereview.chromium.org/53153002/diff/210001/ppapi/host/resource_mess... File ppapi/host/resource_message_filter.h (right): https://codereview.chromium.org/53153002/diff/210001/ppapi/host/resource_mess... ppapi/host/resource_message_filter.h:117: ResourceMessageFilter, internal::ResourceMessageFilterDeleteTraits>; On 2013/11/01 20:30:39, dmichael wrote: > Are these friend decls both still necessary? Yeah, unfortunately all of them are necessary. :/ https://codereview.chromium.org/53153002/diff/210001/ppapi/host/resource_mess... ppapi/host/resource_message_filter.h:118: friend struct internal::ResourceMessageFilterDeleteTraits; Done. Thanks! On 2013/11/01 20:30:39, dmichael wrote: > Optional: strictly speaking, you could do the work of InternalDestruct inside > ResourceMessageFilterDeleteTraits, since it's a friend. Then you wouldn't need > InternalDestruct, and wouldn't have to do "delete this", which always looks a > little scary (even when it's right :-) ).
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yzshen@chromium.org/53153002/290001
Sorry for I got bad news for ya. Compile failed with a clobber build on linux_clang. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yzshen@chromium.org/53153002/550001
Retried try job too often on linux_rel for step(s) ppapi_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yzshen@chromium.org/53153002/780001
Message was sent while issue was closed.
Change committed as 235905 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
