Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(260)

Issue 2664953004: Do not block in SimpleThread::Start(). (Closed)

Created:
3 years, 10 months ago by fdoray
Modified:
3 years, 10 months ago
CC:
chromium-reviews, mlamouri+watch-content_chromium.org, vmpstr+watch_chromium.org, jam, feature-media-reviews_chromium.org, darin-cc_chromium.org, kinuko+watch, blink-worker-reviews_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Do not block in SimpleThread::Start(). There is no good reason to block in SimpleThread::Start(). In addition to introducing unnecessary latency, blocking in Start() prevents a non-joinable DelegateSimpleThread from being deleted from Run() without external synchronization (it is wrong to delete SimpleThread while Start() is waiting on the |event_| member). BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel Review-Url: https://codereview.chromium.org/2664953004 Cr-Commit-Position: refs/heads/master@{#448622} Committed: https://chromium.googlesource.com/chromium/src/+/8ad2f4f9bb3d55974ea1707ff4186bbe321ea499

Patch Set 1 #

Patch Set 2 : fix build error #

Patch Set 3 : GetTid() #

Patch Set 4 : HasBeenStarted() #

Total comments: 16

Patch Set 5 : CR gab #21 #

Total comments: 2

Patch Set 6 : CR gab #26 (comment) #

Unified diffs Side-by-side diffs Delta from patch set Stats (+64 lines, -65 lines) Patch
M base/threading/sequenced_worker_pool.cc View 1 chunk +1 line, -1 line 0 comments Download
M base/threading/simple_thread.h View 1 2 3 4 4 chunks +19 lines, -15 lines 0 comments Download
M base/threading/simple_thread.cc View 1 2 3 4 5 2 chunks +28 lines, -16 lines 0 comments Download
M base/threading/simple_thread_unittest.cc View 2 chunks +2 lines, -13 lines 0 comments Download
M base/threading/thread_restrictions.h View 1 2 3 4 2 chunks +0 lines, -2 lines 0 comments Download
M base/win/wait_chain_unittest.cc View 1 2 3 chunks +3 lines, -3 lines 0 comments Download
M content/renderer/categorized_worker_pool.h View 1 2 3 4 1 chunk +2 lines, -3 lines 0 comments Download
M content/renderer/categorized_worker_pool.cc View 1 chunk +5 lines, -0 lines 0 comments Download
M content/renderer/render_thread_impl.cc View 1 2 3 4 1 chunk +1 line, -1 line 0 comments Download
M media/audio/win/audio_low_latency_output_win.cc View 1 2 3 2 chunks +3 lines, -11 lines 0 comments Download

Messages

Total messages: 42 (28 generated)
fdoray
ptal
3 years, 10 months ago (2017-02-02 19:58:50 UTC) #18
gab
lgtm w/ comments https://codereview.chromium.org/2664953004/diff/60001/base/threading/simple_thread.cc File base/threading/simple_thread.cc (left): https://codereview.chromium.org/2664953004/diff/60001/base/threading/simple_thread.cc#oldcode39 base/threading/simple_thread.cc:39: ThreadRestrictions::ScopedAllowWait allow_wait; Remove friend class SimpleThread; ...
3 years, 10 months ago (2017-02-06 20:29:11 UTC) #21
fdoray
jam@: Please review changes in content/ henrika@: Please review changes in media/audio/win/ https://codereview.chromium.org/2664953004/diff/60001/base/threading/simple_thread.cc File base/threading/simple_thread.cc ...
3 years, 10 months ago (2017-02-06 20:46:49 UTC) #25
gab
still lgtm w/ nit https://codereview.chromium.org/2664953004/diff/80001/base/threading/simple_thread.cc File base/threading/simple_thread.cc (right): https://codereview.chromium.org/2664953004/diff/80001/base/threading/simple_thread.cc#newcode37 base/threading/simple_thread.cc:37: // Otherwise, Run() could hit ...
3 years, 10 months ago (2017-02-06 21:19:28 UTC) #26
jam
lgtm
3 years, 10 months ago (2017-02-07 01:23:23 UTC) #29
henrika (OOO until Aug 14)
lgtm
3 years, 10 months ago (2017-02-07 09:10:45 UTC) #30
fdoray
https://codereview.chromium.org/2664953004/diff/80001/base/threading/simple_thread.cc File base/threading/simple_thread.cc (right): https://codereview.chromium.org/2664953004/diff/80001/base/threading/simple_thread.cc#newcode37 base/threading/simple_thread.cc:37: // Otherwise, Run() could hit a DCHECK when deleting ...
3 years, 10 months ago (2017-02-07 12:57:17 UTC) #31
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2664953004/100001
3 years, 10 months ago (2017-02-07 12:57:39 UTC) #34
commit-bot: I haz the power
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/8ad2f4f9bb3d55974ea1707ff4186bbe321ea499
3 years, 10 months ago (2017-02-07 15:03:19 UTC) #37
battre
On 2017/02/07 15:03:19, commit-bot: I haz the power wrote: > Committed patchset #6 (id:100001) as ...
3 years, 10 months ago (2017-02-07 17:38:55 UTC) #38
battre
A revert of this CL (patchset #6 id:100001) has been created in https://codereview.chromium.org/2685533003/ by battre@chromium.org. ...
3 years, 10 months ago (2017-02-07 17:39:44 UTC) #39
gab
On 2017/02/07 17:39:44, battre (OOO) wrote: > A revert of this CL (patchset #6 id:100001) ...
3 years, 10 months ago (2017-02-07 18:34:53 UTC) #40
gab
On 2017/02/07 18:34:53, gab wrote: > On 2017/02/07 17:39:44, battre (OOO) wrote: > > A ...
3 years, 10 months ago (2017-02-07 19:48:15 UTC) #41
gab
3 years, 10 months ago (2017-02-07 20:34:53 UTC) #42
Message was sent while issue was closed.
On 2017/02/07 19:48:15, gab wrote:
> On 2017/02/07 18:34:53, gab wrote:
> > On 2017/02/07 17:39:44, battre (OOO) wrote:
> > > A revert of this CL (patchset #6 id:100001) has been created in
> > > https://codereview.chromium.org/2685533003/ by
> > https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=battre@chromium.org.
> > > 
> > > The reason for reverting is: Attempt to fix iOS Simulator issues with
> > >
> >
>
SkipOnShutdown/TaskSchedulerTaskTrackerTest.WillPostAndRunLongTaskBeforeShutdown/0.
> > 
> > FYI, I think this might also be at cause for Linux Tests failures:
> >
>
https://uberchromegw.corp.google.com/i/chromium.linux/builders/Linux%20Tests%...
> > 
> > (hangs in content_browsertests failed interactive_ui_tests failed
> > sync_integration_tests with only two blamed CLs including this one:
> >
>
https://chromium.googlesource.com/chromium/src/+log/d68b4a106ecc41b8b16079a30...)
> 
> Tests pass again after revert so clearly at fault..

Hangs also on Linux ChromiumOS Tests:
https://uberchromegw.corp.google.com/i/chromium.chromiumos/builders/Linux%20C...

Powered by Google App Engine
This is Rietveld 408576698