Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1006933003:
Add full SetThreadPriority support to Windows. (Closed)
|
Created:
5 years, 9 months ago by gab Modified:
5 years, 8 months ago Reviewers:
rvargas (doing something else) CC:
chromium-reviews, erikwright+watch_chromium.org, Lei Zhang Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd full SetThreadPriority support to Windows.
Also introducing the cross-platform GetThreadPriority() method to allow for
cross-platform tests for this code.
Enhanced PlatformThreadTests.
Discovering issue 468793 in the process where it appears that the POSIX priority
manipulation code doesn't actually work in practice...
BUG=456903, 468793, 338961, 468932
Committed: https://crrev.com/f1921cb37b83bce16c07ca93975cded2b2863dd0
Cr-Commit-Position: refs/heads/master@{#323266}
Patch Set 1 #
Total comments: 54
Patch Set 2 : merge up to r322239 #Patch Set 3 : review:rvargas #
Total comments: 2
Patch Set 4 : review:rvargas #Patch Set 5 : Better PlatformThreadTests -- fixing some races tripped by ASAN #Patch Set 6 : compile nits #Patch Set 7 : slight logic order tweak #Patch Set 8 : fix test exit race #
Total comments: 7
Patch Set 9 : nits #
Total comments: 2
Patch Set 10 : tweaks #
Messages
Total messages: 38 (18 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
Patchset #1 (id:80001) has been deleted
Patchset #1 (id:100001) has been deleted
Patchset #1 (id:120001) has been deleted
Patchset #1 (id:140001) has been deleted
Patchset #1 (id:160001) has been deleted
Patchset #1 (id:180001) has been deleted
gab@chromium.org changed reviewers: + rvargas@chromium.org
Ricardo PTAL, this is based on https://codereview.chromium.org/1015503002/ which you just reviewed. CC thestig because this implements part of issue 338961. Regarding just discovered issue 468793, I plan to find owners to look into the POSIX issues once this CL lands. Thanks! Gab
https://codereview.chromium.org/1006933003/diff/200001/base/threading/platfor... File base/threading/platform_thread_win.cc (right): https://codereview.chromium.org/1006933003/diff/200001/base/threading/platfor... base/threading/platform_thread_win.cc:270: success = !!::SetThreadPriority(handle.handle_, THREAD_PRIORITY_LOWEST); One question, do you think we should even allow background mode to be set from other threads or should we instead DCHECK here to make sure callers know to use the path resulting in the best behaviour (for now this is only mentioned in the API, but not enforced).
https://codereview.chromium.org/1006933003/diff/200001/base/threading/platfor... File base/threading/platform_thread.h (right): https://codereview.chromium.org/1006933003/diff/200001/base/threading/platfor... base/threading/platform_thread.h:115: kThreadPriority_Normal, Let's order this according to priority. https://codereview.chromium.org/1006933003/diff/200001/base/threading/platfor... base/threading/platform_thread.h:122: // platforms if this mode is set from the current thread. nit: "if this mode"? Do you mean this priority? https://codereview.chromium.org/1006933003/diff/200001/base/threading/platfor... File base/threading/platform_thread_freebsd.cc (right): https://codereview.chromium.org/1006933003/diff/200001/base/threading/platfor... base/threading/platform_thread_freebsd.cc:52: // Mirrors HandleSetThreadPriorityForPlatform()'s implementation. Do we need this comment? https://codereview.chromium.org/1006933003/diff/200001/base/threading/platfor... base/threading/platform_thread_freebsd.cc:55: if (pthread_getschedparam(pthread_self(), &maybe_sched_rr, why self() and not handle? https://codereview.chromium.org/1006933003/diff/200001/base/threading/platfor... File base/threading/platform_thread_internal_posix.h (right): https://codereview.chromium.org/1006933003/diff/200001/base/threading/platfor... base/threading/platform_thread_internal_posix.h:35: // Mirrors HandleSetThreadPriorityForPlatform(). Returns true if there is a I think we should remove the first sentence. Just document what the fn does. https://codereview.chromium.org/1006933003/diff/200001/base/threading/platfor... base/threading/platform_thread_internal_posix.h:39: bool HandleGetThreadPriorityForPlatform(PlatformThreadHandle handle, I know I just said that this was ok, but reading this CL the HandleXX is confusing (because we also receive a handle as argument). I think we should remove that prefix and have Set/GetThreadPriorityForPlatform() https://codereview.chromium.org/1006933003/diff/200001/base/threading/platfor... File base/threading/platform_thread_posix.cc (right): https://codereview.chromium.org/1006933003/diff/200001/base/threading/platfor... base/threading/platform_thread_posix.cc:282: ScopedClearErrno clear_errno; errno = 0 instead? https://codereview.chromium.org/1006933003/diff/200001/base/threading/platfor... File base/threading/platform_thread_win.cc (right): https://codereview.chromium.org/1006933003/diff/200001/base/threading/platfor... base/threading/platform_thread_win.cc:139: return PlatformThreadHandle(::GetCurrentThread()); Sounds like it will leak. https://codereview.chromium.org/1006933003/diff/200001/base/threading/platfor... base/threading/platform_thread_win.cc:242: bool unset = BOOL? https://codereview.chromium.org/1006933003/diff/200001/base/threading/platfor... base/threading/platform_thread_win.cc:242: bool unset = BOOL? https://codereview.chromium.org/1006933003/diff/200001/base/threading/platfor... base/threading/platform_thread_win.cc:243: !!::SetThreadPriority(handle.handle_, THREAD_MODE_BACKGROUND_END); ... I really hate !! to convert BOOL to bool https://codereview.chromium.org/1006933003/diff/200001/base/threading/platfor... base/threading/platform_thread_win.cc:267: success = This is problematic as this is a non-reentrant property. Calling SetPriority(background) twice is not forbidden by our API (and I don't really think it should be). https://codereview.chromium.org/1006933003/diff/200001/base/threading/platfor... base/threading/platform_thread_win.cc:270: success = !!::SetThreadPriority(handle.handle_, THREAD_PRIORITY_LOWEST); On 2015/03/19 18:06:07, gab wrote: > One question, do you think we should even allow background mode to be set from > other threads or should we instead DCHECK here to make sure callers know to use > the path resulting in the best behaviour (for now this is only mentioned in the > API, but not enforced). We should either allow any priority to be set from anywhere, or document that priorities should only be set from the same thread. https://codereview.chromium.org/1006933003/diff/200001/base/threading/platfor... base/threading/platform_thread_win.cc:277: DPCHECK(success); Should not check on a OS function return (and elsewhere) https://codereview.chromium.org/1006933003/diff/200001/base/threading/platfor... base/threading/platform_thread_win.cc:286: // The only way to test for background mode is to explicitly try to exit it Why not always set the priority to lowest (any os) and ignore this state in this function?
Hey Ricardo, sorry about the delay, I stripped out the true background mode handling on Windows for now to flush out the architectural changes and we can revisit it in a follow-up CL. Cheers, Gab https://codereview.chromium.org/1006933003/diff/200001/base/threading/platfor... File base/threading/platform_thread.h (right): https://codereview.chromium.org/1006933003/diff/200001/base/threading/platfor... base/threading/platform_thread.h:115: kThreadPriority_Normal, On 2015/03/19 22:19:21, rvargas (slow to respond) wrote: > Let's order this according to priority. I was thinking about that too, but I was thinking we should probably also make this an "enum class" (since we toggle it back-and-forth with the OS priority expressed as an integer and can easily mix the two otherwise) -- feels like this belongs in a follow-up (or precursor CL), WDYT? https://codereview.chromium.org/1006933003/diff/200001/base/threading/platfor... base/threading/platform_thread.h:122: // platforms if this mode is set from the current thread. On 2015/03/19 22:19:21, rvargas (slow to respond) wrote: > nit: "if this mode"? Do you mean this priority? Done. https://codereview.chromium.org/1006933003/diff/200001/base/threading/platfor... File base/threading/platform_thread_freebsd.cc (right): https://codereview.chromium.org/1006933003/diff/200001/base/threading/platfor... base/threading/platform_thread_freebsd.cc:52: // Mirrors HandleSetThreadPriorityForPlatform()'s implementation. On 2015/03/19 22:19:21, rvargas (slow to respond) wrote: > Do we need this comment? I like it, if anything it answers your question below :-) https://codereview.chromium.org/1006933003/diff/200001/base/threading/platfor... base/threading/platform_thread_freebsd.cc:55: if (pthread_getschedparam(pthread_self(), &maybe_sched_rr, On 2015/03/19 22:19:21, rvargas (slow to respond) wrote: > why self() and not handle? Because it mirrors HandleSetThreadPriorityForPlatform()'s implementation. I agree that this looks broken (or at least doesn't respect the existing API -- filed http://crbug.com/468793 for someoen to investigate POSIX thread priority once this lands) https://codereview.chromium.org/1006933003/diff/200001/base/threading/platfor... File base/threading/platform_thread_internal_posix.h (right): https://codereview.chromium.org/1006933003/diff/200001/base/threading/platfor... base/threading/platform_thread_internal_posix.h:35: // Mirrors HandleSetThreadPriorityForPlatform(). Returns true if there is a On 2015/03/19 22:19:21, rvargas (slow to respond) wrote: > I think we should remove the first sentence. Just document what the fn does. Done. https://codereview.chromium.org/1006933003/diff/200001/base/threading/platfor... base/threading/platform_thread_internal_posix.h:39: bool HandleGetThreadPriorityForPlatform(PlatformThreadHandle handle, On 2015/03/19 22:19:21, rvargas (slow to respond) wrote: > I know I just said that this was ok, but reading this CL the HandleXX is > confusing (because we also receive a handle as argument). I think we should > remove that prefix and have Set/GetThreadPriorityForPlatform() Done. https://codereview.chromium.org/1006933003/diff/200001/base/threading/platfor... File base/threading/platform_thread_posix.cc (right): https://codereview.chromium.org/1006933003/diff/200001/base/threading/platfor... base/threading/platform_thread_posix.cc:282: ScopedClearErrno clear_errno; On 2015/03/19 22:19:21, rvargas (slow to respond) wrote: > errno = 0 instead? I don't know much about Chromium POSIX development, assumed that it was perhaps prefered not to clobber errno here as a base/ citizen, but I don't care either way, up to you. https://codereview.chromium.org/1006933003/diff/200001/base/threading/platfor... File base/threading/platform_thread_win.cc (right): https://codereview.chromium.org/1006933003/diff/200001/base/threading/platfor... base/threading/platform_thread_win.cc:139: return PlatformThreadHandle(::GetCurrentThread()); On 2015/03/19 22:19:21, rvargas (slow to respond) wrote: > Sounds like it will leak. ::GetCurrentThread() returns a pseudo-handle which isn't leakable AFAIK. https://codereview.chromium.org/1006933003/diff/200001/base/threading/platfor... base/threading/platform_thread_win.cc:242: bool unset = On 2015/03/19 22:19:21, rvargas (slow to respond) wrote: > BOOL? Done. https://codereview.chromium.org/1006933003/diff/200001/base/threading/platfor... base/threading/platform_thread_win.cc:242: bool unset = On 2015/03/19 22:19:21, rvargas (slow to respond) wrote: > BOOL? Done. https://codereview.chromium.org/1006933003/diff/200001/base/threading/platfor... base/threading/platform_thread_win.cc:243: !!::SetThreadPriority(handle.handle_, THREAD_MODE_BACKGROUND_END); On 2015/03/19 22:19:21, rvargas (slow to respond) wrote: > ... I really hate !! to convert BOOL to bool Done. https://codereview.chromium.org/1006933003/diff/200001/base/threading/platfor... base/threading/platform_thread_win.cc:267: success = On 2015/03/19 22:19:21, rvargas (slow to respond) wrote: > This is problematic as this is a non-reentrant property. > > Calling SetPriority(background) twice is not forbidden by our API (and I don't > really think it should be). Good point, fixed. https://codereview.chromium.org/1006933003/diff/200001/base/threading/platfor... base/threading/platform_thread_win.cc:270: success = !!::SetThreadPriority(handle.handle_, THREAD_PRIORITY_LOWEST); On 2015/03/19 22:19:21, rvargas (slow to respond) wrote: > On 2015/03/19 18:06:07, gab wrote: > > One question, do you think we should even allow background mode to be set from > > other threads or should we instead DCHECK here to make sure callers know to > use > > the path resulting in the best behaviour (for now this is only mentioned in > the > > API, but not enforced). > > We should either allow any priority to be set from anywhere, or document that > priorities should only be set from the same thread. Ok, for now going with the simple change to get the architecture in, will consider a more advanced background in a follow-up. https://codereview.chromium.org/1006933003/diff/200001/base/threading/platfor... base/threading/platform_thread_win.cc:277: DPCHECK(success); On 2015/03/19 22:19:21, rvargas (slow to respond) wrote: > Should not check on a OS function return (and elsewhere) To me the goal of a debug check is to find out if this doesn't behave as expected in some configurations (rather than silently fail) on some bots or some dev machines -- or even on new OS versions as they release. https://codereview.chromium.org/1006933003/diff/200001/base/threading/platfor... base/threading/platform_thread_win.cc:286: // The only way to test for background mode is to explicitly try to exit it On 2015/03/19 22:19:21, rvargas (slow to respond) wrote: > Why not always set the priority to lowest (any os) and ignore this state in this > function? Well because we desire to use true background mode to get background IO/memory priority as well, but I decided to strip that out in this CL which already has enough structural changes, I'll consider bringing it back on its own or under a different form in a follow-up CL (PS: I don't understand why Windows' API is dumb and only works from current thread...)
The CQ bit was checked by gab@chromium.org to run a CQ dry run
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1006933003/240001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
https://codereview.chromium.org/1006933003/diff/200001/base/threading/platfor... File base/threading/platform_thread.h (right): https://codereview.chromium.org/1006933003/diff/200001/base/threading/platfor... base/threading/platform_thread.h:115: kThreadPriority_Normal, On 2015/03/30 20:14:45, gab wrote: > On 2015/03/19 22:19:21, rvargas (slow to respond) wrote: > > Let's order this according to priority. > > I was thinking about that too, but I was thinking we should probably also make > this an "enum class" (since we toggle it back-and-forth with the OS priority > expressed as an integer and can easily mix the two otherwise) -- feels like this > belongs in a follow-up (or precursor CL), WDYT? I doesn't have to be part of this CL, but we need a clear priority order here. https://codereview.chromium.org/1006933003/diff/200001/base/threading/platfor... File base/threading/platform_thread_freebsd.cc (right): https://codereview.chromium.org/1006933003/diff/200001/base/threading/platfor... base/threading/platform_thread_freebsd.cc:52: // Mirrors HandleSetThreadPriorityForPlatform()'s implementation. On 2015/03/30 20:14:45, gab wrote: > On 2015/03/19 22:19:21, rvargas (slow to respond) wrote: > > Do we need this comment? > > I like it, if anything it answers your question below :-) It doesn't answer my question :p. I noticed you were following the other function, and both seem buggy... but this comment seems too general for my taste (we have tons of methods that are a logical complement to others so we could add the same comment everywhere). If what you want to point out is that the bug is not really yours, then the comment is way too subtle. Up to you. https://codereview.chromium.org/1006933003/diff/200001/base/threading/platfor... base/threading/platform_thread_freebsd.cc:55: if (pthread_getschedparam(pthread_self(), &maybe_sched_rr, On 2015/03/30 20:14:45, gab wrote: > On 2015/03/19 22:19:21, rvargas (slow to respond) wrote: > > why self() and not handle? > > Because it mirrors HandleSetThreadPriorityForPlatform()'s implementation. > > I agree that this looks broken (or at least doesn't respect the existing API -- > filed http://crbug.com/468793 for someoen to investigate POSIX thread priority > once this lands) I don't think that's a strong enough justification (unless of course the code is correct). We should clarify this behavior before checking in more code, or at least add a clear TODO pointing to the bug you filed. https://codereview.chromium.org/1006933003/diff/200001/base/threading/platfor... File base/threading/platform_thread_posix.cc (right): https://codereview.chromium.org/1006933003/diff/200001/base/threading/platfor... base/threading/platform_thread_posix.cc:282: ScopedClearErrno clear_errno; On 2015/03/30 20:14:45, gab wrote: > On 2015/03/19 22:19:21, rvargas (slow to respond) wrote: > > errno = 0 instead? > > I don't know much about Chromium POSIX development, assumed that it was perhaps > prefered not to clobber errno here as a base/ citizen, but I don't care either > way, up to you. There is no contract on base to preserve errno (or last error). All that the API you're calling requires is for it to be zero beforehand. https://codereview.chromium.org/1006933003/diff/200001/base/threading/platfor... File base/threading/platform_thread_win.cc (right): https://codereview.chromium.org/1006933003/diff/200001/base/threading/platfor... base/threading/platform_thread_win.cc:139: return PlatformThreadHandle(::GetCurrentThread()); On 2015/03/30 20:14:45, gab wrote: > On 2015/03/19 22:19:21, rvargas (slow to respond) wrote: > > Sounds like it will leak. > > ::GetCurrentThread() returns a pseudo-handle which isn't leakable AFAIK. Yes, but now you have a valid PlatformThreadHandle that cannot be used anywhere but the current thread. Sounds like a broken implementation to me. https://codereview.chromium.org/1006933003/diff/200001/base/threading/platfor... base/threading/platform_thread_win.cc:277: DPCHECK(success); On 2015/03/30 20:14:45, gab wrote: > On 2015/03/19 22:19:21, rvargas (slow to respond) wrote: > > Should not check on a OS function return (and elsewhere) > > To me the goal of a debug check is to find out if this doesn't behave as > expected in some configurations (rather than silently fail) on some bots or some > dev machines -- or even on new OS versions as they release. A DCHECK is by definition some invariant of the code. An invariant is something that is somehow under control of the developer that is calling this function. The error code of an operation performed by the OS (or any form of status saying if the operation failed or not), is way outside the things that are under control of the developer. I feel very strongly against this practice... it just leads to debug builds that are very hard to use as random DCHEKS hit all the time. https://codereview.chromium.org/1006933003/diff/200001/base/threading/platfor... base/threading/platform_thread_win.cc:286: // The only way to test for background mode is to explicitly try to exit it On 2015/03/30 20:14:46, gab wrote: > On 2015/03/19 22:19:21, rvargas (slow to respond) wrote: > > Why not always set the priority to lowest (any os) and ignore this state in > this > > function? > > Well because we desire to use true background mode to get background IO/memory > priority as well, but I decided to strip that out in this CL which already has > enough structural changes, I'll consider bringing it back on its own or under a > different form in a follow-up CL (PS: I don't understand why Windows' API is > dumb and only works from current thread...) That's not what I meant. What I meant was that we could: SetThreadPriority(kThreadPriority_Background) { SetThreadPriority(THREAD_PRIORITY_LOWEST); If (we can) SetThreadPriority(THREAD_MODE_BACKGROUND_BEGIN); } GetThreadPriority() { if (THREAD_PRIORITY_LOWEST) return kThreadPriority_Background; } re PS: I suspect someone decided that injecting a thread to set other threads to background io was not a good thing. But asymmetric APIs are a pain. https://codereview.chromium.org/1006933003/diff/240001/base/threading/platfor... File base/threading/platform_thread_win.cc (right): https://codereview.chromium.org/1006933003/diff/240001/base/threading/platfor... base/threading/platform_thread_win.cc:256: DCHECK_NE(desired_priority, THREAD_PRIORITY_ERROR_RETURN); Isn't this the same as line 253?
Thanks PTAL. Cheers, Gab https://codereview.chromium.org/1006933003/diff/200001/base/threading/platfor... File base/threading/platform_thread_freebsd.cc (right): https://codereview.chromium.org/1006933003/diff/200001/base/threading/platfor... base/threading/platform_thread_freebsd.cc:55: if (pthread_getschedparam(pthread_self(), &maybe_sched_rr, On 2015/03/30 22:31:22, rvargas (slow to respond) wrote: > On 2015/03/30 20:14:45, gab wrote: > > On 2015/03/19 22:19:21, rvargas (slow to respond) wrote: > > > why self() and not handle? > > > > Because it mirrors HandleSetThreadPriorityForPlatform()'s implementation. > > > > I agree that this looks broken (or at least doesn't respect the existing API > -- > > filed http://crbug.com/468793 for someoen to investigate POSIX thread priority > > once this lands) > > I don't think that's a strong enough justification (unless of course the code is > correct). We should clarify this behavior before checking in more code, or at > least add a clear TODO pointing to the bug you filed. Added a TODO, will find an owner once this lands. https://codereview.chromium.org/1006933003/diff/200001/base/threading/platfor... File base/threading/platform_thread_posix.cc (right): https://codereview.chromium.org/1006933003/diff/200001/base/threading/platfor... base/threading/platform_thread_posix.cc:282: ScopedClearErrno clear_errno; On 2015/03/30 22:31:22, rvargas (slow to respond) wrote: > On 2015/03/30 20:14:45, gab wrote: > > On 2015/03/19 22:19:21, rvargas (slow to respond) wrote: > > > errno = 0 instead? > > > > I don't know much about Chromium POSIX development, assumed that it was > perhaps > > prefered not to clobber errno here as a base/ citizen, but I don't care either > > way, up to you. > > There is no contract on base to preserve errno (or last error). All that the API > you're calling requires is for it to be zero beforehand. Done. https://codereview.chromium.org/1006933003/diff/200001/base/threading/platfor... File base/threading/platform_thread_win.cc (right): https://codereview.chromium.org/1006933003/diff/200001/base/threading/platfor... base/threading/platform_thread_win.cc:139: return PlatformThreadHandle(::GetCurrentThread()); On 2015/03/30 22:31:23, rvargas (slow to respond) wrote: > On 2015/03/30 20:14:45, gab wrote: > > On 2015/03/19 22:19:21, rvargas (slow to respond) wrote: > > > Sounds like it will leak. > > > > ::GetCurrentThread() returns a pseudo-handle which isn't leakable AFAIK. > > Yes, but now you have a valid PlatformThreadHandle that cannot be used anywhere > but the current thread. Sounds like a broken implementation to me. Right, actually adjusted the API comment on PlatformThread::CurrentHandle() to reflect that. We can't Duplicate the pseudo-handle to get a real one either as then it would leak -- this entire handle API seems fragile, but this is outside the scope of this CL IMO. https://codereview.chromium.org/1006933003/diff/200001/base/threading/platfor... base/threading/platform_thread_win.cc:277: DPCHECK(success); On 2015/03/30 22:31:22, rvargas (slow to respond) wrote: > On 2015/03/30 20:14:45, gab wrote: > > On 2015/03/19 22:19:21, rvargas (slow to respond) wrote: > > > Should not check on a OS function return (and elsewhere) > > > > To me the goal of a debug check is to find out if this doesn't behave as > > expected in some configurations (rather than silently fail) on some bots or > some > > dev machines -- or even on new OS versions as they release. > > A DCHECK is by definition some invariant of the code. An invariant is something > that is somehow under control of the developer that is calling this function. > > The error code of an operation performed by the OS (or any form of status saying > if the operation failed or not), is way outside the things that are under > control of the developer. > > I feel very strongly against this practice... it just leads to debug builds that > are very hard to use as random DCHEKS hit all the time. > Okay changed to use DPLOG_IF(ERROR, !success) https://codereview.chromium.org/1006933003/diff/200001/base/threading/platfor... base/threading/platform_thread_win.cc:286: // The only way to test for background mode is to explicitly try to exit it On 2015/03/30 22:31:23, rvargas (slow to respond) wrote: > On 2015/03/30 20:14:46, gab wrote: > > On 2015/03/19 22:19:21, rvargas (slow to respond) wrote: > > > Why not always set the priority to lowest (any os) and ignore this state in > > this > > > function? > > > > Well because we desire to use true background mode to get background IO/memory > > priority as well, but I decided to strip that out in this CL which already has > > enough structural changes, I'll consider bringing it back on its own or under > a > > different form in a follow-up CL (PS: I don't understand why Windows' API is > > dumb and only works from current thread...) > > That's not what I meant. What I meant was that we could: > SetThreadPriority(kThreadPriority_Background) { > SetThreadPriority(THREAD_PRIORITY_LOWEST); > If (we can) SetThreadPriority(THREAD_MODE_BACKGROUND_BEGIN); > } > > GetThreadPriority() { > if (THREAD_PRIORITY_LOWEST) return kThreadPriority_Background; > } > > re PS: I suspect someone decided that injecting a thread to set other threads to > background io was not a good thing. But asymmetric APIs are a pain. Ah I see, still think it will be easier to do background mode in a separate CL. https://codereview.chromium.org/1006933003/diff/240001/base/threading/platfor... File base/threading/platform_thread_win.cc (right): https://codereview.chromium.org/1006933003/diff/240001/base/threading/platfor... base/threading/platform_thread_win.cc:256: DCHECK_NE(desired_priority, THREAD_PRIORITY_ERROR_RETURN); On 2015/03/30 22:31:23, rvargas (slow to respond) wrote: > Isn't this the same as line 253? If all case statements set |desired_priority|, yes, this is just explicitly stating the contract that they must.
Note: Added a few more tweaks in latest patch sets. Thanks, Gab https://codereview.chromium.org/1006933003/diff/200001/base/threading/platfor... File base/threading/platform_thread_win.cc (right): https://codereview.chromium.org/1006933003/diff/200001/base/threading/platfor... base/threading/platform_thread_win.cc:277: DPCHECK(success); On 2015/03/31 14:02:33, gab wrote: > On 2015/03/30 22:31:22, rvargas (slow to respond) wrote: > > On 2015/03/30 20:14:45, gab wrote: > > > On 2015/03/19 22:19:21, rvargas (slow to respond) wrote: > > > > Should not check on a OS function return (and elsewhere) > > > > > > To me the goal of a debug check is to find out if this doesn't behave as > > > expected in some configurations (rather than silently fail) on some bots or > > some > > > dev machines -- or even on new OS versions as they release. > > > > A DCHECK is by definition some invariant of the code. An invariant is > something > > that is somehow under control of the developer that is calling this function. > > > > The error code of an operation performed by the OS (or any form of status > saying > > if the operation failed or not), is way outside the things that are under > > control of the developer. > > > > I feel very strongly against this practice... it just leads to debug builds > that > > are very hard to use as random DCHEKS hit all the time. > > > > Okay changed to use DPLOG_IF(ERROR, !success) But FWIW, I'm more in favor of explicitly checking OS behaviours that we rely on. There is precedent for such checks, e.g.: https://code.google.com/p/chromium/codesearch#chromium/src/base/synchronizati...
Patchset #9 (id:360001) has been deleted
Patchset #8 (id:340001) has been deleted
Patchset #8 (id:360001) has been deleted
It looks basically good. Thanks. https://codereview.chromium.org/1006933003/diff/200001/base/threading/platfor... File base/threading/platform_thread_win.cc (right): https://codereview.chromium.org/1006933003/diff/200001/base/threading/platfor... base/threading/platform_thread_win.cc:139: return PlatformThreadHandle(::GetCurrentThread()); On 2015/03/31 14:02:33, gab wrote: > On 2015/03/30 22:31:23, rvargas (slow to respond) wrote: > > On 2015/03/30 20:14:45, gab wrote: > > > On 2015/03/19 22:19:21, rvargas (slow to respond) wrote: > > > > Sounds like it will leak. > > > > > > ::GetCurrentThread() returns a pseudo-handle which isn't leakable AFAIK. > > > > Yes, but now you have a valid PlatformThreadHandle that cannot be used > anywhere > > but the current thread. Sounds like a broken implementation to me. > > Right, actually adjusted the API comment on PlatformThread::CurrentHandle() to > reflect that. I'm not really sure what's the right hing to do now. There's exactly 3 users of CurrentHandle() today: two of them are Android calls to SetThreadPriority (tightly coupled with this CL) and the last one is CompositorOutputSurface setting main_thread_handle_, with a TODO to implement on windows... which leads me to believe that adding a comment about the handle not being usable from another thread is not going to help the only user of this call. > We can't Duplicate the pseudo-handle to get a real one either as then it would > leak -- this entire handle API seems fragile, but this is outside the scope of > this CL IMO. Yeah, that's why I CC'd you on the bug I filed about removing this whole thing. So while I agree that changing the usage model is clearly out of scope, I'm not convinced that we are vested enough in preserving the current model (almost no use!). From that point of view, having NOTIMPLEMENTED here is a very strong signal that something is broken, while returning the pseudo-handle creates the impression that things are OK. I would prefer not implementing this method and instead working around it in your code. At least it is less work when removing this class, and doesn't encourage external use. https://codereview.chromium.org/1006933003/diff/200001/base/threading/platfor... base/threading/platform_thread_win.cc:277: DPCHECK(success); On 2015/03/31 18:35:25, gab wrote: > On 2015/03/31 14:02:33, gab wrote: > > On 2015/03/30 22:31:22, rvargas (slow to respond) wrote: > > > On 2015/03/30 20:14:45, gab wrote: > > > > On 2015/03/19 22:19:21, rvargas (slow to respond) wrote: > > > > > Should not check on a OS function return (and elsewhere) > > > > > > > > To me the goal of a debug check is to find out if this doesn't behave as > > > > expected in some configurations (rather than silently fail) on some bots > or > > > some > > > > dev machines -- or even on new OS versions as they release. > > > > > > A DCHECK is by definition some invariant of the code. An invariant is > > something > > > that is somehow under control of the developer that is calling this > function. > > > > > > The error code of an operation performed by the OS (or any form of status > > saying > > > if the operation failed or not), is way outside the things that are under > > > control of the developer. > > > > > > I feel very strongly against this practice... it just leads to debug builds > > that > > > are very hard to use as random DCHEKS hit all the time. > > > > > > > Okay changed to use DPLOG_IF(ERROR, !success) > > But FWIW, I'm more in favor of explicitly checking OS behaviours that we rely > on. There is precedent for such checks, e.g.: > https://code.google.com/p/chromium/codesearch#chromium/src/base/synchronizati... Well played, sir. I should have removed that one. https://codereview.chromium.org/1006933003/diff/200001/base/threading/platfor... base/threading/platform_thread_win.cc:286: // The only way to test for background mode is to explicitly try to exit it On 2015/03/31 14:02:33, gab wrote: > On 2015/03/30 22:31:23, rvargas (slow to respond) wrote: > > On 2015/03/30 20:14:46, gab wrote: > > > On 2015/03/19 22:19:21, rvargas (slow to respond) wrote: > > > > Why not always set the priority to lowest (any os) and ignore this state > in > > > this > > > > function? > > > > > > Well because we desire to use true background mode to get background > IO/memory > > > priority as well, but I decided to strip that out in this CL which already > has > > > enough structural changes, I'll consider bringing it back on its own or > under > > a > > > different form in a follow-up CL (PS: I don't understand why Windows' API is > > > dumb and only works from current thread...) > > > > That's not what I meant. What I meant was that we could: > > SetThreadPriority(kThreadPriority_Background) { > > SetThreadPriority(THREAD_PRIORITY_LOWEST); > > If (we can) SetThreadPriority(THREAD_MODE_BACKGROUND_BEGIN); > > } > > > > GetThreadPriority() { > > if (THREAD_PRIORITY_LOWEST) return kThreadPriority_Background; > > } > > > > re PS: I suspect someone decided that injecting a thread to set other threads > to > > background io was not a good thing. But asymmetric APIs are a pain. > > Ah I see, still think it will be easier to do background mode in a separate CL. Acknowledged. https://codereview.chromium.org/1006933003/diff/380001/base/threading/platfor... File base/threading/platform_thread_freebsd.cc (right): https://codereview.chromium.org/1006933003/diff/380001/base/threading/platfor... base/threading/platform_thread_freebsd.cc:55: if (pthread_getschedparam(pthread_self(), &maybe_sched_rr, Don't see the TODO https://codereview.chromium.org/1006933003/diff/380001/base/threading/platfor... File base/threading/platform_thread_unittest.cc (right): https://codereview.chromium.org/1006933003/diff/380001/base/threading/platfor... base/threading/platform_thread_unittest.cc:85: thread_started_.Signal(); Shouldn't this be set between 76 and 77? (do we still need yield and sleep?)
Thanks, replies below and some tweaks, PTAL. https://codereview.chromium.org/1006933003/diff/200001/base/threading/platfor... File base/threading/platform_thread_win.cc (right): https://codereview.chromium.org/1006933003/diff/200001/base/threading/platfor... base/threading/platform_thread_win.cc:139: return PlatformThreadHandle(::GetCurrentThread()); On 2015/03/31 19:27:09, rvargas (slow to respond) wrote: > On 2015/03/31 14:02:33, gab wrote: > > On 2015/03/30 22:31:23, rvargas (slow to respond) wrote: > > > On 2015/03/30 20:14:45, gab wrote: > > > > On 2015/03/19 22:19:21, rvargas (slow to respond) wrote: > > > > > Sounds like it will leak. > > > > > > > > ::GetCurrentThread() returns a pseudo-handle which isn't leakable AFAIK. > > > > > > Yes, but now you have a valid PlatformThreadHandle that cannot be used > > anywhere > > > but the current thread. Sounds like a broken implementation to me. > > > > Right, actually adjusted the API comment on PlatformThread::CurrentHandle() > to > > reflect that. > > I'm not really sure what's the right hing to do now. There's exactly 3 users of > CurrentHandle() today: two of them are Android calls to SetThreadPriority > (tightly coupled with this CL) and the last one is CompositorOutputSurface > setting main_thread_handle_, with a TODO to implement on windows... which leads > me to believe that adding a comment about the handle not being usable from > another thread is not going to help the only user of this call. > > > We can't Duplicate the pseudo-handle to get a real one either as then it would > > leak -- this entire handle API seems fragile, but this is outside the scope of > > this CL IMO. > > Yeah, that's why I CC'd you on the bug I filed about removing this whole thing. > > So while I agree that changing the usage model is clearly out of scope, I'm not > convinced that we are vested enough in preserving the current model (almost no > use!). From that point of view, having NOTIMPLEMENTED here is a very strong > signal that something is broken, while returning the pseudo-handle creates the > impression that things are OK. > > I would prefer not implementing this method and instead working around it in > your code. At least it is less work when removing this class, and doesn't > encourage external use. I'm not convinced that it not applying to existing use cases warrants hardcoding hacks in places it could now be useful (the next step for me is to experiment with backgrounding threads which may very well need a cross-platform way of expressing "current thread"). I agree however that restricting the API for all platforms hurts existing use cases, so I made the API comment Windows specific, how about that? TBH, I wouldn't worry too much about its usage spreading wildly. If anything I think this comment makes it clearer (i.e., I disagree that keeping a NONIMPLEMENTED() on Windows hidden behind an API that doesn't mention it will help keep callers away). https://codereview.chromium.org/1006933003/diff/380001/base/threading/platfor... File base/threading/platform_thread_freebsd.cc (right): https://codereview.chromium.org/1006933003/diff/380001/base/threading/platfor... base/threading/platform_thread_freebsd.cc:55: if (pthread_getschedparam(pthread_self(), &maybe_sched_rr, On 2015/03/31 19:27:10, rvargas (slow to respond) wrote: > Don't see the TODO Oops had put it in _linux.cc only... https://codereview.chromium.org/1006933003/diff/380001/base/threading/platfor... File base/threading/platform_thread_unittest.cc (right): https://codereview.chromium.org/1006933003/diff/380001/base/threading/platfor... base/threading/platform_thread_unittest.cc:85: thread_started_.Signal(); On 2015/03/31 19:27:10, rvargas (slow to respond) wrote: > Shouldn't this be set between 76 and 77? (do we still need yield and sleep?) I guess the yield and sleep are no longer needed. The only value was to make sure that we don't somehow change thread IDs after we do that, but PlatformThreadTest.Function already verifies that for the main thread. Removed. Otherwise, it does belong on this line as we need to call TrivialThread::ThreadMain() first to set did_run().
https://codereview.chromium.org/1006933003/diff/200001/base/threading/platfor... File base/threading/platform_thread_win.cc (right): https://codereview.chromium.org/1006933003/diff/200001/base/threading/platfor... base/threading/platform_thread_win.cc:139: return PlatformThreadHandle(::GetCurrentThread()); On 2015/03/31 20:18:20, gab wrote: > On 2015/03/31 19:27:09, rvargas (slow to respond) wrote: > > On 2015/03/31 14:02:33, gab wrote: > > > On 2015/03/30 22:31:23, rvargas (slow to respond) wrote: > > > > On 2015/03/30 20:14:45, gab wrote: > > > > > On 2015/03/19 22:19:21, rvargas (slow to respond) wrote: > > > > > > Sounds like it will leak. > > > > > > > > > > ::GetCurrentThread() returns a pseudo-handle which isn't leakable AFAIK. > > > > > > > > Yes, but now you have a valid PlatformThreadHandle that cannot be used > > > anywhere > > > > but the current thread. Sounds like a broken implementation to me. > > > > > > Right, actually adjusted the API comment on PlatformThread::CurrentHandle() > > to > > > reflect that. > > > > I'm not really sure what's the right hing to do now. There's exactly 3 users > of > > CurrentHandle() today: two of them are Android calls to SetThreadPriority > > (tightly coupled with this CL) and the last one is CompositorOutputSurface > > setting main_thread_handle_, with a TODO to implement on windows... which > leads > > me to believe that adding a comment about the handle not being usable from > > another thread is not going to help the only user of this call. > > > > > We can't Duplicate the pseudo-handle to get a real one either as then it > would > > > leak -- this entire handle API seems fragile, but this is outside the scope > of > > > this CL IMO. > > > > Yeah, that's why I CC'd you on the bug I filed about removing this whole > thing. > > > > So while I agree that changing the usage model is clearly out of scope, I'm > not > > convinced that we are vested enough in preserving the current model (almost no > > use!). From that point of view, having NOTIMPLEMENTED here is a very strong > > signal that something is broken, while returning the pseudo-handle creates the > > impression that things are OK. > > > > I would prefer not implementing this method and instead working around it in > > your code. At least it is less work when removing this class, and doesn't > > encourage external use. > > I'm not convinced that it not applying to existing use cases warrants hardcoding > hacks in places it could now be useful (the next step for me is to experiment > with backgrounding threads which may very well need a cross-platform way of > expressing "current thread"). > > I agree however that restricting the API for all platforms hurts existing use > cases, so I made the API comment Windows specific, how about that? > > TBH, I wouldn't worry too much about its usage spreading wildly. If anything I > think this comment makes it clearer (i.e., I disagree that keeping a > NONIMPLEMENTED() on Windows hidden behind an API that doesn't mention it will > help keep callers away). I agree that the current situation is not good because there is no indication that the API is not implemented everywhere (other than the obvious crash as soon as someone tries to use it on multiplatform code!). So a comment is a welcomed addition. (to be fair, having NOTIMPLEMENTED without a note on the header is fairly common). But... the current status is fairly limiting to anyone that tries to use this, which is a good thing because the API as it is seems broken. I'd rather remove the API completely than move to a more imbalanced state: I don't think we should have base exposing APIs that behave quite different per platform (x can use from any thread, y cannot). So if you believe that you need this functionality for your next CL, and that what you want is a pseudo-handle that can only be used from the current thread, then I'd suggest renaming the method to be more obvious about it ("PseudoHandleForCurrenThread()", I'm bad with names). That will fit your new code and two out of the three current uses, and hopefully provide a strong hint about not using it to identify threads. I don't know what to do about the third one though... it probably needs a real handle, and the API needs a way to close that handle (which should be a move-only RAAI type so PlatformThreadHandle is not a good fit). I would be fine using the same code for the part that is implemented (remember that said code is not finished as it presumably doesn't work on Windows), and adding yet another TODO to do The Right Thing later. https://codereview.chromium.org/1006933003/diff/380001/base/threading/platfor... File base/threading/platform_thread_unittest.cc (right): https://codereview.chromium.org/1006933003/diff/380001/base/threading/platfor... base/threading/platform_thread_unittest.cc:85: thread_started_.Signal(); On 2015/03/31 20:18:20, gab wrote: > On 2015/03/31 19:27:10, rvargas (slow to respond) wrote: > > Shouldn't this be set between 76 and 77? (do we still need yield and sleep?) > > I guess the yield and sleep are no longer needed. The only value was to make > sure that we don't somehow change thread IDs after we do that, but > PlatformThreadTest.Function already verifies that for the main thread. > > Removed. > > Otherwise, it does belong on this line as we need to call > TrivialThread::ThreadMain() first to set did_run(). Thanks. But then I believe the name is misleading because this is not about the thread having started but about the thread having finished whatever it was supposed to do (set did_run_?). We should name things accordingly. And it's a nice improvement btw. https://codereview.chromium.org/1006933003/diff/400001/base/threading/platfor... File base/threading/platform_thread_freebsd.cc (right): https://codereview.chromium.org/1006933003/diff/400001/base/threading/platfor... base/threading/platform_thread_freebsd.cc:54: // TODO: Assess the correctness of using |pthread_self()| below instead of nit: add a name (no, the name on a TODO is not meant to point to the person in charge of implementing it).
Patchset #10 (id:420001) has been deleted
See below. Thanks! GAb https://codereview.chromium.org/1006933003/diff/200001/base/threading/platfor... File base/threading/platform_thread_win.cc (right): https://codereview.chromium.org/1006933003/diff/200001/base/threading/platfor... base/threading/platform_thread_win.cc:139: return PlatformThreadHandle(::GetCurrentThread()); On 2015/03/31 22:43:44, rvargas (slow to respond) wrote: > On 2015/03/31 20:18:20, gab wrote: > > On 2015/03/31 19:27:09, rvargas (slow to respond) wrote: > > > On 2015/03/31 14:02:33, gab wrote: > > > > On 2015/03/30 22:31:23, rvargas (slow to respond) wrote: > > > > > On 2015/03/30 20:14:45, gab wrote: > > > > > > On 2015/03/19 22:19:21, rvargas (slow to respond) wrote: > > > > > > > Sounds like it will leak. > > > > > > > > > > > > ::GetCurrentThread() returns a pseudo-handle which isn't leakable > AFAIK. > > > > > > > > > > Yes, but now you have a valid PlatformThreadHandle that cannot be used > > > > anywhere > > > > > but the current thread. Sounds like a broken implementation to me. > > > > > > > > Right, actually adjusted the API comment on > PlatformThread::CurrentHandle() > > > to > > > > reflect that. > > > > > > I'm not really sure what's the right hing to do now. There's exactly 3 users > > of > > > CurrentHandle() today: two of them are Android calls to SetThreadPriority > > > (tightly coupled with this CL) and the last one is CompositorOutputSurface > > > setting main_thread_handle_, with a TODO to implement on windows... which > > leads > > > me to believe that adding a comment about the handle not being usable from > > > another thread is not going to help the only user of this call. > > > > > > > We can't Duplicate the pseudo-handle to get a real one either as then it > > would > > > > leak -- this entire handle API seems fragile, but this is outside the > scope > > of > > > > this CL IMO. > > > > > > Yeah, that's why I CC'd you on the bug I filed about removing this whole > > thing. > > > > > > So while I agree that changing the usage model is clearly out of scope, I'm > > not > > > convinced that we are vested enough in preserving the current model (almost > no > > > use!). From that point of view, having NOTIMPLEMENTED here is a very strong > > > signal that something is broken, while returning the pseudo-handle creates > the > > > impression that things are OK. > > > > > > I would prefer not implementing this method and instead working around it in > > > your code. At least it is less work when removing this class, and doesn't > > > encourage external use. > > > > I'm not convinced that it not applying to existing use cases warrants > hardcoding > > hacks in places it could now be useful (the next step for me is to experiment > > with backgrounding threads which may very well need a cross-platform way of > > expressing "current thread"). > > > > I agree however that restricting the API for all platforms hurts existing use > > cases, so I made the API comment Windows specific, how about that? > > > > TBH, I wouldn't worry too much about its usage spreading wildly. If anything I > > think this comment makes it clearer (i.e., I disagree that keeping a > > NONIMPLEMENTED() on Windows hidden behind an API that doesn't mention it will > > help keep callers away). > > I agree that the current situation is not good because there is no indication > that the API is not implemented everywhere (other than the obvious crash as soon > as someone tries to use it on multiplatform code!). So a comment is a welcomed > addition. (to be fair, having NOTIMPLEMENTED without a note on the header is > fairly common). > > But... the current status is fairly limiting to anyone that tries to use this, > which is a good thing because the API as it is seems broken. I'd rather remove > the API completely than move to a more imbalanced state: I don't think we should > have base exposing APIs that behave quite different per platform (x can use from > any thread, y cannot). > > So if you believe that you need this functionality for your next CL, and that > what you want is a pseudo-handle that can only be used from the current thread, > then I'd suggest renaming the method to be more obvious about it > ("PseudoHandleForCurrenThread()", I'm bad with names). That will fit your new > code and two out of the three current uses, and hopefully provide a strong hint > about not using it to identify threads. > > I don't know what to do about the third one though... it probably needs a real > handle, and the API needs a way to close that handle (which should be a > move-only RAAI type so PlatformThreadHandle is not a good fit). I would be fine > using the same code for the part that is implemented (remember that said code is > not finished as it presumably doesn't work on Windows), and adding yet another > TODO to do The Right Thing later. How about we make a single call that returns an object representing a pseudo-handle which exposes a GetThreadSafeCopy() method which itself returns real handle inside an RAII object? i.e., something like: class PlatformThreadHandle; (pure-virtual - move-only RAII type) class PlatformThreadPseudoHandle : public PlatformThreadHandle, public NonThreadSafe { public: Handle handle() override { CalledOnValidThread(); return pseudo_handle_; } PlatformThreadHandle GetThreadSafeCopy() { return PlatformThreadThreadSafeHandle(*this); } } class PlatformThreadThreadSafeHandle : public PlatformThreadHandle { public: PlatformThreadThreadSafeHandle(const PlatformThreadHandle& other) { (duplicate |other|...) } ~PlatformThreadThreadSafeHandle() { (release handle...) } Handle handle() override { return handle_; } } This way most users don't pay the cost of obtaining the real handle and those that truly need one, can happily get one? Feels outside the scope of this CL, happy to attack this as a precursor or follow-up, whichever your prefer. https://codereview.chromium.org/1006933003/diff/380001/base/threading/platfor... File base/threading/platform_thread_unittest.cc (right): https://codereview.chromium.org/1006933003/diff/380001/base/threading/platfor... base/threading/platform_thread_unittest.cc:85: thread_started_.Signal(); On 2015/03/31 22:43:45, rvargas (slow to respond) wrote: > On 2015/03/31 20:18:20, gab wrote: > > On 2015/03/31 19:27:10, rvargas (slow to respond) wrote: > > > Shouldn't this be set between 76 and 77? (do we still need yield and sleep?) > > > > I guess the yield and sleep are no longer needed. The only value was to make > > sure that we don't somehow change thread IDs after we do that, but > > PlatformThreadTest.Function already verifies that for the main thread. > > > > Removed. > > > > Otherwise, it does belong on this line as we need to call > > TrivialThread::ThreadMain() first to set did_run(). > > Thanks. But then I believe the name is misleading because this is not about the > thread having started but about the thread having finished whatever it was > supposed to do (set did_run_?). We should name things accordingly. > > And it's a nice improvement btw. I see, agreed, actually it's the dependency on TrivialThread that doesn't make much sense here, removed and replaced by a more meaningful IsRunning() method. I now actually think that the tests based on TrivialThread actually test a strict subset of what FunctionTestThread-based tests do, shall we get rid of them? https://codereview.chromium.org/1006933003/diff/400001/base/threading/platfor... File base/threading/platform_thread_freebsd.cc (right): https://codereview.chromium.org/1006933003/diff/400001/base/threading/platfor... base/threading/platform_thread_freebsd.cc:54: // TODO: Assess the correctness of using |pthread_self()| below instead of On 2015/03/31 22:43:45, rvargas (slow to respond) wrote: > nit: add a name (no, the name on a TODO is not meant to point to the person in > charge of implementing it). Done.
Thanks for your patience. https://codereview.chromium.org/1006933003/diff/200001/base/threading/platfor... File base/threading/platform_thread_win.cc (right): https://codereview.chromium.org/1006933003/diff/200001/base/threading/platfor... base/threading/platform_thread_win.cc:139: return PlatformThreadHandle(::GetCurrentThread()); On 2015/04/01 01:14:39, gab wrote: > On 2015/03/31 22:43:44, rvargas (slow to respond) wrote: > > On 2015/03/31 20:18:20, gab wrote: > > > On 2015/03/31 19:27:09, rvargas (slow to respond) wrote: > > > > On 2015/03/31 14:02:33, gab wrote: > > > > > On 2015/03/30 22:31:23, rvargas (slow to respond) wrote: > > > > > > On 2015/03/30 20:14:45, gab wrote: > > > > > > > On 2015/03/19 22:19:21, rvargas (slow to respond) wrote: > > > > > > > > Sounds like it will leak. > > > > > > > > > > > > > > ::GetCurrentThread() returns a pseudo-handle which isn't leakable > > AFAIK. > > > > > > > > > > > > Yes, but now you have a valid PlatformThreadHandle that cannot be used > > > > > anywhere > > > > > > but the current thread. Sounds like a broken implementation to me. > > > > > > > > > > Right, actually adjusted the API comment on > > PlatformThread::CurrentHandle() > > > > to > > > > > reflect that. > > > > > > > > I'm not really sure what's the right hing to do now. There's exactly 3 > users > > > of > > > > CurrentHandle() today: two of them are Android calls to SetThreadPriority > > > > (tightly coupled with this CL) and the last one is CompositorOutputSurface > > > > setting main_thread_handle_, with a TODO to implement on windows... which > > > leads > > > > me to believe that adding a comment about the handle not being usable from > > > > another thread is not going to help the only user of this call. > > > > > > > > > We can't Duplicate the pseudo-handle to get a real one either as then it > > > would > > > > > leak -- this entire handle API seems fragile, but this is outside the > > scope > > > of > > > > > this CL IMO. > > > > > > > > Yeah, that's why I CC'd you on the bug I filed about removing this whole > > > thing. > > > > > > > > So while I agree that changing the usage model is clearly out of scope, > I'm > > > not > > > > convinced that we are vested enough in preserving the current model > (almost > > no > > > > use!). From that point of view, having NOTIMPLEMENTED here is a very > strong > > > > signal that something is broken, while returning the pseudo-handle creates > > the > > > > impression that things are OK. > > > > > > > > I would prefer not implementing this method and instead working around it > in > > > > your code. At least it is less work when removing this class, and doesn't > > > > encourage external use. > > > > > > I'm not convinced that it not applying to existing use cases warrants > > hardcoding > > > hacks in places it could now be useful (the next step for me is to > experiment > > > with backgrounding threads which may very well need a cross-platform way of > > > expressing "current thread"). > > > > > > I agree however that restricting the API for all platforms hurts existing > use > > > cases, so I made the API comment Windows specific, how about that? > > > > > > TBH, I wouldn't worry too much about its usage spreading wildly. If anything > I > > > think this comment makes it clearer (i.e., I disagree that keeping a > > > NONIMPLEMENTED() on Windows hidden behind an API that doesn't mention it > will > > > help keep callers away). > > > > I agree that the current situation is not good because there is no indication > > that the API is not implemented everywhere (other than the obvious crash as > soon > > as someone tries to use it on multiplatform code!). So a comment is a welcomed > > addition. (to be fair, having NOTIMPLEMENTED without a note on the header is > > fairly common). > > > > But... the current status is fairly limiting to anyone that tries to use this, > > which is a good thing because the API as it is seems broken. I'd rather remove > > the API completely than move to a more imbalanced state: I don't think we > should > > have base exposing APIs that behave quite different per platform (x can use > from > > any thread, y cannot). > > > > So if you believe that you need this functionality for your next CL, and that > > what you want is a pseudo-handle that can only be used from the current > thread, > > then I'd suggest renaming the method to be more obvious about it > > ("PseudoHandleForCurrenThread()", I'm bad with names). That will fit your new > > code and two out of the three current uses, and hopefully provide a strong > hint > > about not using it to identify threads. > > > > I don't know what to do about the third one though... it probably needs a real > > handle, and the API needs a way to close that handle (which should be a > > move-only RAAI type so PlatformThreadHandle is not a good fit). I would be > fine > > using the same code for the part that is implemented (remember that said code > is > > not finished as it presumably doesn't work on Windows), and adding yet another > > TODO to do The Right Thing later. > > How about we make a single call that returns an object representing a > pseudo-handle which exposes a GetThreadSafeCopy() method which itself returns > real handle inside an RAII object? > > i.e., something like: > > class PlatformThreadHandle; (pure-virtual - move-only RAII type) > > class PlatformThreadPseudoHandle : public PlatformThreadHandle, > public NonThreadSafe { > public: > Handle handle() override { CalledOnValidThread(); return pseudo_handle_; } > > PlatformThreadHandle GetThreadSafeCopy() { return > PlatformThreadThreadSafeHandle(*this); } > } > > class PlatformThreadThreadSafeHandle : public PlatformThreadHandle { > public: > PlatformThreadThreadSafeHandle(const PlatformThreadHandle& other) { > (duplicate |other|...) > } > > ~PlatformThreadThreadSafeHandle() { (release handle...) } > > Handle handle() override { return handle_; } > } > > > > This way most users don't pay the cost of obtaining the real handle and those > that truly need one, can happily get one? > > Feels outside the scope of this CL, happy to attack this as a precursor or > follow-up, whichever your prefer. How does this play in the context of crbug.com/468932 ? I mean, I like the idea of something that is implicitly pseudoish, but I'm not sure about PlatformThreadHandle (it's mostly a matter of naming, I guess). I sort of think that base::Thread should be the real object (to parallel base::Process). Note that the way Process deals with this issue is by an internal bool that keeps track of the object being a real handle or a pseudo handle; that way callers mostly don't care about what they have. The issue in this case is that CurrentThread is more subtle in that just passing it to another thread makes it invalid... hence the allure of a subclass. (but NonThreadSafe objects in general can be created on a different thread than where they are used all the time). Anyways, yes, that doesn't belong here. Ideally, we should start moving in a better direction sort of in parallel. We are adding new APIs and we might as well add them were we think they should live long term. In that context, it can be post or pre- this CL... with the caveat that just looking at this CL, there are no new calls of PlatformThread::CurrentHandle() (right?) so it's not that we _have_ to implement this on Windows as part of this CL. https://codereview.chromium.org/1006933003/diff/380001/base/threading/platfor... File base/threading/platform_thread_unittest.cc (right): https://codereview.chromium.org/1006933003/diff/380001/base/threading/platfor... base/threading/platform_thread_unittest.cc:85: thread_started_.Signal(); On 2015/04/01 01:14:39, gab wrote: > On 2015/03/31 22:43:45, rvargas (slow to respond) wrote: > > On 2015/03/31 20:18:20, gab wrote: > > > On 2015/03/31 19:27:10, rvargas (slow to respond) wrote: > > > > Shouldn't this be set between 76 and 77? (do we still need yield and > sleep?) > > > > > > I guess the yield and sleep are no longer needed. The only value was to make > > > sure that we don't somehow change thread IDs after we do that, but > > > PlatformThreadTest.Function already verifies that for the main thread. > > > > > > Removed. > > > > > > Otherwise, it does belong on this line as we need to call > > > TrivialThread::ThreadMain() first to set did_run(). > > > > Thanks. But then I believe the name is misleading because this is not about > the > > thread having started but about the thread having finished whatever it was > > supposed to do (set did_run_?). We should name things accordingly. > > > > And it's a nice improvement btw. > > I see, agreed, actually it's the dependency on TrivialThread that doesn't make > much sense here, removed and replaced by a more meaningful IsRunning() method. > > I now actually think that the tests based on TrivialThread actually test a > strict subset of what FunctionTestThread-based tests do, shall we get rid of > them? Given that one set has explicit synchronization and the other doesn't, it seems better to keep them.
https://codereview.chromium.org/1006933003/diff/200001/base/threading/platfor... File base/threading/platform_thread_win.cc (right): https://codereview.chromium.org/1006933003/diff/200001/base/threading/platfor... base/threading/platform_thread_win.cc:139: return PlatformThreadHandle(::GetCurrentThread()); On 2015/04/01 02:04:55, rvargas (slow to respond) wrote: > On 2015/04/01 01:14:39, gab wrote: > > On 2015/03/31 22:43:44, rvargas (slow to respond) wrote: > > > On 2015/03/31 20:18:20, gab wrote: > > > > On 2015/03/31 19:27:09, rvargas (slow to respond) wrote: > > > > > On 2015/03/31 14:02:33, gab wrote: > > > > > > On 2015/03/30 22:31:23, rvargas (slow to respond) wrote: > > > > > > > On 2015/03/30 20:14:45, gab wrote: > > > > > > > > On 2015/03/19 22:19:21, rvargas (slow to respond) wrote: > > > > > > > > > Sounds like it will leak. > > > > > > > > > > > > > > > > ::GetCurrentThread() returns a pseudo-handle which isn't leakable > > > AFAIK. > > > > > > > > > > > > > > Yes, but now you have a valid PlatformThreadHandle that cannot be > used > > > > > > anywhere > > > > > > > but the current thread. Sounds like a broken implementation to me. > > > > > > > > > > > > Right, actually adjusted the API comment on > > > PlatformThread::CurrentHandle() > > > > > to > > > > > > reflect that. > > > > > > > > > > I'm not really sure what's the right hing to do now. There's exactly 3 > > users > > > > of > > > > > CurrentHandle() today: two of them are Android calls to > SetThreadPriority > > > > > (tightly coupled with this CL) and the last one is > CompositorOutputSurface > > > > > setting main_thread_handle_, with a TODO to implement on windows... > which > > > > leads > > > > > me to believe that adding a comment about the handle not being usable > from > > > > > another thread is not going to help the only user of this call. > > > > > > > > > > > We can't Duplicate the pseudo-handle to get a real one either as then > it > > > > would > > > > > > leak -- this entire handle API seems fragile, but this is outside the > > > scope > > > > of > > > > > > this CL IMO. > > > > > > > > > > Yeah, that's why I CC'd you on the bug I filed about removing this whole > > > > thing. > > > > > > > > > > So while I agree that changing the usage model is clearly out of scope, > > I'm > > > > not > > > > > convinced that we are vested enough in preserving the current model > > (almost > > > no > > > > > use!). From that point of view, having NOTIMPLEMENTED here is a very > > strong > > > > > signal that something is broken, while returning the pseudo-handle > creates > > > the > > > > > impression that things are OK. > > > > > > > > > > I would prefer not implementing this method and instead working around > it > > in > > > > > your code. At least it is less work when removing this class, and > doesn't > > > > > encourage external use. > > > > > > > > I'm not convinced that it not applying to existing use cases warrants > > > hardcoding > > > > hacks in places it could now be useful (the next step for me is to > > experiment > > > > with backgrounding threads which may very well need a cross-platform way > of > > > > expressing "current thread"). > > > > > > > > I agree however that restricting the API for all platforms hurts existing > > use > > > > cases, so I made the API comment Windows specific, how about that? > > > > > > > > TBH, I wouldn't worry too much about its usage spreading wildly. If > anything > > I > > > > think this comment makes it clearer (i.e., I disagree that keeping a > > > > NONIMPLEMENTED() on Windows hidden behind an API that doesn't mention it > > will > > > > help keep callers away). > > > > > > I agree that the current situation is not good because there is no > indication > > > that the API is not implemented everywhere (other than the obvious crash as > > soon > > > as someone tries to use it on multiplatform code!). So a comment is a > welcomed > > > addition. (to be fair, having NOTIMPLEMENTED without a note on the header is > > > fairly common). > > > > > > But... the current status is fairly limiting to anyone that tries to use > this, > > > which is a good thing because the API as it is seems broken. I'd rather > remove > > > the API completely than move to a more imbalanced state: I don't think we > > should > > > have base exposing APIs that behave quite different per platform (x can use > > from > > > any thread, y cannot). > > > > > > So if you believe that you need this functionality for your next CL, and > that > > > what you want is a pseudo-handle that can only be used from the current > > thread, > > > then I'd suggest renaming the method to be more obvious about it > > > ("PseudoHandleForCurrenThread()", I'm bad with names). That will fit your > new > > > code and two out of the three current uses, and hopefully provide a strong > > hint > > > about not using it to identify threads. > > > > > > I don't know what to do about the third one though... it probably needs a > real > > > handle, and the API needs a way to close that handle (which should be a > > > move-only RAAI type so PlatformThreadHandle is not a good fit). I would be > > fine > > > using the same code for the part that is implemented (remember that said > code > > is > > > not finished as it presumably doesn't work on Windows), and adding yet > another > > > TODO to do The Right Thing later. > > > > How about we make a single call that returns an object representing a > > pseudo-handle which exposes a GetThreadSafeCopy() method which itself returns > > real handle inside an RAII object? > > > > i.e., something like: > > > > class PlatformThreadHandle; (pure-virtual - move-only RAII type) > > > > class PlatformThreadPseudoHandle : public PlatformThreadHandle, > > public NonThreadSafe { > > public: > > Handle handle() override { CalledOnValidThread(); return pseudo_handle_; } > > > > PlatformThreadHandle GetThreadSafeCopy() { return > > PlatformThreadThreadSafeHandle(*this); } > > } > > > > class PlatformThreadThreadSafeHandle : public PlatformThreadHandle { > > public: > > PlatformThreadThreadSafeHandle(const PlatformThreadHandle& other) { > > (duplicate |other|...) > > } > > > > ~PlatformThreadThreadSafeHandle() { (release handle...) } > > > > Handle handle() override { return handle_; } > > } > > > > > > > > This way most users don't pay the cost of obtaining the real handle and those > > that truly need one, can happily get one? > > > > Feels outside the scope of this CL, happy to attack this as a precursor or > > follow-up, whichever your prefer. > > How does this play in the context of crbug.com/468932 ? > > I mean, I like the idea of something that is implicitly pseudoish, but I'm not > sure about PlatformThreadHandle (it's mostly a matter of naming, I guess). I > sort of think that base::Thread should be the real object (to parallel > base::Process). > > Note that the way Process deals with this issue is by an internal bool that > keeps track of the object being a real handle or a pseudo handle; that way > callers mostly don't care about what they have. The issue in this case is that > CurrentThread is more subtle in that just passing it to another thread makes it > invalid... hence the allure of a subclass. (but NonThreadSafe objects in general > can be created on a different thread than where they are used all the time). Non-thread-safe objects yes, but not implementers of NonThreadSafe AFAIK (unless they call DetachFromThread() which stops the checking and kind of defeats the purpose). > > Anyways, yes, that doesn't belong here. Ideally, we should start moving in a > better direction sort of in parallel. We are adding new APIs and we might as > well add them were we think they should live long term. > > In that context, it can be post or pre- this CL... with the caveat that just > looking at this CL, there are no new calls of PlatformThread::CurrentHandle() > (right?) so it's not that we _have_ to implement this on Windows as part of this > CL. It is used in this CL (in the new tests), I can have Windows if'defs there for now instead of fixing the Windows impl in this CL, up to you.
ps 10 lgtm after nailing final nit. https://codereview.chromium.org/1006933003/diff/200001/base/threading/platfor... File base/threading/platform_thread_win.cc (right): https://codereview.chromium.org/1006933003/diff/200001/base/threading/platfor... base/threading/platform_thread_win.cc:139: return PlatformThreadHandle(::GetCurrentThread()); > > > > In that context, it can be post or pre- this CL... with the caveat that just > > looking at this CL, there are no new calls of PlatformThread::CurrentHandle() > > (right?) so it's not that we _have_ to implement this on Windows as part of > this > > CL. > > It is used in this CL (in the new tests), I can have Windows if'defs there for > now instead of fixing the Windows impl in this CL, up to you. How about this: if PlatformThread::CurrentHandle is fixed/replaced in less than say, two or three weeks then go ahead and just return ::GetCurrentThread. If the fix may take longer than that then let's go with ifdefs on the test.
The CQ bit was checked by gab@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1006933003/440001
Message was sent while issue was closed.
Committed patchset #10 (id:440001)
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/f1921cb37b83bce16c07ca93975cded2b2863dd0 Cr-Commit-Position: refs/heads/master@{#323266}
Message was sent while issue was closed.
Oops, forgot to send drafts before landing: https://codereview.chromium.org/1006933003/diff/200001/base/threading/platfor... File base/threading/platform_thread_win.cc (right): https://codereview.chromium.org/1006933003/diff/200001/base/threading/platfor... base/threading/platform_thread_win.cc:139: return PlatformThreadHandle(::GetCurrentThread()); On 2015/04/01 16:31:30, rvargas (soon out of office) wrote: > > > > > > In that context, it can be post or pre- this CL... with the caveat that just > > > looking at this CL, there are no new calls of > PlatformThread::CurrentHandle() > > > (right?) so it's not that we _have_ to implement this on Windows as part of > > this > > > CL. > > > > It is used in this CL (in the new tests), I can have Windows if'defs there for > > now instead of fixing the Windows impl in this CL, up to you. > > How about this: if PlatformThread::CurrentHandle is fixed/replaced in less than > say, two or three weeks then go ahead and just return ::GetCurrentThread. If the > fix may take longer than that then let's go with ifdefs on the test. Okay, I will work on this next, expect a CL soon. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
