Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
15203009:
posix thread: Attempt to set audio threads to real time. (Closed)
|
Created:
7 years, 7 months ago by dgreid Modified:
7 years, 6 months ago Reviewers:
jar (doing other things), Chris Rogers, DaveMoore, piman, brettw, willchan no longer on Chromium CC:
chromium-reviews, erikwright+watch_chromium.org, sail+watch_chromium.org, DaleCurtis Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
Descriptionposix thread: Attempt to set audio threads to real time.
Set the scheduling class of audio threads to SCHED_RR if possible. If
not, fall back to setting the nice level. Setting the thread to
real time helps when playing/recording low latency audio. Also
changing the scheduler keeps audio threads from being limited by
bandwidth limits placed on background tabs, those apply to SCHED_OTHER
threads only. The rest of the process will be limited by these, only
the audio threads will have high priority.
This still isn't perfect, youtube will occasionally glitch on arm,
either because of video decode or maybe a thread audio is dependent on
that isn't real time.
BUG=chromium:213118
TEST=check audio thread priorities with ps, see that they are set.
Play audio from grooveshark or youtube in the background while loading
other tabs and listen for dropouts.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=203896
Patch Set 1 #
Total comments: 6
Patch Set 2 : move to new platform_thread_linux #
Total comments: 2
Patch Set 3 : Add DHCHECK for rt prio success on cros #
Total comments: 4
Patch Set 4 : switch back from dcheck to return for cros #Patch Set 5 : braces for if. #
Total comments: 11
Patch Set 6 : remove log and extra chromeos return #Messages
Total messages: 27 (0 generated)
This will do nothing on desktop linux unless the user has allowed the chrome process access to real time scheduling. For chromeos I've posted a change to let chrome do this. The platform will limit the amount of time a process can run in realtime to 80% of the cpu. I tested this by spinning two realtime tasks on spring, the ui remained functional and I could close the offending tabs. What do you think? This makes a huge difference with complicated webaudio pages such as our new eq tuning interface.
On 2013/05/17 18:53:41, dgreid wrote: > This will do nothing on desktop linux unless the user has allowed the chrome > process access to real time scheduling. For chromeos I've posted a change to > let chrome do this. > > The platform will limit the amount of time a process can run in realtime to 80% > of the cpu. I tested this by spinning two realtime tasks on spring, the ui > remained functional and I could close the offending tabs. > > What do you think? This makes a huge difference with complicated webaudio pages > such as our new eq tuning interface. Thanks Dylan, I'll leave it to the Linux experts for the details of this review. Just FYI we currently create the thread with base::kThreadPriority_RealtimeAudio only for clients of AudioOutputDevice (<audio>/<video>, WebAudio, WebRTC), but PPAPI still doesn't, so anything Flash-related will not *yet* benefit. Dale is looking into that aspect...
All the OS side changes for ChomeOS to allow this are ready to land.
https://codereview.chromium.org/15203009/diff/1/base/threading/platform_threa... File base/threading/platform_thread_posix.cc (right): https://codereview.chromium.org/15203009/diff/1/base/threading/platform_threa... base/threading/platform_thread_posix.cc:70: #if defined(OS_LINUX) If we're doing something different on OS_CHROMEOS we shouldn't also mess with the niceness of threads. It's not clear how it will interact with the cgroups usage.
https://codereview.chromium.org/15203009/diff/1/base/threading/platform_threa... File base/threading/platform_thread_posix.cc (right): https://codereview.chromium.org/15203009/diff/1/base/threading/platform_threa... base/threading/platform_thread_posix.cc:70: #if defined(OS_LINUX) On 2013/05/23 17:20:16, DaveMoore wrote: > If we're doing something different on OS_CHROMEOS we shouldn't also mess with > the niceness of threads. It's not clear how it will interact with the cgroups > usage. This change stops switching the niceness of the thread, instead changing its scheduler. There is also a platform side change that re-instates the restriction against setting a favorable nice level.
https://codereview.chromium.org/15203009/diff/1/base/threading/platform_threa... File base/threading/platform_thread_posix.cc (right): https://codereview.chromium.org/15203009/diff/1/base/threading/platform_threa... base/threading/platform_thread_posix.cc:70: #if defined(OS_LINUX) Then I think we should explicitly avoid ever setting the niceness on chromeos, even if SetCurrentThreadRealTime() fails. On 2013/05/23 17:24:47, dgreid wrote: > On 2013/05/23 17:20:16, DaveMoore wrote: > > If we're doing something different on OS_CHROMEOS we shouldn't also mess with > > the niceness of threads. It's not clear how it will interact with the cgroups > > usage. > > This change stops switching the niceness of the thread, instead changing its > scheduler. There is also a platform side change that re-instates the > restriction against setting a favorable nice level. >
also all this code was moved yesterday, I'll update the patch to reflect that shortly. https://codereview.chromium.org/15203009/diff/1/base/threading/platform_threa... File base/threading/platform_thread_posix.cc (right): https://codereview.chromium.org/15203009/diff/1/base/threading/platform_threa... base/threading/platform_thread_posix.cc:70: #if defined(OS_LINUX) On 2013/05/23 18:21:12, DaveMoore wrote: > Then I think we should explicitly avoid ever setting the niceness on chromeos, > even if SetCurrentThreadRealTime() fails. We still want to set the nice level if it is positive. It looks like we might want to still allow for negative values for Display? That's new to me. What do we gain by not setting the nice value in the fall through on ChromeOS? we want to do it for linux. On ChromeOS it won't happen unless the system config changes and even then it would just fail.
On Fri, May 17, 2013 at 11:53 AM, <dgreid@chromium.org> wrote: > Reviewers: DaveMoore, Chris Rogers, piman, > > Message: > This will do nothing on desktop linux unless the user has allowed the > chrome > process access to real time scheduling. For chromeos I've posted a change > to > let chrome do this. > > The platform will limit the amount of time a process can run in realtime > to 80% > of the cpu. I tested this by spinning two realtime tasks on spring, the ui > remained functional and I could close the offending tabs. > What guarantees 80%? Is it per process or total? What happens if you have 5 tabs each taking 80% on a 4-core machine? (or 3 on a 2-core machine) > What do you think? This makes a huge difference with complicated webaudio > pages > such as our new eq tuning interface. > > Description: > posix thread: Attempt to set audio threads to real time. > > Set the scheduling class of audio threads to SCHED_RR if possible. If > not, fall back to setting the nice level. Setting the thread to > real time helps when playing/recording low latency audio. Also > changing the scheduler keeps audio threads from being limited by > bandwidth limits placed on background tabs, those apply to SCHED_OTHER > threads only. The rest of the process will be limited by these, only > the audio threads will have high priority. > > This still isn't perfect, youtube will occasionally glitch on arm, > either because of video decode or maybe a thread audio is dependent on > that isn't real time. > > BUG=chromium:213118 > TEST=check audio thread priorities with ps, see that they are set. > Play audio from grooveshark or youtube in the background while loading > other tabs and listen for dropouts. > > Please review this at https://codereview.chromium.**org/15203009/<https://codereview.chromium.org/1... > > SVN Base: https://chromium.googlesource.**com/chromium/src.git@master<https://chromium.... > > Affected files: > M base/threading/platform_**thread_posix.cc > > > Index: base/threading/platform_**thread_posix.cc > diff --git a/base/threading/platform_**thread_posix.cc > b/base/threading/platform_**thread_posix.cc > index 43e58b73375a0c3273422d9c28e101**50d3835197..** > a4d23cfe0afe825882dc94fc727dab**741a10df0a 100644 > --- a/base/threading/platform_**thread_posix.cc > +++ b/base/threading/platform_**thread_posix.cc > @@ -48,6 +48,18 @@ struct ThreadParams { > ThreadPriority priority; > }; > > +#if defined(OS_LINUX) > +static int SetCurrentThreadRealTime() { > + const int kRealTimePrio = 8; > + > + struct sched_param sched_param; > + memset(&sched_param, 0, sizeof(sched_param)); > + sched_param.sched_priority = kRealTimePrio; > + > + return pthread_setschedparam(pthread_**self(), SCHED_RR, &sched_param); > +} > +#endif > + > void SetCurrentThreadPriority(**ThreadPriority priority) { > #if defined(OS_LINUX) || defined(OS_ANDROID) > switch (priority) { > @@ -56,6 +68,11 @@ void SetCurrentThreadPriority(**ThreadPriority > priority) { > break; > case kThreadPriority_RealtimeAudio: > #if defined(OS_LINUX) > + if (SetCurrentThreadRealTime() == 0) > + // Got real time priority. > + return; > + > + // Otherwise try to set an advantageous nice level. > const int kNiceSetting = -10; > // Linux isn't posix compliant with setpriority(2), it will set a > thread > // priority if it is passed a tid, not affecting the rest of the > threads > > >
On 2013/05/23 19:34:44, piman wrote: > On Fri, May 17, 2013 at 11:53 AM, <mailto:dgreid@chromium.org> wrote: > > > Reviewers: DaveMoore, Chris Rogers, piman, > > > > Message: > > This will do nothing on desktop linux unless the user has allowed the > > chrome > > process access to real time scheduling. For chromeos I've posted a change > > to > > let chrome do this. > > > > The platform will limit the amount of time a process can run in realtime > > to 80% > > of the cpu. I tested this by spinning two realtime tasks on spring, the ui > > remained functional and I could close the offending tabs. > > > > What guarantees 80%? Is it per process or total? > What happens if you have 5 tabs each taking 80% on a 4-core machine? (or 3 > on a 2-core machine) > The 80% limit is set in for the realtime scheduler. It allows the realtime scheduler to take up 80% of the cpu, then disables it and the normal scheduler takes over. On a two core machine, rt tasks will eat up to 160% of the cpu. Whether there are 2, 3, 4, or more rt tasks dividing this time, the rest of the cpu is dedicated to "normal" tasks.
https://codereview.chromium.org/15203009/diff/1/base/threading/platform_threa... File base/threading/platform_thread_posix.cc (right): https://codereview.chromium.org/15203009/diff/1/base/threading/platform_threa... base/threading/platform_thread_posix.cc:70: #if defined(OS_LINUX) It's at best misleading, and at worst will lead to hard to diagnose problems. Basically if you expect the realtime scheduling to work on Chromeos you should count on it and spit out an error if it fails. There really should be a test somewhere that confirms that this functionality works on cros. Is there? I haven't see the cros changes. I know that the cgroup stuff has been fragile as there are a few places that have to agree on how it works. On 2013/05/23 18:36:28, dgreid wrote: > On 2013/05/23 18:21:12, DaveMoore wrote: > > Then I think we should explicitly avoid ever setting the niceness on chromeos, > > even if SetCurrentThreadRealTime() fails. > > We still want to set the nice level if it is positive. It looks like we might > want to still allow for negative values for Display? That's new to me. > > What do we gain by not setting the nice value in the fall through on ChromeOS? > we want to do it for linux. On ChromeOS it won't happen unless the system config > changes and even then it would just fail. >
https://codereview.chromium.org/15203009/diff/1/base/threading/platform_threa... File base/threading/platform_thread_posix.cc (right): https://codereview.chromium.org/15203009/diff/1/base/threading/platform_threa... base/threading/platform_thread_posix.cc:70: #if defined(OS_LINUX) On 2013/05/23 20:16:31, DaveMoore wrote: > It's at best misleading, and at worst will lead to hard to diagnose problems. > Basically if you expect the realtime scheduling to work on Chromeos you should > count on it and spit out an error if it fails. There really should be a test Thanks Dave, Not sure about the consequences of this, but we should certainly be logging failures here. I'll add a return and a log. > somewhere that confirms that this functionality works on cros. Is there? I > haven't see the cros changes. I know that the cgroup stuff has been fragile as > there are a few places that have to agree on how it works. > There are no tests of any of this. If there is a way to get the thread id of an audio thread to autotest than it would be easy to verify it has the correct priority. Is that easy to get? It would be much better to add a test since this has the same three places needing to agree as cgroups does.
Testing all the way into Chrome could be tricky as you say. Perhaps a standalone test that the chronos user can set a thread to realtime priority would be enough. On 2013/05/23 20:37:49, dgreid wrote: > https://codereview.chromium.org/15203009/diff/1/base/threading/platform_threa... > File base/threading/platform_thread_posix.cc (right): > > https://codereview.chromium.org/15203009/diff/1/base/threading/platform_threa... > base/threading/platform_thread_posix.cc:70: #if defined(OS_LINUX) > On 2013/05/23 20:16:31, DaveMoore wrote: > > It's at best misleading, and at worst will lead to hard to diagnose problems. > > Basically if you expect the realtime scheduling to work on Chromeos you should > > count on it and spit out an error if it fails. There really should be a test > > Thanks Dave, > > Not sure about the consequences of this, but we should certainly be logging > failures here. I'll add a return and a log. > > > somewhere that confirms that this functionality works on cros. Is there? I > > haven't see the cros changes. I know that the cgroup stuff has been fragile as > > there are a few places that have to agree on how it works. > > > > There are no tests of any of this. If there is a way to get the thread id of an > audio thread to autotest than it would be easy to verify it has the correct > priority. Is that easy to get? > > It would be much better to add a test since this has the same three places > needing to agree as cgroups does.
On 2013/05/23 20:50:02, DaveMoore wrote: > Testing all the way into Chrome could be tricky as you say. Perhaps a standalone > test that the chronos user can set a thread to realtime priority would be > enough. I found an easy way to test that the 80% limit is set: https://gerrit.chromium.org/gerrit/#/c/56549/ but nothing easy for checking that Chrome's limit has been set. There is a prlimit(2) call in Linux, but I'm not sure if I can get autotest to do that for me. > > On 2013/05/23 20:37:49, dgreid wrote: > > > https://codereview.chromium.org/15203009/diff/1/base/threading/platform_threa... > > File base/threading/platform_thread_posix.cc (right): > > > > > https://codereview.chromium.org/15203009/diff/1/base/threading/platform_threa... > > base/threading/platform_thread_posix.cc:70: #if defined(OS_LINUX) > > On 2013/05/23 20:16:31, DaveMoore wrote: > > > It's at best misleading, and at worst will lead to hard to diagnose > problems. > > > Basically if you expect the realtime scheduling to work on Chromeos you > should > > > count on it and spit out an error if it fails. There really should be a test > > > > Thanks Dave, > > > > Not sure about the consequences of this, but we should certainly be logging > > failures here. I'll add a return and a log. > > > > > somewhere that confirms that this functionality works on cros. Is there? I > > > haven't see the cros changes. I know that the cgroup stuff has been fragile > as > > > there are a few places that have to agree on how it works. > > > > > > > There are no tests of any of this. If there is a way to get the thread id of > an > > audio thread to autotest than it would be easy to verify it has the correct > > priority. Is that easy to get? > > > > It would be much better to add a test since this has the same three places > > needing to agree as cgroups does.
lgtm
I defer to our Linux experts for correctness review. One suggestion though. https://codereview.chromium.org/15203009/diff/14001/base/threading/platform_t... File base/threading/platform_thread_linux.cc (right): https://codereview.chromium.org/15203009/diff/14001/base/threading/platform_t... base/threading/platform_thread_linux.cc:86: if (pthread_setschedparam(pthread_self(), SCHED_RR, &sched_param) == 0) { int res = pthread_setschedparam(pthread_self(), SCHED_RR, &sched_param); (D)LOG_IF(INFO, res != 0) << "Failed to set audio thread to real time"; #if defined(OS_CHROMEOS) CHECK_EQ(res, 0); #endif Instead?
https://codereview.chromium.org/15203009/diff/14001/base/threading/platform_t... File base/threading/platform_thread_linux.cc (right): https://codereview.chromium.org/15203009/diff/14001/base/threading/platform_t... base/threading/platform_thread_linux.cc:86: if (pthread_setschedparam(pthread_self(), SCHED_RR, &sched_param) == 0) { On 2013/05/28 19:15:25, DaleCurtis wrote: > int res = pthread_setschedparam(pthread_self(), SCHED_RR, &sched_param); > (D)LOG_IF(INFO, res != 0) << "Failed to set audio thread to real time"; There still needs to be a return in the success path, but keeping res around for the check seems like a good idea, I'll modify it accordingly. > #if defined(OS_CHROMEOS) > CHECK_EQ(res, 0); > #endif This will fail on ChromeOS versions that don't have the permission set (older ones). That could cause some trouble when bisecting, I've added a DCHECK, and can make it a CHECK if the consensus is that it is worth it. > > Instead?
https://codereview.chromium.org/15203009/diff/13002/base/threading/platform_t... File base/threading/platform_thread_linux.cc (right): https://codereview.chromium.org/15203009/diff/13002/base/threading/platform_t... base/threading/platform_thread_linux.cc:89: return; nit: need braces if the clause takes multiple lines. https://codereview.chromium.org/15203009/diff/13002/base/threading/platform_t... base/threading/platform_thread_linux.cc:94: DCHECK_EQ(res, 0); // RT prio should always work on ChromeOS. This prevents debugging chromeos=1 builds on the desktop. We're already logging if it fails, that's enough IMO.
https://codereview.chromium.org/15203009/diff/13002/base/threading/platform_t... File base/threading/platform_thread_linux.cc (right): https://codereview.chromium.org/15203009/diff/13002/base/threading/platform_t... base/threading/platform_thread_linux.cc:89: return; On 2013/05/28 21:58:56, piman wrote: > nit: need braces if the clause takes multiple lines. Done. https://codereview.chromium.org/15203009/diff/13002/base/threading/platform_t... base/threading/platform_thread_linux.cc:94: DCHECK_EQ(res, 0); // RT prio should always work on ChromeOS. On 2013/05/28 21:58:56, piman wrote: > This prevents debugging chromeos=1 builds on the desktop. > We're already logging if it fails, that's enough IMO. Good point, switched this back to a return.
LGTM, but I'm not an OWNER.
add some more owners
https://codereview.chromium.org/15203009/diff/18002/base/threading/platform_t... File base/threading/platform_thread_linux.cc (right): https://codereview.chromium.org/15203009/diff/18002/base/threading/platform_t... base/threading/platform_thread_linux.cc:78: #ifndef OS_NACL nit: To make this a bit more readable, how about: #if defined(OS_NACL) return; #endif // OS_NACL I didn't like having to read the nested ifdefs.... especially as the range has grown. https://codereview.chromium.org/15203009/diff/18002/base/threading/platform_t... base/threading/platform_thread_linux.cc:91: LOG(INFO) << "Failed to set audio thread to real time."; We've tended to avoid doing LOG() in favor of DLOG(). Most (all?) of the time, no one pays attention to this, and we we blowing a ton of memory with these strings. Brett went through the code removing pretty much all he could find... so it is probably better to stick with that plan. https://codereview.chromium.org/15203009/diff/18002/base/threading/platform_t... base/threading/platform_thread_linux.cc:93: return; // RT prio should always work on ChromeOS. Never set nice level. This seems strange. Are you saying the request should fail... and return an error on line 86, but you should ignore that error and act like it succeeded?
https://codereview.chromium.org/15203009/diff/18002/base/threading/platform_t... File base/threading/platform_thread_linux.cc (right): https://codereview.chromium.org/15203009/diff/18002/base/threading/platform_t... base/threading/platform_thread_linux.cc:78: #ifndef OS_NACL On 2013/05/29 02:07:04, jar wrote: > nit: To make this a bit more readable, how about: > #if defined(OS_NACL) > return; > #endif // OS_NACL > > I didn't like having to read the nested ifdefs.... especially as the range has > grown. agreed, it would be better, but it won't compile, most of the below functions don't exist there. https://codereview.chromium.org/15203009/diff/18002/base/threading/platform_t... base/threading/platform_thread_linux.cc:91: LOG(INFO) << "Failed to set audio thread to real time."; On 2013/05/29 02:07:04, jar wrote: > We've tended to avoid doing LOG() in favor of DLOG(). Most (all?) of the time, > no one pays attention to this, and we we blowing a ton of memory with these > strings. Brett went through the code removing pretty much all he could find... > so it is probably better to stick with that plan. I'm OK removing this, it's probably not worth the string since it shouldn't ever fail. It would leave us without the any insight into the failure, but I can investigate a way to list realtime threads in chromeos feedback that would be a good indicator. Sound like a good solution? https://codereview.chromium.org/15203009/diff/18002/base/threading/platform_t... base/threading/platform_thread_linux.cc:93: return; // RT prio should always work on ChromeOS. Never set nice level. On 2013/05/29 02:07:04, jar wrote: > This seems strange. Are you saying the request should fail... and return an > error on line 86, but you should ignore that error and act like it succeeded? On linux the request will fail unless the user has explicitly allowed chrome RT thread access then attempt to set the nice level as a fallback. Per comments on patch set one, the fallback behavior wasn't desired for chrome os we bail here instead. I'm not attached to this part at all. This is code that should never run. Do you think it would be better without it?
https://codereview.chromium.org/15203009/diff/18002/base/threading/platform_t... File base/threading/platform_thread_linux.cc (right): https://codereview.chromium.org/15203009/diff/18002/base/threading/platform_t... base/threading/platform_thread_linux.cc:78: #ifndef OS_NACL On 2013/05/29 18:52:34, dgreid wrote: > On 2013/05/29 02:07:04, jar wrote: > > nit: To make this a bit more readable, how about: > > #if defined(OS_NACL) > > return; > > #endif // OS_NACL > > > > I didn't like having to read the nested ifdefs.... especially as the range > has > > grown. > > agreed, it would be better, but it won't compile, most of the below functions > don't exist there. > OK... Please add comments after each #endif to indicate what the matching ifdef was. https://codereview.chromium.org/15203009/diff/18002/base/threading/platform_t... base/threading/platform_thread_linux.cc:91: LOG(INFO) << "Failed to set audio thread to real time."; On 2013/05/29 18:52:34, dgreid wrote: > On 2013/05/29 02:07:04, jar wrote: > > We've tended to avoid doing LOG() in favor of DLOG(). Most (all?) of the > time, > > no one pays attention to this, and we we blowing a ton of memory with these > > strings. Brett went through the code removing pretty much all he could > find... > > so it is probably better to stick with that plan. > > I'm OK removing this, it's probably not worth the string since it shouldn't ever > fail. It would leave us without the any insight into the failure, but I can > investigate a way to list realtime threads in chromeos feedback that would be a > good indicator. Sound like a good solution? WFM https://codereview.chromium.org/15203009/diff/18002/base/threading/platform_t... base/threading/platform_thread_linux.cc:93: return; // RT prio should always work on ChromeOS. Never set nice level. On 2013/05/29 18:52:34, dgreid wrote: > On 2013/05/29 02:07:04, jar wrote: > > This seems strange. Are you saying the request should fail... and return an > > error on line 86, but you should ignore that error and act like it succeeded? > > On linux the request will fail unless the user has explicitly allowed chrome RT > thread access then attempt to set the nice level as a fallback. Per comments on > patch set one, the fallback behavior wasn't desired for chrome os we bail here > instead. > > I'm not attached to this part at all. This is code that should never run. Do > you think it would be better without it? If you think it will never run... less code seems better. ...unless there is a catastrophic problem with the "default" behavior. It sounds like they have problems when they reach this point... and adding ifdefs to little advantage seems like a waste.
Thanks for the comments, I also found a way to add a test that the limits are set so that the calls will never fail on ChromeOS (as long as the new test is passing). I should have that test posted by the end of the day. https://codereview.chromium.org/15203009/diff/18002/base/threading/platform_t... File base/threading/platform_thread_linux.cc (right): https://codereview.chromium.org/15203009/diff/18002/base/threading/platform_t... base/threading/platform_thread_linux.cc:78: #ifndef OS_NACL On 2013/05/30 18:15:25, jar wrote: > On 2013/05/29 18:52:34, dgreid wrote: > > On 2013/05/29 02:07:04, jar wrote: > > > nit: To make this a bit more readable, how about: > > > #if defined(OS_NACL) > > > return; > > > #endif // OS_NACL > > > > > > I didn't like having to read the nested ifdefs.... especially as the range > > has > > > grown. > > > > agreed, it would be better, but it won't compile, most of the below functions > > don't exist there. > > > > OK... > Please add comments after each #endif to indicate what the matching ifdef was. Done. https://codereview.chromium.org/15203009/diff/18002/base/threading/platform_t... base/threading/platform_thread_linux.cc:91: LOG(INFO) << "Failed to set audio thread to real time."; On 2013/05/30 18:15:25, jar wrote: > On 2013/05/29 18:52:34, dgreid wrote: > > On 2013/05/29 02:07:04, jar wrote: > > > We've tended to avoid doing LOG() in favor of DLOG(). Most (all?) of the > > time, > > > no one pays attention to this, and we we blowing a ton of memory with these > > > strings. Brett went through the code removing pretty much all he could > > find... > > > so it is probably better to stick with that plan. > > > > I'm OK removing this, it's probably not worth the string since it shouldn't > ever > > fail. It would leave us without the any insight into the failure, but I can > > investigate a way to list realtime threads in chromeos feedback that would be > a > > good indicator. Sound like a good solution? > > WFM Great, I've also found a way to test that this and the below code never run on ChromeOS.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dgreid@chromium.org/15203009/34001
Message was sent while issue was closed.
Change committed as 203896 |
