Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10900020:
Fix regression in BrowserThread's optimization of when it skips a lock. The variable name was misna… (Closed)
|
Created:
8 years, 3 months ago by jam Modified:
8 years, 3 months ago Reviewers:
Jói CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam Base URL:
svn://chrome-svn/chrome/trunk/src/ Visibility:
Public. |
DescriptionFix regression in BrowserThread's optimization of when it skips a lock. The variable name was misnamed before, which contributed to this. I've updated the variable name to make it clearer.
Credit to liujundota@gmail.com who noticed this.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=153977
Patch Set 1 #
Messages
Total messages: 8 (0 generated)
test8 On Thu, Aug 30, 2012 at 6:31 PM, John Abd-El-Malek <jam@chromium.org> wrote: > test 7 > > > On Thu, Aug 30, 2012 at 6:24 PM, John Abd-El-Malek <jam@chromium.org>wrote: > >> test6 >> >> >> On Thu, Aug 30, 2012 at 6:20 PM, John Abd-El-Malek <jam@chromium.org>wrote: >> >>> test4 >>> >>> >>> On Thu, Aug 30, 2012 at 5:44 PM, John Abd-El-Malek <jam@chromium.org>wrote: >>> >>>> (test again) >>>> >>>> >>>> On Wed, Aug 29, 2012 at 2:23 PM, John Abd-El-Malek <jam@chromium.org>wrote: >>>> >>>>> Done (also testing rietveld update by email) >>>>> >>>>> >>>>> On Wed, Aug 29, 2012 at 7:30 AM, Jói Sigurðsson <joi@chromium.org>wrote: >>>>> >>>>>> Doh! I started writing a long essay on why this was wrong, but it's >>>>>> not. LGTM. >>>>>> >>>>>> The variable name was confusing before, and also the comment on lines >>>>>> 234-236 is a bit too wishy-washy, can you improve the comment to say >>>>>> "target thread" and "current thread" instead of "other thread" and >>>>>> "this one"? Maybe even add "since in that case the target thread's >>>>>> destructor, which clears its entry from globals.threads, cannot run >>>>>> while the current thread is running"? >>>>>> >>>>>> Cheers, >>>>>> Jói >>>>>> >>>>>> >>>>>> On Tue, Aug 28, 2012 at 11:09 PM, <jam@chromium.org> wrote: >>>>>> > Reviewers: Jói, >>>>>> > >>>>>> > Description: >>>>>> > Fix regression in BrowserThread's optimization of when it skips a >>>>>> lock. The >>>>>> > variable name was misnamed before, which contributed to this. I've >>>>>> updated >>>>>> > the >>>>>> > variable name to make it clearer. >>>>>> > >>>>>> > Credit to liujundota@gmail.com who noticed this. >>>>>> > >>>>>> > Please review this at http://codereview.chromium.org/10900020/ >>>>>> > >>>>>> > SVN Base: svn://chrome-svn/chrome/trunk/src/ >>>>>> > >>>>>> > Affected files: >>>>>> > M content/browser/browser_thread_impl.cc >>>>>> > >>>>>> > >>>>>> > Index: content/browser/browser_thread_impl.cc >>>>>> > =================================================================== >>>>>> > --- content/browser/browser_thread_impl.cc (revision 153862) >>>>>> > +++ content/browser/browser_thread_impl.cc (working copy) >>>>>> > @@ -238,12 +238,12 @@ >>>>>> > // which would require a lock because std::map isn't thread safe, >>>>>> > defeating >>>>>> > // the whole purpose of this optimization. >>>>>> > BrowserThread::ID current_thread; >>>>>> > - bool guaranteed_to_outlive_target_thread = >>>>>> > + bool target_thread_outlives_current = >>>>>> > GetCurrentThreadIdentifier(¤t_thread) && >>>>>> > - current_thread <= identifier; >>>>>> > + current_thread >= identifier; >>>>>> > >>>>>> > BrowserThreadGlobals& globals = g_globals.Get(); >>>>>> > - if (!guaranteed_to_outlive_target_thread) >>>>>> > + if (!target_thread_outlives_current) >>>>>> > globals.lock.Acquire(); >>>>>> > >>>>>> > MessageLoop* message_loop = globals.threads[identifier] ? >>>>>> > @@ -256,7 +256,7 @@ >>>>>> > } >>>>>> > } >>>>>> > >>>>>> > - if (!guaranteed_to_outlive_target_thread) >>>>>> > + if (!target_thread_outlives_current) >>>>>> > globals.lock.Release(); >>>>>> > >>>>>> > return !!message_loop; >>>>>> > >>>>>> > >>>>>> >>>>> >>>>> >>>> >>> >> >
test9 On Thu, Aug 30, 2012 at 7:11 PM, John Abd-El-Malek <jam@chromium.org> wrote: > (for those wondering, I'm testing out fixes for reply-by-email, which has > been mostly busted for a while) > > > On Thu, Aug 30, 2012 at 6:58 PM, John Abd-El-Malek <jam@chromium.org>wrote: > >> test8 >> >> >> On Thu, Aug 30, 2012 at 6:31 PM, John Abd-El-Malek <jam@chromium.org>wrote: >> >>> test 7 >>> >>> >>> On Thu, Aug 30, 2012 at 6:24 PM, John Abd-El-Malek <jam@chromium.org>wrote: >>> >>>> test6 >>>> >>>> >>>> On Thu, Aug 30, 2012 at 6:20 PM, John Abd-El-Malek <jam@chromium.org>wrote: >>>> >>>>> test4 >>>>> >>>>> >>>>> On Thu, Aug 30, 2012 at 5:44 PM, John Abd-El-Malek <jam@chromium.org>wrote: >>>>> >>>>>> (test again) >>>>>> >>>>>> >>>>>> On Wed, Aug 29, 2012 at 2:23 PM, John Abd-El-Malek <jam@chromium.org>wrote: >>>>>> >>>>>>> Done (also testing rietveld update by email) >>>>>>> >>>>>>> >>>>>>> On Wed, Aug 29, 2012 at 7:30 AM, Jói Sigurðsson <joi@chromium.org>wrote: >>>>>>> >>>>>>>> Doh! I started writing a long essay on why this was wrong, but >>>>>>>> it's not. LGTM. >>>>>>>> >>>>>>>> The variable name was confusing before, and also the comment on >>>>>>>> lines >>>>>>>> 234-236 is a bit too wishy-washy, can you improve the comment to say >>>>>>>> "target thread" and "current thread" instead of "other thread" and >>>>>>>> "this one"? Maybe even add "since in that case the target thread's >>>>>>>> destructor, which clears its entry from globals.threads, cannot run >>>>>>>> while the current thread is running"? >>>>>>>> >>>>>>>> Cheers, >>>>>>>> Jói >>>>>>>> >>>>>>>> >>>>>>>> On Tue, Aug 28, 2012 at 11:09 PM, <jam@chromium.org> wrote: >>>>>>>> > Reviewers: Jói, >>>>>>>> > >>>>>>>> > Description: >>>>>>>> > Fix regression in BrowserThread's optimization of when it skips a >>>>>>>> lock. The >>>>>>>> > variable name was misnamed before, which contributed to this. >>>>>>>> I've updated >>>>>>>> > the >>>>>>>> > variable name to make it clearer. >>>>>>>> > >>>>>>>> > Credit to liujundota@gmail.com who noticed this. >>>>>>>> > >>>>>>>> > Please review this at http://codereview.chromium.org/10900020/ >>>>>>>> > >>>>>>>> > SVN Base: svn://chrome-svn/chrome/trunk/src/ >>>>>>>> > >>>>>>>> > Affected files: >>>>>>>> > M content/browser/browser_thread_impl.cc >>>>>>>> > >>>>>>>> > >>>>>>>> > Index: content/browser/browser_thread_impl.cc >>>>>>>> > >>>>>>>> =================================================================== >>>>>>>> > --- content/browser/browser_thread_impl.cc (revision 153862) >>>>>>>> > +++ content/browser/browser_thread_impl.cc (working copy) >>>>>>>> > @@ -238,12 +238,12 @@ >>>>>>>> > // which would require a lock because std::map isn't thread >>>>>>>> safe, >>>>>>>> > defeating >>>>>>>> > // the whole purpose of this optimization. >>>>>>>> > BrowserThread::ID current_thread; >>>>>>>> > - bool guaranteed_to_outlive_target_thread = >>>>>>>> > + bool target_thread_outlives_current = >>>>>>>> > GetCurrentThreadIdentifier(¤t_thread) && >>>>>>>> > - current_thread <= identifier; >>>>>>>> > + current_thread >= identifier; >>>>>>>> > >>>>>>>> > BrowserThreadGlobals& globals = g_globals.Get(); >>>>>>>> > - if (!guaranteed_to_outlive_target_thread) >>>>>>>> > + if (!target_thread_outlives_current) >>>>>>>> > globals.lock.Acquire(); >>>>>>>> > >>>>>>>> > MessageLoop* message_loop = globals.threads[identifier] ? >>>>>>>> > @@ -256,7 +256,7 @@ >>>>>>>> > } >>>>>>>> > } >>>>>>>> > >>>>>>>> > - if (!guaranteed_to_outlive_target_thread) >>>>>>>> > + if (!target_thread_outlives_current) >>>>>>>> > globals.lock.Release(); >>>>>>>> > >>>>>>>> > return !!message_loop; >>>>>>>> > >>>>>>>> > >>>>>>>> >>>>>>> >>>>>>> >>>>>> >>>>> >>>> >>> >> >
test11 On Wed, Oct 3, 2012 at 6:13 PM, John Abd-El-Malek <jam@chromium.org> wrote: > test10 > > > On Fri, Aug 31, 2012 at 9:14 AM, John Abd-El-Malek <jam@chromium.org>wrote: > >> test9 >> >> >> On Thu, Aug 30, 2012 at 7:11 PM, John Abd-El-Malek <jam@chromium.org>wrote: >> >>> (for those wondering, I'm testing out fixes for reply-by-email, which >>> has been mostly busted for a while) >>> >>> >>> On Thu, Aug 30, 2012 at 6:58 PM, John Abd-El-Malek <jam@chromium.org>wrote: >>> >>>> test8 >>>> >>>> >>>> On Thu, Aug 30, 2012 at 6:31 PM, John Abd-El-Malek <jam@chromium.org>wrote: >>>> >>>>> test 7 >>>>> >>>>> >>>>> On Thu, Aug 30, 2012 at 6:24 PM, John Abd-El-Malek <jam@chromium.org>wrote: >>>>> >>>>>> test6 >>>>>> >>>>>> >>>>>> On Thu, Aug 30, 2012 at 6:20 PM, John Abd-El-Malek <jam@chromium.org>wrote: >>>>>> >>>>>>> test4 >>>>>>> >>>>>>> >>>>>>> On Thu, Aug 30, 2012 at 5:44 PM, John Abd-El-Malek <jam@chromium.org >>>>>>> > wrote: >>>>>>> >>>>>>>> (test again) >>>>>>>> >>>>>>>> >>>>>>>> On Wed, Aug 29, 2012 at 2:23 PM, John Abd-El-Malek < >>>>>>>> jam@chromium.org> wrote: >>>>>>>> >>>>>>>>> Done (also testing rietveld update by email) >>>>>>>>> >>>>>>>>> >>>>>>>>> On Wed, Aug 29, 2012 at 7:30 AM, Jói Sigurðsson <joi@chromium.org>wrote: >>>>>>>>> >>>>>>>>>> Doh! I started writing a long essay on why this was wrong, but >>>>>>>>>> it's not. LGTM. >>>>>>>>>> >>>>>>>>>> The variable name was confusing before, and also the comment on >>>>>>>>>> lines >>>>>>>>>> 234-236 is a bit too wishy-washy, can you improve the comment to >>>>>>>>>> say >>>>>>>>>> "target thread" and "current thread" instead of "other thread" and >>>>>>>>>> "this one"? Maybe even add "since in that case the target >>>>>>>>>> thread's >>>>>>>>>> destructor, which clears its entry from globals.threads, cannot >>>>>>>>>> run >>>>>>>>>> while the current thread is running"? >>>>>>>>>> >>>>>>>>>> Cheers, >>>>>>>>>> Jói >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> On Tue, Aug 28, 2012 at 11:09 PM, <jam@chromium.org> wrote: >>>>>>>>>> > Reviewers: Jói, >>>>>>>>>> > >>>>>>>>>> > Description: >>>>>>>>>> > Fix regression in BrowserThread's optimization of when it skips >>>>>>>>>> a lock. The >>>>>>>>>> > variable name was misnamed before, which contributed to this. >>>>>>>>>> I've updated >>>>>>>>>> > the >>>>>>>>>> > variable name to make it clearer. >>>>>>>>>> > >>>>>>>>>> > Credit to liujundota@gmail.com who noticed this. >>>>>>>>>> > >>>>>>>>>> > Please review this at http://codereview.chromium.org/10900020/ >>>>>>>>>> > >>>>>>>>>> > SVN Base: svn://chrome-svn/chrome/trunk/src/ >>>>>>>>>> > >>>>>>>>>> > Affected files: >>>>>>>>>> > M content/browser/browser_thread_impl.cc >>>>>>>>>> > >>>>>>>>>> > >>>>>>>>>> > Index: content/browser/browser_thread_impl.cc >>>>>>>>>> > >>>>>>>>>> =================================================================== >>>>>>>>>> > --- content/browser/browser_thread_impl.cc (revision >>>>>>>>>> 153862) >>>>>>>>>> > +++ content/browser/browser_thread_impl.cc (working copy) >>>>>>>>>> > @@ -238,12 +238,12 @@ >>>>>>>>>> > // which would require a lock because std::map isn't thread >>>>>>>>>> safe, >>>>>>>>>> > defeating >>>>>>>>>> > // the whole purpose of this optimization. >>>>>>>>>> > BrowserThread::ID current_thread; >>>>>>>>>> > - bool guaranteed_to_outlive_target_thread = >>>>>>>>>> > + bool target_thread_outlives_current = >>>>>>>>>> > GetCurrentThreadIdentifier(¤t_thread) && >>>>>>>>>> > - current_thread <= identifier; >>>>>>>>>> > + current_thread >= identifier; >>>>>>>>>> > >>>>>>>>>> > BrowserThreadGlobals& globals = g_globals.Get(); >>>>>>>>>> > - if (!guaranteed_to_outlive_target_thread) >>>>>>>>>> > + if (!target_thread_outlives_current) >>>>>>>>>> > globals.lock.Acquire(); >>>>>>>>>> > >>>>>>>>>> > MessageLoop* message_loop = globals.threads[identifier] ? >>>>>>>>>> > @@ -256,7 +256,7 @@ >>>>>>>>>> > } >>>>>>>>>> > } >>>>>>>>>> > >>>>>>>>>> > - if (!guaranteed_to_outlive_target_thread) >>>>>>>>>> > + if (!target_thread_outlives_current) >>>>>>>>>> > globals.lock.Release(); >>>>>>>>>> > >>>>>>>>>> > return !!message_loop; >>>>>>>>>> > >>>>>>>>>> > >>>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>> >>>>>>> >>>>>> >>>>> >>>> >>> >> >
test14 On Wed, Oct 3, 2012 at 10:45 PM, John Abd-El-Malek <jam@chromium.org> wrote: > test13 > > > On Wed, Oct 3, 2012 at 6:30 PM, John Abd-El-Malek <jam@chromium.org>wrote: > >> test12 >> >> >> On Wed, Oct 3, 2012 at 6:13 PM, John Abd-El-Malek <jam@chromium.org>wrote: >> >>> test10 >>> >>> >>> On Fri, Aug 31, 2012 at 9:14 AM, John Abd-El-Malek <jam@chromium.org>wrote: >>> >>>> test9 >>>> >>>> >>>> On Thu, Aug 30, 2012 at 7:11 PM, John Abd-El-Malek <jam@chromium.org>wrote: >>>> >>>>> (for those wondering, I'm testing out fixes for reply-by-email, which >>>>> has been mostly busted for a while) >>>>> >>>>> >>>>> On Thu, Aug 30, 2012 at 6:58 PM, John Abd-El-Malek <jam@chromium.org>wrote: >>>>> >>>>>> test8 >>>>>> >>>>>> >>>>>> On Thu, Aug 30, 2012 at 6:31 PM, John Abd-El-Malek <jam@chromium.org>wrote: >>>>>> >>>>>>> test 7 >>>>>>> >>>>>>> >>>>>>> On Thu, Aug 30, 2012 at 6:24 PM, John Abd-El-Malek <jam@chromium.org >>>>>>> > wrote: >>>>>>> >>>>>>>> test6 >>>>>>>> >>>>>>>> >>>>>>>> On Thu, Aug 30, 2012 at 6:20 PM, John Abd-El-Malek < >>>>>>>> jam@chromium.org> wrote: >>>>>>>> >>>>>>>>> test4 >>>>>>>>> >>>>>>>>> >>>>>>>>> On Thu, Aug 30, 2012 at 5:44 PM, John Abd-El-Malek < >>>>>>>>> jam@chromium.org> wrote: >>>>>>>>> >>>>>>>>>> (test again) >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> On Wed, Aug 29, 2012 at 2:23 PM, John Abd-El-Malek < >>>>>>>>>> jam@chromium.org> wrote: >>>>>>>>>> >>>>>>>>>>> Done (also testing rietveld update by email) >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> On Wed, Aug 29, 2012 at 7:30 AM, Jói Sigurðsson < >>>>>>>>>>> joi@chromium.org> wrote: >>>>>>>>>>> >>>>>>>>>>>> Doh! I started writing a long essay on why this was wrong, but >>>>>>>>>>>> it's not. LGTM. >>>>>>>>>>>> >>>>>>>>>>>> The variable name was confusing before, and also the comment on >>>>>>>>>>>> lines >>>>>>>>>>>> 234-236 is a bit too wishy-washy, can you improve the comment >>>>>>>>>>>> to say >>>>>>>>>>>> "target thread" and "current thread" instead of "other thread" >>>>>>>>>>>> and >>>>>>>>>>>> "this one"? Maybe even add "since in that case the target >>>>>>>>>>>> thread's >>>>>>>>>>>> destructor, which clears its entry from globals.threads, cannot >>>>>>>>>>>> run >>>>>>>>>>>> while the current thread is running"? >>>>>>>>>>>> >>>>>>>>>>>> Cheers, >>>>>>>>>>>> Jói >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> On Tue, Aug 28, 2012 at 11:09 PM, <jam@chromium.org> wrote: >>>>>>>>>>>> > Reviewers: Jói, >>>>>>>>>>>> > >>>>>>>>>>>> > Description: >>>>>>>>>>>> > Fix regression in BrowserThread's optimization of when it >>>>>>>>>>>> skips a lock. The >>>>>>>>>>>> > variable name was misnamed before, which contributed to this. >>>>>>>>>>>> I've updated >>>>>>>>>>>> > the >>>>>>>>>>>> > variable name to make it clearer. >>>>>>>>>>>> > >>>>>>>>>>>> > Credit to liujundota@gmail.com who noticed this. >>>>>>>>>>>> > >>>>>>>>>>>> > Please review this at >>>>>>>>>>>> http://codereview.chromium.org/10900020/ >>>>>>>>>>>> > >>>>>>>>>>>> > SVN Base: svn://chrome-svn/chrome/trunk/src/ >>>>>>>>>>>> > >>>>>>>>>>>> > Affected files: >>>>>>>>>>>> > M content/browser/browser_thread_impl.cc >>>>>>>>>>>> > >>>>>>>>>>>> > >>>>>>>>>>>> > Index: content/browser/browser_thread_impl.cc >>>>>>>>>>>> > >>>>>>>>>>>> =================================================================== >>>>>>>>>>>> > --- content/browser/browser_thread_impl.cc (revision >>>>>>>>>>>> 153862) >>>>>>>>>>>> > +++ content/browser/browser_thread_impl.cc (working copy) >>>>>>>>>>>> > @@ -238,12 +238,12 @@ >>>>>>>>>>>> > // which would require a lock because std::map isn't >>>>>>>>>>>> thread safe, >>>>>>>>>>>> > defeating >>>>>>>>>>>> > // the whole purpose of this optimization. >>>>>>>>>>>> > BrowserThread::ID current_thread; >>>>>>>>>>>> > - bool guaranteed_to_outlive_target_thread = >>>>>>>>>>>> > + bool target_thread_outlives_current = >>>>>>>>>>>> > GetCurrentThreadIdentifier(¤t_thread) && >>>>>>>>>>>> > - current_thread <= identifier; >>>>>>>>>>>> > + current_thread >= identifier; >>>>>>>>>>>> > >>>>>>>>>>>> > BrowserThreadGlobals& globals = g_globals.Get(); >>>>>>>>>>>> > - if (!guaranteed_to_outlive_target_thread) >>>>>>>>>>>> > + if (!target_thread_outlives_current) >>>>>>>>>>>> > globals.lock.Acquire(); >>>>>>>>>>>> > >>>>>>>>>>>> > MessageLoop* message_loop = globals.threads[identifier] ? >>>>>>>>>>>> > @@ -256,7 +256,7 @@ >>>>>>>>>>>> > } >>>>>>>>>>>> > } >>>>>>>>>>>> > >>>>>>>>>>>> > - if (!guaranteed_to_outlive_target_thread) >>>>>>>>>>>> > + if (!target_thread_outlives_current) >>>>>>>>>>>> > globals.lock.Release(); >>>>>>>>>>>> > >>>>>>>>>>>> > return !!message_loop; >>>>>>>>>>>> > >>>>>>>>>>>> > >>>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>> >>>>>>>>> >>>>>>>> >>>>>>> >>>>>> >>>>> >>>> >>> >> >
test13 On Wed, Oct 3, 2012 at 6:30 PM, John Abd-El-Malek <jam@chromium.org> wrote: > test12 > > > On Wed, Oct 3, 2012 at 6:13 PM, John Abd-El-Malek <jam@chromium.org>wrote: > >> test10 >> >> >> On Fri, Aug 31, 2012 at 9:14 AM, John Abd-El-Malek <jam@chromium.org>wrote: >> >>> test9 >>> >>> >>> On Thu, Aug 30, 2012 at 7:11 PM, John Abd-El-Malek <jam@chromium.org>wrote: >>> >>>> (for those wondering, I'm testing out fixes for reply-by-email, which >>>> has been mostly busted for a while) >>>> >>>> >>>> On Thu, Aug 30, 2012 at 6:58 PM, John Abd-El-Malek <jam@chromium.org>wrote: >>>> >>>>> test8 >>>>> >>>>> >>>>> On Thu, Aug 30, 2012 at 6:31 PM, John Abd-El-Malek <jam@chromium.org>wrote: >>>>> >>>>>> test 7 >>>>>> >>>>>> >>>>>> On Thu, Aug 30, 2012 at 6:24 PM, John Abd-El-Malek <jam@chromium.org>wrote: >>>>>> >>>>>>> test6 >>>>>>> >>>>>>> >>>>>>> On Thu, Aug 30, 2012 at 6:20 PM, John Abd-El-Malek <jam@chromium.org >>>>>>> > wrote: >>>>>>> >>>>>>>> test4 >>>>>>>> >>>>>>>> >>>>>>>> On Thu, Aug 30, 2012 at 5:44 PM, John Abd-El-Malek < >>>>>>>> jam@chromium.org> wrote: >>>>>>>> >>>>>>>>> (test again) >>>>>>>>> >>>>>>>>> >>>>>>>>> On Wed, Aug 29, 2012 at 2:23 PM, John Abd-El-Malek < >>>>>>>>> jam@chromium.org> wrote: >>>>>>>>> >>>>>>>>>> Done (also testing rietveld update by email) >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> On Wed, Aug 29, 2012 at 7:30 AM, Jói Sigurðsson <joi@chromium.org >>>>>>>>>> > wrote: >>>>>>>>>> >>>>>>>>>>> Doh! I started writing a long essay on why this was wrong, but >>>>>>>>>>> it's not. LGTM. >>>>>>>>>>> >>>>>>>>>>> The variable name was confusing before, and also the comment on >>>>>>>>>>> lines >>>>>>>>>>> 234-236 is a bit too wishy-washy, can you improve the comment to >>>>>>>>>>> say >>>>>>>>>>> "target thread" and "current thread" instead of "other thread" >>>>>>>>>>> and >>>>>>>>>>> "this one"? Maybe even add "since in that case the target >>>>>>>>>>> thread's >>>>>>>>>>> destructor, which clears its entry from globals.threads, cannot >>>>>>>>>>> run >>>>>>>>>>> while the current thread is running"? >>>>>>>>>>> >>>>>>>>>>> Cheers, >>>>>>>>>>> Jói >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> On Tue, Aug 28, 2012 at 11:09 PM, <jam@chromium.org> wrote: >>>>>>>>>>> > Reviewers: Jói, >>>>>>>>>>> > >>>>>>>>>>> > Description: >>>>>>>>>>> > Fix regression in BrowserThread's optimization of when it >>>>>>>>>>> skips a lock. The >>>>>>>>>>> > variable name was misnamed before, which contributed to this. >>>>>>>>>>> I've updated >>>>>>>>>>> > the >>>>>>>>>>> > variable name to make it clearer. >>>>>>>>>>> > >>>>>>>>>>> > Credit to liujundota@gmail.com who noticed this. >>>>>>>>>>> > >>>>>>>>>>> > Please review this at http://codereview.chromium.org/10900020/ >>>>>>>>>>> > >>>>>>>>>>> > SVN Base: svn://chrome-svn/chrome/trunk/src/ >>>>>>>>>>> > >>>>>>>>>>> > Affected files: >>>>>>>>>>> > M content/browser/browser_thread_impl.cc >>>>>>>>>>> > >>>>>>>>>>> > >>>>>>>>>>> > Index: content/browser/browser_thread_impl.cc >>>>>>>>>>> > >>>>>>>>>>> =================================================================== >>>>>>>>>>> > --- content/browser/browser_thread_impl.cc (revision >>>>>>>>>>> 153862) >>>>>>>>>>> > +++ content/browser/browser_thread_impl.cc (working copy) >>>>>>>>>>> > @@ -238,12 +238,12 @@ >>>>>>>>>>> > // which would require a lock because std::map isn't thread >>>>>>>>>>> safe, >>>>>>>>>>> > defeating >>>>>>>>>>> > // the whole purpose of this optimization. >>>>>>>>>>> > BrowserThread::ID current_thread; >>>>>>>>>>> > - bool guaranteed_to_outlive_target_thread = >>>>>>>>>>> > + bool target_thread_outlives_current = >>>>>>>>>>> > GetCurrentThreadIdentifier(¤t_thread) && >>>>>>>>>>> > - current_thread <= identifier; >>>>>>>>>>> > + current_thread >= identifier; >>>>>>>>>>> > >>>>>>>>>>> > BrowserThreadGlobals& globals = g_globals.Get(); >>>>>>>>>>> > - if (!guaranteed_to_outlive_target_thread) >>>>>>>>>>> > + if (!target_thread_outlives_current) >>>>>>>>>>> > globals.lock.Acquire(); >>>>>>>>>>> > >>>>>>>>>>> > MessageLoop* message_loop = globals.threads[identifier] ? >>>>>>>>>>> > @@ -256,7 +256,7 @@ >>>>>>>>>>> > } >>>>>>>>>>> > } >>>>>>>>>>> > >>>>>>>>>>> > - if (!guaranteed_to_outlive_target_thread) >>>>>>>>>>> > + if (!target_thread_outlives_current) >>>>>>>>>>> > globals.lock.Release(); >>>>>>>>>>> > >>>>>>>>>>> > return !!message_loop; >>>>>>>>>>> > >>>>>>>>>>> > >>>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>> >>>>>>>> >>>>>>> >>>>>> >>>>> >>>> >>> >> >
test10 On Fri, Aug 31, 2012 at 9:14 AM, John Abd-El-Malek <jam@chromium.org> wrote: > test9 > > > On Thu, Aug 30, 2012 at 7:11 PM, John Abd-El-Malek <jam@chromium.org>wrote: > >> (for those wondering, I'm testing out fixes for reply-by-email, which has >> been mostly busted for a while) >> >> >> On Thu, Aug 30, 2012 at 6:58 PM, John Abd-El-Malek <jam@chromium.org>wrote: >> >>> test8 >>> >>> >>> On Thu, Aug 30, 2012 at 6:31 PM, John Abd-El-Malek <jam@chromium.org>wrote: >>> >>>> test 7 >>>> >>>> >>>> On Thu, Aug 30, 2012 at 6:24 PM, John Abd-El-Malek <jam@chromium.org>wrote: >>>> >>>>> test6 >>>>> >>>>> >>>>> On Thu, Aug 30, 2012 at 6:20 PM, John Abd-El-Malek <jam@chromium.org>wrote: >>>>> >>>>>> test4 >>>>>> >>>>>> >>>>>> On Thu, Aug 30, 2012 at 5:44 PM, John Abd-El-Malek <jam@chromium.org>wrote: >>>>>> >>>>>>> (test again) >>>>>>> >>>>>>> >>>>>>> On Wed, Aug 29, 2012 at 2:23 PM, John Abd-El-Malek <jam@chromium.org >>>>>>> > wrote: >>>>>>> >>>>>>>> Done (also testing rietveld update by email) >>>>>>>> >>>>>>>> >>>>>>>> On Wed, Aug 29, 2012 at 7:30 AM, Jói Sigurðsson <joi@chromium.org>wrote: >>>>>>>> >>>>>>>>> Doh! I started writing a long essay on why this was wrong, but >>>>>>>>> it's not. LGTM. >>>>>>>>> >>>>>>>>> The variable name was confusing before, and also the comment on >>>>>>>>> lines >>>>>>>>> 234-236 is a bit too wishy-washy, can you improve the comment to >>>>>>>>> say >>>>>>>>> "target thread" and "current thread" instead of "other thread" and >>>>>>>>> "this one"? Maybe even add "since in that case the target thread's >>>>>>>>> destructor, which clears its entry from globals.threads, cannot run >>>>>>>>> while the current thread is running"? >>>>>>>>> >>>>>>>>> Cheers, >>>>>>>>> Jói >>>>>>>>> >>>>>>>>> >>>>>>>>> On Tue, Aug 28, 2012 at 11:09 PM, <jam@chromium.org> wrote: >>>>>>>>> > Reviewers: Jói, >>>>>>>>> > >>>>>>>>> > Description: >>>>>>>>> > Fix regression in BrowserThread's optimization of when it skips >>>>>>>>> a lock. The >>>>>>>>> > variable name was misnamed before, which contributed to this. >>>>>>>>> I've updated >>>>>>>>> > the >>>>>>>>> > variable name to make it clearer. >>>>>>>>> > >>>>>>>>> > Credit to liujundota@gmail.com who noticed this. >>>>>>>>> > >>>>>>>>> > Please review this at http://codereview.chromium.org/10900020/ >>>>>>>>> > >>>>>>>>> > SVN Base: svn://chrome-svn/chrome/trunk/src/ >>>>>>>>> > >>>>>>>>> > Affected files: >>>>>>>>> > M content/browser/browser_thread_impl.cc >>>>>>>>> > >>>>>>>>> > >>>>>>>>> > Index: content/browser/browser_thread_impl.cc >>>>>>>>> > >>>>>>>>> =================================================================== >>>>>>>>> > --- content/browser/browser_thread_impl.cc (revision 153862) >>>>>>>>> > +++ content/browser/browser_thread_impl.cc (working copy) >>>>>>>>> > @@ -238,12 +238,12 @@ >>>>>>>>> > // which would require a lock because std::map isn't thread >>>>>>>>> safe, >>>>>>>>> > defeating >>>>>>>>> > // the whole purpose of this optimization. >>>>>>>>> > BrowserThread::ID current_thread; >>>>>>>>> > - bool guaranteed_to_outlive_target_thread = >>>>>>>>> > + bool target_thread_outlives_current = >>>>>>>>> > GetCurrentThreadIdentifier(¤t_thread) && >>>>>>>>> > - current_thread <= identifier; >>>>>>>>> > + current_thread >= identifier; >>>>>>>>> > >>>>>>>>> > BrowserThreadGlobals& globals = g_globals.Get(); >>>>>>>>> > - if (!guaranteed_to_outlive_target_thread) >>>>>>>>> > + if (!target_thread_outlives_current) >>>>>>>>> > globals.lock.Acquire(); >>>>>>>>> > >>>>>>>>> > MessageLoop* message_loop = globals.threads[identifier] ? >>>>>>>>> > @@ -256,7 +256,7 @@ >>>>>>>>> > } >>>>>>>>> > } >>>>>>>>> > >>>>>>>>> > - if (!guaranteed_to_outlive_target_thread) >>>>>>>>> > + if (!target_thread_outlives_current) >>>>>>>>> > globals.lock.Release(); >>>>>>>>> > >>>>>>>>> > return !!message_loop; >>>>>>>>> > >>>>>>>>> > >>>>>>>>> >>>>>>>> >>>>>>>> >>>>>>> >>>>>> >>>>> >>>> >>> >> >
test12 On Wed, Oct 3, 2012 at 6:13 PM, John Abd-El-Malek <jam@chromium.org> wrote: > test10 > > > On Fri, Aug 31, 2012 at 9:14 AM, John Abd-El-Malek <jam@chromium.org>wrote: > >> test9 >> >> >> On Thu, Aug 30, 2012 at 7:11 PM, John Abd-El-Malek <jam@chromium.org>wrote: >> >>> (for those wondering, I'm testing out fixes for reply-by-email, which >>> has been mostly busted for a while) >>> >>> >>> On Thu, Aug 30, 2012 at 6:58 PM, John Abd-El-Malek <jam@chromium.org>wrote: >>> >>>> test8 >>>> >>>> >>>> On Thu, Aug 30, 2012 at 6:31 PM, John Abd-El-Malek <jam@chromium.org>wrote: >>>> >>>>> test 7 >>>>> >>>>> >>>>> On Thu, Aug 30, 2012 at 6:24 PM, John Abd-El-Malek <jam@chromium.org>wrote: >>>>> >>>>>> test6 >>>>>> >>>>>> >>>>>> On Thu, Aug 30, 2012 at 6:20 PM, John Abd-El-Malek <jam@chromium.org>wrote: >>>>>> >>>>>>> test4 >>>>>>> >>>>>>> >>>>>>> On Thu, Aug 30, 2012 at 5:44 PM, John Abd-El-Malek <jam@chromium.org >>>>>>> > wrote: >>>>>>> >>>>>>>> (test again) >>>>>>>> >>>>>>>> >>>>>>>> On Wed, Aug 29, 2012 at 2:23 PM, John Abd-El-Malek < >>>>>>>> jam@chromium.org> wrote: >>>>>>>> >>>>>>>>> Done (also testing rietveld update by email) >>>>>>>>> >>>>>>>>> >>>>>>>>> On Wed, Aug 29, 2012 at 7:30 AM, Jói Sigurðsson <joi@chromium.org>wrote: >>>>>>>>> >>>>>>>>>> Doh! I started writing a long essay on why this was wrong, but >>>>>>>>>> it's not. LGTM. >>>>>>>>>> >>>>>>>>>> The variable name was confusing before, and also the comment on >>>>>>>>>> lines >>>>>>>>>> 234-236 is a bit too wishy-washy, can you improve the comment to >>>>>>>>>> say >>>>>>>>>> "target thread" and "current thread" instead of "other thread" and >>>>>>>>>> "this one"? Maybe even add "since in that case the target >>>>>>>>>> thread's >>>>>>>>>> destructor, which clears its entry from globals.threads, cannot >>>>>>>>>> run >>>>>>>>>> while the current thread is running"? >>>>>>>>>> >>>>>>>>>> Cheers, >>>>>>>>>> Jói >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> On Tue, Aug 28, 2012 at 11:09 PM, <jam@chromium.org> wrote: >>>>>>>>>> > Reviewers: Jói, >>>>>>>>>> > >>>>>>>>>> > Description: >>>>>>>>>> > Fix regression in BrowserThread's optimization of when it skips >>>>>>>>>> a lock. The >>>>>>>>>> > variable name was misnamed before, which contributed to this. >>>>>>>>>> I've updated >>>>>>>>>> > the >>>>>>>>>> > variable name to make it clearer. >>>>>>>>>> > >>>>>>>>>> > Credit to liujundota@gmail.com who noticed this. >>>>>>>>>> > >>>>>>>>>> > Please review this at http://codereview.chromium.org/10900020/ >>>>>>>>>> > >>>>>>>>>> > SVN Base: svn://chrome-svn/chrome/trunk/src/ >>>>>>>>>> > >>>>>>>>>> > Affected files: >>>>>>>>>> > M content/browser/browser_thread_impl.cc >>>>>>>>>> > >>>>>>>>>> > >>>>>>>>>> > Index: content/browser/browser_thread_impl.cc >>>>>>>>>> > >>>>>>>>>> =================================================================== >>>>>>>>>> > --- content/browser/browser_thread_impl.cc (revision >>>>>>>>>> 153862) >>>>>>>>>> > +++ content/browser/browser_thread_impl.cc (working copy) >>>>>>>>>> > @@ -238,12 +238,12 @@ >>>>>>>>>> > // which would require a lock because std::map isn't thread >>>>>>>>>> safe, >>>>>>>>>> > defeating >>>>>>>>>> > // the whole purpose of this optimization. >>>>>>>>>> > BrowserThread::ID current_thread; >>>>>>>>>> > - bool guaranteed_to_outlive_target_thread = >>>>>>>>>> > + bool target_thread_outlives_current = >>>>>>>>>> > GetCurrentThreadIdentifier(¤t_thread) && >>>>>>>>>> > - current_thread <= identifier; >>>>>>>>>> > + current_thread >= identifier; >>>>>>>>>> > >>>>>>>>>> > BrowserThreadGlobals& globals = g_globals.Get(); >>>>>>>>>> > - if (!guaranteed_to_outlive_target_thread) >>>>>>>>>> > + if (!target_thread_outlives_current) >>>>>>>>>> > globals.lock.Acquire(); >>>>>>>>>> > >>>>>>>>>> > MessageLoop* message_loop = globals.threads[identifier] ? >>>>>>>>>> > @@ -256,7 +256,7 @@ >>>>>>>>>> > } >>>>>>>>>> > } >>>>>>>>>> > >>>>>>>>>> > - if (!guaranteed_to_outlive_target_thread) >>>>>>>>>> > + if (!target_thread_outlives_current) >>>>>>>>>> > globals.lock.Release(); >>>>>>>>>> > >>>>>>>>>> > return !!message_loop; >>>>>>>>>> > >>>>>>>>>> > >>>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>> >>>>>>> >>>>>> >>>>> >>>> >>> >> > |
||||||||||||||||||||||||||||||||||||||||||||||||||
