Add |joinable| to Thread::Options |
||
Issue descriptionRequired for MessageLoop backed non-joinable threads as discussed @ https://codereview.chromium.org/2103883004/#msg14 CL: https://codereview.chromium.org/2135413003/ Currently blocked on first cleaning up existing API with https://codereview.chromium.org/2145463002/ CC eventual reviewers FYI
,
Jul 21 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/8504bffa5efb122e7e5ac960b745a82b79efef39 commit 8504bffa5efb122e7e5ac960b745a82b79efef39 Author: gab <gab@chromium.org> Date: Thu Jul 21 13:42:57 2016 Remove unused base::Thread::thread_handle() getter. Fix only user (dbus/test_server.cc) to use Thread on stack which is automatically joined on destruction (and thus doesn't need the exposed handle it was using to manually join). BUG= 629716 Review-Url: https://codereview.chromium.org/2162293002 Cr-Commit-Position: refs/heads/master@{#406831} [modify] https://crrev.com/8504bffa5efb122e7e5ac960b745a82b79efef39/base/threading/thread.h [modify] https://crrev.com/8504bffa5efb122e7e5ac960b745a82b79efef39/dbus/test_server.cc
,
Jul 22 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/1734a80f1519819eb6a3d8eb43064f8fb2c2e067 commit 1734a80f1519819eb6a3d8eb43064f8fb2c2e067 Author: gab <gab@chromium.org> Date: Fri Jul 22 21:26:13 2016 Modernize base::Thread No-op cleanup extracted from https://codereview.chromium.org/2135413003/#ps40001 In this CL: - Explicitly document the threading requirements of the API after grokking its use of locks in some places and lack of in some others) - (It's mostly thread unsafe; except for a few calls which are okay from the owner as well as the underlying thread; and one method which is actually thread-safe) - Add assertions as such (and catch issues on try bots :-O -- http://crbug.com/629139) - Remove |thread_lock_| which is unnecessary under the documented conditions (delayed until http://crbug.com/629139 is fixed..). - C++11 member initializers (makes it cleaner to add new POD members and makes constructors simpler -- e.g. |run_loop_| was uninitialized) - Assertions and tests for current API which allows Start/Stop/Start cycles but wasn't tested... (actually more than that is poorly/not tested in this class but I'm merely adding tests for parts I'm stressing in my actual follow-up CL). - Comment nits - Make event-based tests use WaitableEvent instead of sleeping + toggling a bool. BUG= 629716 , 629139 Review-Url: https://codereview.chromium.org/2145463002 Cr-Commit-Position: refs/heads/master@{#407265} [modify] https://crrev.com/1734a80f1519819eb6a3d8eb43064f8fb2c2e067/base/threading/thread.cc [modify] https://crrev.com/1734a80f1519819eb6a3d8eb43064f8fb2c2e067/base/threading/thread.h [modify] https://crrev.com/1734a80f1519819eb6a3d8eb43064f8fb2c2e067/base/threading/thread_unittest.cc
,
Aug 1 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/d0cc00da87f2e1ccae783ad876848518b1c389f0 commit d0cc00da87f2e1ccae783ad876848518b1c389f0 Author: gab <gab@chromium.org> Date: Mon Aug 01 19:24:03 2016 Loosen/improve AtomicFlag semantics. In particular don't attach to construction thread, the requirement was I think trying to articulate that it doesn't make sense to call Set() from different threads as it's then not clear which thread IsSet() is memory consistent with. But: 1) What's required is *sequence* not *thread* checking. 2) Calls to Set() need to be sequenced w.r.t. to each other, but not with respect to construction (AtomicFlag is not modified before Set()). Update restrictions to reflect that and update comments to reflect the guarantees provided by them + its usage of atomics/barriers. Also expose UnsafeReset() outside of "testing", it will be used in https://codereview.chromium.org/2135413003/. BUG= 629716 Review-Url: https://codereview.chromium.org/2187333002 Cr-Commit-Position: refs/heads/master@{#409020} [modify] https://crrev.com/d0cc00da87f2e1ccae783ad876848518b1c389f0/base/synchronization/atomic_flag.cc [modify] https://crrev.com/d0cc00da87f2e1ccae783ad876848518b1c389f0/base/synchronization/atomic_flag.h [modify] https://crrev.com/d0cc00da87f2e1ccae783ad876848518b1c389f0/base/synchronization/atomic_flag_unittest.cc
,
Aug 3 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/9a473d5bfb65cffcb6576496232f66023af79fdf commit 9a473d5bfb65cffcb6576496232f66023af79fdf Author: gab <gab@chromium.org> Date: Wed Aug 03 19:43:50 2016 Add |joinable| to Thread::Options Required for MessageLoop backed non-joinable threads as discussed @ https://codereview.chromium.org/2103883004/#msg14 BUG= 622400 , 629716 , 629139 Review-Url: https://codereview.chromium.org/2135413003 Cr-Commit-Position: refs/heads/master@{#409597} [modify] https://crrev.com/9a473d5bfb65cffcb6576496232f66023af79fdf/base/threading/platform_thread.h [modify] https://crrev.com/9a473d5bfb65cffcb6576496232f66023af79fdf/base/threading/platform_thread_posix.cc [modify] https://crrev.com/9a473d5bfb65cffcb6576496232f66023af79fdf/base/threading/platform_thread_win.cc [modify] https://crrev.com/9a473d5bfb65cffcb6576496232f66023af79fdf/base/threading/thread.cc [modify] https://crrev.com/9a473d5bfb65cffcb6576496232f66023af79fdf/base/threading/thread.h [modify] https://crrev.com/9a473d5bfb65cffcb6576496232f66023af79fdf/base/threading/thread_unittest.cc [modify] https://crrev.com/9a473d5bfb65cffcb6576496232f66023af79fdf/content/browser/browser_thread_impl.cc [modify] https://crrev.com/9a473d5bfb65cffcb6576496232f66023af79fdf/ios/web/web_thread_impl.cc
,
Aug 4 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/8b35b0572443f4a84377a1fbd199a1edabe3d7d6 commit 8b35b0572443f4a84377a1fbd199a1edabe3d7d6 Author: gab <gab@chromium.org> Date: Thu Aug 04 17:59:56 2016 Add joinable option to SimpleThread::Options as was just done for Thread::Options. SimpleThread will be more suitable than Thread for https://codereview.chromium.org/2197753002/ per it not needing a MessageLoop. Also, cleanup the SimpleThread::Options API, making it more like Thread::Options. BUG= 629716 TBR=avi@chromium.org Review-Url: https://codereview.chromium.org/2204333003 Cr-Commit-Position: refs/heads/master@{#409830} [modify] https://crrev.com/8b35b0572443f4a84377a1fbd199a1edabe3d7d6/base/threading/simple_thread.cc [modify] https://crrev.com/8b35b0572443f4a84377a1fbd199a1edabe3d7d6/base/threading/simple_thread.h [modify] https://crrev.com/8b35b0572443f4a84377a1fbd199a1edabe3d7d6/base/threading/simple_thread_unittest.cc [modify] https://crrev.com/8b35b0572443f4a84377a1fbd199a1edabe3d7d6/content/renderer/categorized_worker_pool.cc
,
Aug 4 2016
,
Aug 4 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/8b35b0572443f4a84377a1fbd199a1edabe3d7d6 commit 8b35b0572443f4a84377a1fbd199a1edabe3d7d6 Author: gab <gab@chromium.org> Date: Thu Aug 04 17:59:56 2016 Add joinable option to SimpleThread::Options as was just done for Thread::Options. SimpleThread will be more suitable than Thread for https://codereview.chromium.org/2197753002/ per it not needing a MessageLoop. Also, cleanup the SimpleThread::Options API, making it more like Thread::Options. BUG= 629716 TBR=avi@chromium.org Review-Url: https://codereview.chromium.org/2204333003 Cr-Commit-Position: refs/heads/master@{#409830} [modify] https://crrev.com/8b35b0572443f4a84377a1fbd199a1edabe3d7d6/base/threading/simple_thread.cc [modify] https://crrev.com/8b35b0572443f4a84377a1fbd199a1edabe3d7d6/base/threading/simple_thread.h [modify] https://crrev.com/8b35b0572443f4a84377a1fbd199a1edabe3d7d6/base/threading/simple_thread_unittest.cc [modify] https://crrev.com/8b35b0572443f4a84377a1fbd199a1edabe3d7d6/content/renderer/categorized_worker_pool.cc
,
Aug 5 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/4ef71eefd26cd7c4f3ee5c7d4fbff9aaed2a5459 commit 4ef71eefd26cd7c4f3ee5c7d4fbff9aaed2a5459 Author: reillyg <reillyg@chromium.org> Date: Fri Aug 05 20:54:50 2016 Fix TSAN suppressions for non-joinable ThreadTest. The compiler does not include the class name in the symbol name for the TestBody method so the suppression added in r409915 doesn't work. This patch disables the test under TSan instead. BUG= 634383 , 629716 TBR=thestig@chromium.org NOTRY=true Review-Url: https://codereview.chromium.org/2218663003 Cr-Commit-Position: refs/heads/master@{#410159} [modify] https://crrev.com/4ef71eefd26cd7c4f3ee5c7d4fbff9aaed2a5459/base/threading/thread_unittest.cc [modify] https://crrev.com/4ef71eefd26cd7c4f3ee5c7d4fbff9aaed2a5459/build/sanitizers/tsan_suppressions.cc
,
Aug 5 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a9094bdb2cda679e8bf3a5b43b27c12efa975d3c commit a9094bdb2cda679e8bf3a5b43b27c12efa975d3c Author: justincohen <justincohen@chromium.org> Date: Fri Aug 05 21:43:45 2016 Revert of Add joinable option to SimpleThread::Options as was just done for Thread. (patchset #2 id:20001 of https://codereview.chromium.org/2204333003/ ) Reason for revert: Reverting this since it is breaking the iOS base_unittests. gba@ is working on a fix in https://codereview.chromium.org/2218983003/ Original issue's description: > Add joinable option to SimpleThread::Options as was just done for Thread::Options. > > SimpleThread will be more suitable than Thread for > https://codereview.chromium.org/2197753002/ per it not needing a MessageLoop. > > Also, cleanup the SimpleThread::Options API, making it more like Thread::Options. > > BUG= 629716 > TBR=avi@chromium.org > > Committed: https://crrev.com/8b35b0572443f4a84377a1fbd199a1edabe3d7d6 > Cr-Commit-Position: refs/heads/master@{#409830} TBR=thestig@chromium.org,avi@chromium.org,gab@chromium.org BUG= 629716 Review-Url: https://codereview.chromium.org/2219663006 Cr-Commit-Position: refs/heads/master@{#410169} [modify] https://crrev.com/a9094bdb2cda679e8bf3a5b43b27c12efa975d3c/base/threading/simple_thread.cc [modify] https://crrev.com/a9094bdb2cda679e8bf3a5b43b27c12efa975d3c/base/threading/simple_thread.h [modify] https://crrev.com/a9094bdb2cda679e8bf3a5b43b27c12efa975d3c/base/threading/simple_thread_unittest.cc [modify] https://crrev.com/a9094bdb2cda679e8bf3a5b43b27c12efa975d3c/content/renderer/categorized_worker_pool.cc
,
Aug 13 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/addc8ef1bcae5bff546fba73eca8bea3e640ebcd commit addc8ef1bcae5bff546fba73eca8bea3e640ebcd Author: gab <gab@chromium.org> Date: Sat Aug 13 17:28:50 2016 Add joinable option to SimpleThread::Options as was just done for Thread::Options. SimpleThread will be more suitable than Thread for https://codereview.chromium.org/2197753002/ per it not needing a MessageLoop. Also, cleanup the SimpleThread::Options API, making it more like Thread::Options. Non-joinable SimpleThreads will have to remain alive so long as their Run() method uses member state (SimpleThread itself won't after invoking Run()). DelegateSimpleThread provides a safe way to do this by explicitly supporting being deleted while Run() is active but ensuring the Delegate itself outlives Run()). Note: it might seem overkill to let DelegateSimpleThread be deleted early but this is eventually what I want to do for base::Thread (https://crbug.com/629139#c14) and this is a good precursor to this paradigm (it really takes all its sense on base::Thread where the destructor is more meaningful though -- winds down the MessageLoop). The logic on SimpleThread is fairly simple, the complexity is in the assertions (which were put behind DCHECK_IS_ON() to clearly isolate them) and the tests. BUG= 629716 , 634835, 629139 TBR=avi@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_tsan_rel_ng Committed: https://crrev.com/8b35b0572443f4a84377a1fbd199a1edabe3d7d6 Cr-Commit-Position: refs/heads/master@{#409830} Reverted: https://crrev.com/a9094bdb2cda679e8bf3a5b43b27c12efa975d3c Review-Url: https://codereview.chromium.org/2204333003 Cr-Original-Commit-Position: refs/heads/master@{#410169} Cr-Commit-Position: refs/heads/master@{#411897} [modify] https://crrev.com/addc8ef1bcae5bff546fba73eca8bea3e640ebcd/base/threading/simple_thread.cc [modify] https://crrev.com/addc8ef1bcae5bff546fba73eca8bea3e640ebcd/base/threading/simple_thread.h [modify] https://crrev.com/addc8ef1bcae5bff546fba73eca8bea3e640ebcd/base/threading/simple_thread_unittest.cc [modify] https://crrev.com/addc8ef1bcae5bff546fba73eca8bea3e640ebcd/content/renderer/categorized_worker_pool.cc |
||
►
Sign in to add a comment |
||
Comment 1 by gab@chromium.org
, Jul 20 2016