Issue 9022034: Convert various ReleaseSoon methods to use base::Bind()

Issue 9022034: Convert various ReleaseSoon methods to use base::Bind() (Closed)

Created:
8 years, 12 months ago by dcheng

Modified:
8 years, 11 months ago

Reviewers:
Jói, Andrew T Wilson (Slow), ajwong, awong, brettw, joi

CC:
chromium-reviews, sadrul, jam, dpranke-watch+content_chromium.org, joi+watch-content_chromium.org, darin-cc_chromium.org, brettw-cc_chromium.org

Base URL:
svn://svn.chromium.org/chrome/trunk/src

Visibility:
Public.

More Reviews

Description

Convert various ReleaseSoon methods to use base::Bind() BUG=none TEST=none TBR=brettw,atwilson Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=116030

Patch Set 1 #

Patch Set 2 : . #

Patch Set 3 : Using GMLPFT #

Patch Set 4 : Update #

Patch Set 5 : . #

Patch Set 6 : . #

Patch Set 7 : Fix test #

Created: 8 years, 11 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+84 lines, -48 lines)			Patch
M	base/message_loop.h	View	1 2 3 4 5	2 chunks	+7 lines, -1 line	0 comments	Download
M	base/message_loop.cc	View	1 2 3 4 5	1 chunk	+7 lines, -0 lines	0 comments	Download
M	base/message_loop_helpers.h	View	1 2 3 4	3 chunks	+46 lines, -15 lines	0 comments	Download
M	base/message_loop_proxy.h	View		3 chunks	+9 lines, -3 lines	0 comments	Download
M	base/message_loop_proxy.cc	View		1 chunk	+7 lines, -0 lines	0 comments	Download
M	base/task.h	View	1 2	1 chunk	+0 lines, -23 lines	0 comments	Download
M	chrome/browser/sync/glue/http_bridge_unittest.cc	View	1 2 3 4 5 6	1 chunk	+5 lines, -3 lines	0 comments	Download
M	content/public/browser/browser_thread.h	View	1 2	3 chunks	+3 lines, -3 lines	0 comments	Download

Messages

Total messages: 8 (0 generated)

Expand Messages | Collapse Messages

awong

LGTM with caveat that we should watch perf bots afterwards to see if the GetMessageLoopProxyForThread() ...

8 years, 11 months ago (2011-12-29 23:40:20 UTC) #3

joi

> Why isn't that thing cached in TLS anyways? It's not always the MessageLoopProxy for ...

8 years, 11 months ago (2011-12-30 13:20:31 UTC) #4

awong

On 2011/12/30 13:20:31, joi wrote: > > Why isn't that thing cached in TLS anyways? ...

8 years, 11 months ago (2012-01-03 20:42:43 UTC) #5

joi

> Sure, but on each thread that calls it, there could be a ID ->> ...

8 years, 11 months ago (2012-01-04 09:11:48 UTC) #6

awong

On 2012/01/04 09:11:48, joi wrote: > > Sure, but on each thread that calls it, ...

8 years, 11 months ago (2012-01-04 19:57:16 UTC) #7

On 2012/01/04 09:11:48, joi wrote:
> > Sure, but on each thread that calls it, there could be a ID ->>
> MessageLoopProxy> map inside TLS couldn't there?
> Sure, that could be done.  Before adding the complexity of caching for
> each thread, and the slight amount of extra per-thread storage, you
> would want to verify that there is actually contention on the lock
> over g_browser_threads (in content/browser/browser_thread_impl.cc).
> Especially because there is a trick employed in
> BrowserThreadImpl::PostTaskHelper to not even grab the lock when the
> target thread is guaranteed to outlive the current thread.

I'm not quite sure that effects this.  If we changed
BrowserThread::GetMessageLoopProxyForThread()to be:

scoped_refptr<MessageLoopProxy> BrowserThread::GetMessageLoopProxyForThread(int
id) {
  // tls_mlp_map is an std::map.  Could even use an array if you
  // want.
  if (tls_mlp_map->find(id) == tls_mlp_map->end()) {
    tls_mlp_map[id] = new BrowserThreadMessageLoopProxy(id);
  }

  return tls_mlp_map[id];
}

The lock doesn't come into play at all.

You're right that we do pick up a little memory overhead and additional
complexity as the trade for avoiding the malloc on each call to
GetMessageLoopProxyForThread().  I'm not sure it's worth it either.

-Albert

 
> On Tue, Jan 3, 2012 at 8:42 PM,  <mailto:ajwong@chromium.org> wrote:
> > On 2011/12/30 13:20:31, joi wrote:
> >>
> >> > Why isn't that thing cached in TLS anyways?
> >> It's not always the MessageLoopProxy for the current thread (see ID
> >> parameter). &nbsp;Maybe I'm misunderstanding the question.
> >
> >
> > Sure, but on each thread that calls it, there could be a ID ->
> > MessageLoopProxy
> > map inside TLS couldn't there?
> >
> >
> >> Cheers,
> >> Jói
> >
> >
> >
> >> On Thu, Dec 29, 2011 at 11:40 PM, &nbsp;<mailto:ajwong@chromium.org> wrote:
> >> > LGTM
> >> >
> >> > with caveat that we should watch perf bots afterwards to see if the
> >> > GetMessageLoopProxyForThread() call in BrowserThread actually ends up
> >> > eating
> >> > any
> >> > serious amounts time.
> >> >
> >> > Why isn't that thing cached in TLS anyways?
> >> >
> >> > http://codereview.chromium.org/9022034/
> >
> >
> >
> >
> > http://codereview.chromium.org/9022034/

joi

8 years, 11 months ago (2012-01-05 09:28:03 UTC) #8

Still LGTM :)

Ah, I see, I misunderstood your proposal a bit - you wanted to cache
MessageLoopProxy objects per thread, I was thinking BrowserThreadImpl
objects (like g_browser_threads in
content/browser/browser_thread_impl.cc).  Your proposal is much more
sensible than what I thought you meant :D but I think we agree we
won't make that change for now at least.  It might be an interesting
thing to profile though.

Cheers,
Jói


On Wed, Jan 4, 2012 at 7:57 PM,  <ajwong@chromium.org> wrote:
> On 2012/01/04 09:11:48, joi wrote:
>>
>> > Sure, but on each thread that calls it, there could be a ID ->>
>> MessageLoopProxy> map inside TLS couldn't there?
>> Sure, that could be done.  Before adding the complexity of caching for
>> each thread, and the slight amount of extra per-thread storage, you
>> would want to verify that there is actually contention on the lock
>> over g_browser_threads (in content/browser/browser_thread_impl.cc).
>> Especially because there is a trick employed in
>> BrowserThreadImpl::PostTaskHelper to not even grab the lock when the
>> target thread is guaranteed to outlive the current thread.
>
>
> I'm not quite sure that effects this.  If we changed
> BrowserThread::GetMessageLoopProxyForThread()to be:
>
> scoped_refptr<MessageLoopProxy>
> BrowserThread::GetMessageLoopProxyForThread(int
> id) {
>  // tls_mlp_map is an std::map.  Could even use an array if you
>  // want.
>  if (tls_mlp_map->find(id) == tls_mlp_map->end()) {
>    tls_mlp_map[id] = new BrowserThreadMessageLoopProxy(id);
>  }
>
>  return tls_mlp_map[id];
> }
>
> The lock doesn't come into play at all.
>
> You're right that we do pick up a little memory overhead and additional
> complexity as the trade for avoiding the malloc on each call to
> GetMessageLoopProxyForThread().  I'm not sure it's worth it either.
>
> -Albert
>
>
>> On Tue, Jan 3, 2012 at 8:42 PM,  <mailto:ajwong@chromium.org> wrote:
>> > On 2011/12/30 13:20:31, joi wrote:
>> >>
>> >> > Why isn't that thing cached in TLS anyways?
>> >> It's not always the MessageLoopProxy for the current thread (see ID
>> >> parameter). &nbsp;Maybe I'm misunderstanding the question.
>>
>> >
>> >
>> > Sure, but on each thread that calls it, there could be a ID ->
>> > MessageLoopProxy
>> > map inside TLS couldn't there?
>> >
>> >
>> >> Cheers,
>> >> Jói
>> >
>> >
>> >
>> >> On Thu, Dec 29, 2011 at 11:40 PM, &nbsp;<mailto:ajwong@chromium.org>
>> >> wrote:
>> >> > LGTM
>> >> >
>> >> > with caveat that we should watch perf bots afterwards to see if the
>> >> > GetMessageLoopProxyForThread() call in BrowserThread actually ends up
>> >> > eating
>> >> > any
>> >> > serious amounts time.
>> >> >
>> >> > Why isn't that thing cached in TLS anyways?
>> >> >
>> >> > http://codereview.chromium.org/9022034/
>> >
>> >
>> >
>> >
>> > http://codereview.chromium.org/9022034/
>
>
>
>
> http://codereview.chromium.org/9022034/

Expand Messages | Collapse Messages