Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1150563002:
base/threading: Lazy thread startup for pthreads (Closed)
|
Created:
5 years, 7 months ago by Takashi Toyoshima Modified:
5 years, 4 months ago CC:
chromium-reviews, piman+watch_chromium.org, erikwright+watch_chromium.org, kinuko Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionbase/threading: Lazy thread startup for pthreads
r331235 made it possible to initialize MessageLoop lazily.
But, on platforms that depends on pthreads, thread creation
still blocks until the newly created thread has started.
This patch stop waiting for the new thread starts. This
got to be possible because I removed thread ID from
PlatformThreadHandle. (crbug.com/468793)
BUG=495097
Committed: https://crrev.com/f6a0438a244d491682cdf10d612374e6d3f36dbc
Cr-Commit-Position: refs/heads/master@{#340746}
Patch Set 1 : try #Patch Set 2 : fix win/mac build errors #Patch Set 3 : retry #Patch Set 4 : v2 (based on ToT for try) #Patch Set 5 : (rebase to show diffs to 1207823004) #Patch Set 6 : rebase, and do not use private handle_ #
Total comments: 4
Patch Set 7 : review #8, #9 #
Total comments: 1
Patch Set 8 : rebased #
Total comments: 6
Patch Set 9 : rebase and review #15 #
Total comments: 1
Messages
Total messages: 24 (8 generated)
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
Patchset #1 (id:40001) has been deleted
Still have problems. I will split some trivial cleanup parts to land it separately.
Patchset #5 (id:140001) has been deleted
toyoshim@chromium.org changed reviewers: + kinuko@chromium.org
Now it's getting to be a easy fix after series of changes for base/threading. Still this depends on unlanded CLs, https://codereview.chromium.org/1193303002/ and https://codereview.chromium.org/1207823004/.
https://codereview.chromium.org/1150563002/diff/180001/base/threading/platfor... File base/threading/platform_thread_posix.cc (right): https://codereview.chromium.org/1150563002/diff/180001/base/threading/platfor... base/threading/platform_thread_posix.cc:109: // Value of |thread_handle| is undefined if pthread_create fails. Oops, self review note: s/thread_handle/handle/.
https://codereview.chromium.org/1150563002/diff/180001/base/threading/platfor... File base/threading/platform_thread_posix.cc (right): https://codereview.chromium.org/1150563002/diff/180001/base/threading/platfor... base/threading/platform_thread_posix.cc:107: ALLOW_UNUSED_LOCAL(unused_params); nit: ignore_result(param.release()) would work
https://codereview.chromium.org/1150563002/diff/180001/base/threading/platfor... File base/threading/platform_thread_posix.cc (right): https://codereview.chromium.org/1150563002/diff/180001/base/threading/platfor... base/threading/platform_thread_posix.cc:107: ALLOW_UNUSED_LOCAL(unused_params); On 2015/07/02 14:23:35, kinuko wrote: > nit: ignore_result(param.release()) would work Done. https://codereview.chromium.org/1150563002/diff/180001/base/threading/platfor... base/threading/platform_thread_posix.cc:109: // Value of |thread_handle| is undefined if pthread_create fails. On 2015/07/02 13:57:50, Takashi Toyoshima (chromium) wrote: > Oops, self review note: s/thread_handle/handle/. Done.
Note from reviews for https://chromiumcodereview.appspot.com/1207823004/ https://chromiumcodereview.appspot.com/1150563002/diff/200001/base/threading/... File base/threading/platform_thread_posix.cc (right): https://chromiumcodereview.appspot.com/1150563002/diff/200001/base/threading/... base/threading/platform_thread_posix.cc:191: base::ThreadRestrictions::ScopedAllowWait allow_wait; Note: remove allow_wait, and add janky measurement code into Thread::thread_id().
Now all dependent patches were submitted. PTAL Patch Set 8? I will submit https://codereview.chromium.org/1259603002/ firstly to measure performance differences before and after this patch.
toyoshim@chromium.org changed reviewers: + thestig@chromium.org
+thestig for base/ OWNERS review.
https://chromiumcodereview.appspot.com/1150563002/diff/220001/base/threading/... File base/threading/platform_thread_posix.cc (left): https://chromiumcodereview.appspot.com/1150563002/diff/220001/base/threading/... base/threading/platform_thread_posix.cc:50: WaitableEvent handle_set; I think you can remove #include base/synchronization/waitable_event.h after this. https://chromiumcodereview.appspot.com/1150563002/diff/220001/base/threading/... File base/threading/platform_thread_posix.cc (right): https://chromiumcodereview.appspot.com/1150563002/diff/220001/base/threading/... base/threading/platform_thread_posix.cc:47: CHECK(params); Is this needed? If the CHECK() fails, we'll just crash a few lines down anyway and know something is wrong. https://chromiumcodereview.appspot.com/1150563002/diff/220001/base/threading/... base/threading/platform_thread_posix.cc:77: CHECK(thread_handle); Will a DCHECK() suffice here?
Patchset #9 (id:240001) has been deleted
PTAL https://codereview.chromium.org/1150563002/diff/220001/base/threading/platfor... File base/threading/platform_thread_posix.cc (left): https://codereview.chromium.org/1150563002/diff/220001/base/threading/platfor... base/threading/platform_thread_posix.cc:50: WaitableEvent handle_set; On 2015/07/28 06:32:55, Lei Zhang wrote: > I think you can remove #include base/synchronization/waitable_event.h after > this. Done. https://codereview.chromium.org/1150563002/diff/220001/base/threading/platfor... File base/threading/platform_thread_posix.cc (right): https://codereview.chromium.org/1150563002/diff/220001/base/threading/platfor... base/threading/platform_thread_posix.cc:47: CHECK(params); Agreed. I removed this redundant check. https://codereview.chromium.org/1150563002/diff/220001/base/threading/platfor... base/threading/platform_thread_posix.cc:77: CHECK(thread_handle); Agreed. This check should be safe to ignore in release builds.
The CQ bit was checked by thestig@chromium.org
lgtm
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1150563002/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1150563002/260001
Message was sent while issue was closed.
Committed patchset #9 (id:260001)
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/f6a0438a244d491682cdf10d612374e6d3f36dbc Cr-Commit-Position: refs/heads/master@{#340746}
Message was sent while issue was closed.
https://codereview.chromium.org/1150563002/diff/260001/base/threading/platfor... File base/threading/platform_thread_posix.cc (right): https://codereview.chromium.org/1150563002/diff/260001/base/threading/platfor... base/threading/platform_thread_posix.cc:46: scoped_ptr<ThreadParams> thread_params(static_cast<ThreadParams*>(params)); Just curious - do we support calling pthread_exit()? Even if not, I would play safe and copy params to a local / delete params before doing anything else.
Message was sent while issue was closed.
On 2015/07/28 21:33:41, Dmitry Skiba wrote: > https://codereview.chromium.org/1150563002/diff/260001/base/threading/platfor... > File base/threading/platform_thread_posix.cc (right): > > https://codereview.chromium.org/1150563002/diff/260001/base/threading/platfor... > base/threading/platform_thread_posix.cc:46: scoped_ptr<ThreadParams> > thread_params(static_cast<ThreadParams*>(params)); > Just curious - do we support calling pthread_exit()? > Even if not, I would play safe and copy params to a local / delete params before > doing anything else. Is your concern that some thread may call pthread_exit(), and cause the scoped_ptr to fail to delete |params| on the newly created thread? BTW, I'm seeing an issue with Valgrind here: https://code.google.com/p/chromium/issues/detail?id=514868 |
