Lower the priority of environment and process data collection.

Lower the priority of environment and process data collection.

BUG=

Committed: https://crrev.com/5e3721ac64f5951d1844d7308ce06c4786788565
Cr-Commit-Position: refs/heads/master@{#365537}

[proberge, 2015-12-07 22:13:28]

Description was changed from

==========
Lower the priority of environment and process data collection.

BUG=
==========

to

==========
Lower the priority of environment and process data collection.

BUG=
==========

[proberge, 2015-12-07 22:13:28]

proberge@chromium.org changed reviewers:
+ gab@chromium.org, grt@chromium.org

[gab, 2015-12-08 16:21:33]

lgtm if grt is okay with adding a dependency on the specific runtime environment
inside this environment which previously had no dependency on where it was
running from (if not, maybe priority toggling should instead be in the caller of
this method -- or however far up one needs to go to be allowed to depend on this
context).

https://codereview.chromium.org/1505813003/diff/1/chrome/browser/safe_browsin...
File
chrome/browser/safe_browsing/incident_reporting/environment_data_collection.cc
(right):

https://codereview.chromium.org/1505813003/diff/1/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/incident_reporting/environment_data_collection.cc:66:
// Lower priority for this task.
If we're going to do this I think we should add:

// Toggling priority only makes sense in a thread pool.
DCHECK(content::BrowserThread::GetBlockingPool()->RunsTasksOnCurrentThread());

here as IMO it only makes sense to do this in a pool (on a message loop this
would preempt other tasks -- arguably it could also in a thread pool if this
becomes the norm but it isn't just yet and the new task_scheduler API should
provide a sane way to do this before we get there).

[proberge, 2015-12-08 16:39:03]

https://codereview.chromium.org/1505813003/diff/1/chrome/browser/safe_browsin...
File
chrome/browser/safe_browsing/incident_reporting/environment_data_collection.cc
(right):

https://codereview.chromium.org/1505813003/diff/1/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/incident_reporting/environment_data_collection.cc:66:
// Lower priority for this task.
On 2015/12/08 16:21:33, gab wrote:
> If we're going to do this I think we should add:
> 
> // Toggling priority only makes sense in a thread pool.
> DCHECK(content::BrowserThread::GetBlockingPool()->RunsTasksOnCurrentThread());
> 
> here as IMO it only makes sense to do this in a pool (on a message loop this
> would preempt other tasks -- arguably it could also in a thread pool if this
> becomes the norm but it isn't just yet and the new task_scheduler API should
> provide a sane way to do this before we get there).

Done.

[grt (UTC plus 2), 2015-12-09 14:35:26]

https://codereview.chromium.org/1505813003/diff/20001/chrome/browser/safe_bro...
File
chrome/browser/safe_browsing/incident_reporting/environment_data_collection.cc
(right):

https://codereview.chromium.org/1505813003/diff/20001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/incident_reporting/environment_data_collection.cc:68:
DCHECK(content::BrowserThread::GetBlockingPool()->RunsTasksOnCurrentThread());
i don't like the dependence on BrowserThread here. gab: are you okay with
changing this to:
  DCHECK(base::SequencedWorkerPool::GetWorkerPoolForCurrentThread());

an alternative is to make a generic "run a callback at bg priority" wrapper that
is used by the IncidentReportingService for this specific task. the same wrapper
could be used for any other tasks elsewhere that could also benefit from running
at bg priority.

[gab, 2015-12-09 21:51:01]

https://codereview.chromium.org/1505813003/diff/20001/chrome/browser/safe_bro...
File
chrome/browser/safe_browsing/incident_reporting/environment_data_collection.cc
(right):

https://codereview.chromium.org/1505813003/diff/20001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/incident_reporting/environment_data_collection.cc:68:
DCHECK(content::BrowserThread::GetBlockingPool()->RunsTasksOnCurrentThread());
On 2015/12/09 14:35:25, grt (no reviews until Dec 14) wrote:
> i don't like the dependence on BrowserThread here. gab: are you okay with
> changing this to:
>   DCHECK(base::SequencedWorkerPool::GetWorkerPoolForCurrentThread());

Yea that works :-) (interestingly this call was recently added and not for this
purpose but nice to see that we can now make this check without depending on
BrowserThread :-))

Is this function called even when there is no incident? i.e. how likely is a
developer to hit this and realize they've broken something if this was to ever
run on a MessageLoop thread?

> 
> an alternative is to make a generic "run a callback at bg priority" wrapper
that
> is used by the IncidentReportingService for this specific task. the same
wrapper
> could be used for any other tasks elsewhere that could also benefit from
running
> at bg priority.

Yea but not sure I want a generic solution as (1) if too many callers do this we
could end up with all threads backgrounded which may hurt some other things
(e.g. async load of pref file for secondary profile is done on BlockingPool
today yet is a high priority task -- addressed by TaskScheduler API) and (2)
this will be supported by the TaskScheduler API which thus reduces the need to
introduce something for the long term here.

[proberge, 2015-12-10 21:00:57]

https://codereview.chromium.org/1505813003/diff/20001/chrome/browser/safe_bro...
File
chrome/browser/safe_browsing/incident_reporting/environment_data_collection.cc
(right):

https://codereview.chromium.org/1505813003/diff/20001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/incident_reporting/environment_data_collection.cc:68:
DCHECK(content::BrowserThread::GetBlockingPool()->RunsTasksOnCurrentThread());
On 2015/12/09 21:51:01, gab wrote:
> On 2015/12/09 14:35:25, grt (no reviews until Dec 14) wrote:
> > i don't like the dependence on BrowserThread here. gab: are you okay with
> > changing this to:
> >   DCHECK(base::SequencedWorkerPool::GetWorkerPoolForCurrentThread());
> 
> Yea that works :-) (interestingly this call was recently added and not for
this
> purpose but nice to see that we can now make this check without depending on
> BrowserThread :-))
> 
> Is this function called even when there is no incident? i.e. how likely is a
> developer to hit this and realize they've broken something if this was to ever
> run on a MessageLoop thread?

This doesn't run if there's no incident, but most developers will trigger this
code as there's a BinaryIncident for development builds.

> > 
> > an alternative is to make a generic "run a callback at bg priority" wrapper
> that
> > is used by the IncidentReportingService for this specific task. the same
> wrapper
> > could be used for any other tasks elsewhere that could also benefit from
> running
> > at bg priority.
> 
> Yea but not sure I want a generic solution as (1) if too many callers do this
we
> could end up with all threads backgrounded which may hurt some other things
> (e.g. async load of pref file for secondary profile is done on BlockingPool
> today yet is a high priority task -- addressed by TaskScheduler API) and (2)
> this will be supported by the TaskScheduler API which thus reduces the need to
> introduce something for the long term here.

[proberge, 2015-12-10 21:11:38]

The CQ bit was checked by proberge@chromium.org

[proberge, 2015-12-10 21:11:39]

The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org
Link to the patchset: https://codereview.chromium.org/1505813003/#ps60001
(title: "Update include")

[commit-bot: I haz the power, 2015-12-10 21:12:51]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1505813003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1505813003/60001

[commit-bot: I haz the power, 2015-12-10 21:27:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-10 21:27:43]

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...)

[proberge, 2015-12-10 21:28:52]

On 2015/12/10 21:27:43, commit-bot: I haz the power wrote:
> 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...)

Looks like I'm missing a LGTM from @grt

[grt (UTC plus 2), 2015-12-14 21:10:25]

lgtm with a paranoid suggestion

https://codereview.chromium.org/1505813003/diff/60001/chrome/browser/safe_bro...
File
chrome/browser/safe_browsing/incident_reporting/environment_data_collection.cc
(right):

https://codereview.chromium.org/1505813003/diff/60001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/incident_reporting/environment_data_collection.cc:96:
base::PlatformThread::SetCurrentThreadPriority(previous_priority);
it would be hella bad if someone accidentally added a "return;" before this.
would you mind using base::ScopedClosureRunner up on line 73 to ensure that the
priority is always reverted? does this work:
  base::ScopedClosureRunner restore_priority(
      base::Bind(&base::PlatformThread::SetCurrentThreadPriority,
                 previous_priority));

[gab, 2015-12-15 16:49:38]

https://codereview.chromium.org/1505813003/diff/60001/chrome/browser/safe_bro...
File
chrome/browser/safe_browsing/incident_reporting/environment_data_collection.cc
(right):

https://codereview.chromium.org/1505813003/diff/60001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/incident_reporting/environment_data_collection.cc:96:
base::PlatformThread::SetCurrentThreadPriority(previous_priority);
On 2015/12/14 21:10:25, grt wrote:
> it would be hella bad if someone accidentally added a "return;" before this.
> would you mind using base::ScopedClosureRunner up on line 73 to ensure that
the
> priority is always reverted? does this work:
>   base::ScopedClosureRunner restore_priority(
>       base::Bind(&base::PlatformThread::SetCurrentThreadPriority,
>                  previous_priority));

That sounds like a great idea to me, also keeps all priority related code at the
top :-). I felt like adding a new scoper for thread priority just for this CL
was overkill so I had refrained from asking but I didn't know about
ScopedClosureRunner :-), very useful for generic scopers!).

[proberge, 2015-12-16 15:19:38]

https://codereview.chromium.org/1505813003/diff/60001/chrome/browser/safe_bro...
File
chrome/browser/safe_browsing/incident_reporting/environment_data_collection.cc
(right):

https://codereview.chromium.org/1505813003/diff/60001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/incident_reporting/environment_data_collection.cc:96:
base::PlatformThread::SetCurrentThreadPriority(previous_priority);
On 2015/12/15 16:49:38, gab wrote:
> On 2015/12/14 21:10:25, grt wrote:
> > it would be hella bad if someone accidentally added a "return;" before this.
> > would you mind using base::ScopedClosureRunner up on line 73 to ensure that
> the
> > priority is always reverted? does this work:
> >   base::ScopedClosureRunner restore_priority(
> >       base::Bind(&base::PlatformThread::SetCurrentThreadPriority,
> >                  previous_priority));
> 
> That sounds like a great idea to me, also keeps all priority related code at
the
> top :-). I felt like adding a new scoper for thread priority just for this CL
> was overkill so I had refrained from asking but I didn't know about
> ScopedClosureRunner :-), very useful for generic scopers!).

Done.

[proberge, 2015-12-16 15:19:46]

The CQ bit was checked by proberge@chromium.org

[proberge, 2015-12-16 15:19:46]

The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org,
grt@chromium.org
Link to the patchset: https://codereview.chromium.org/1505813003/#ps120001
(title: "Move comment")

[commit-bot: I haz the power, 2015-12-16 15:20:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1505813003/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1505813003/120001

[commit-bot: I haz the power, 2015-12-16 16:04:57]

Description was changed from

==========
Lower the priority of environment and process data collection.

BUG=
==========

to

==========
Lower the priority of environment and process data collection.

BUG=
==========

[commit-bot: I haz the power, 2015-12-16 16:05:00]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-12-16 16:06:47]

Description was changed from

==========
Lower the priority of environment and process data collection.

BUG=
==========

to

==========
Lower the priority of environment and process data collection.

BUG=

Committed: https://crrev.com/5e3721ac64f5951d1844d7308ce06c4786788565
Cr-Commit-Position: refs/heads/master@{#365537}
==========

[commit-bot: I haz the power, 2015-12-16 16:06:48]

Patchset 7 (id:??) landed as
https://crrev.com/5e3721ac64f5951d1844d7308ce06c4786788565
Cr-Commit-Position: refs/heads/master@{#365537}