Add ModuleInspector

Add ModuleInspector

ModuleInspector maintains a queue of modules to inspect and process
them on a background task runner one by one in order to minimize the
resource usage of this expensive operation.

BUG=690173
Review-Url: https://codereview.chromium.org/2721503003
Cr-Commit-Position: refs/heads/master@{#455107}
Committed: https://chromium.googlesource.com/chromium/src/+/f693974068bced80bd750931026f248e5b9c6dff

[Patrick Monette, 2017-02-27 20:38:39]

Patchset #1 (id:1) has been deleted

[Patrick Monette, 2017-02-27 20:41:35]

Patchset #1 (id:20001) has been deleted

[Patrick Monette, 2017-02-27 20:42:05]

pmonette@chromium.org changed reviewers:
+ chrisha@chromium.org

[Patrick Monette, 2017-02-27 20:42:05]

Hey, follow up CL that uses InspectModule. PTAL

[chrisha, 2017-03-01 16:03:38]

https://codereview.chromium.org/2721503003/diff/40001/chrome/browser/conflict...
File chrome/browser/conflicts/module_database_win.h (right):

https://codereview.chromium.org/2721503003/diff/40001/chrome/browser/conflict...
chrome/browser/conflicts/module_database_win.h:27: class ModuleDatabase : public
ModuleInspector::Delegate {
Is delegate the right pattern here? A delegate does work on behalf of a class,
and is injected.

This seems more like we're an observer. And given that there's a single function
(OnModuleInspected), maybe we should just use a callback?

https://codereview.chromium.org/2721503003/diff/40001/chrome/browser/conflict...
File chrome/browser/conflicts/module_inspector_win.h (right):

https://codereview.chromium.org/2721503003/diff/40001/chrome/browser/conflict...
chrome/browser/conflicts/module_inspector_win.h:17: // to the delegate.
Prefer callback to delegate?

https://codereview.chromium.org/2721503003/diff/40001/chrome/browser/conflict...
chrome/browser/conflicts/module_inspector_win.h:21: // is possible to increase
the priority level of these task by calling
these tasks*

https://codereview.chromium.org/2721503003/diff/40001/chrome/browser/conflict...
chrome/browser/conflicts/module_inspector_win.h:22: //
IncreaseInspectionPriority().
Can you comment on the threading model? That this class doesn't provide locking,
and the expectations on what gets called where? On what thread will the
callbacks be posted? etc

https://codereview.chromium.org/2721503003/diff/40001/chrome/browser/conflict...
chrome/browser/conflicts/module_inspector_win.h:44: void
StartInspectingModule();
StartInspectingModules? (plural)

This will kick off inspection of all queued modules, not just the first one.

https://codereview.chromium.org/2721503003/diff/40001/chrome/browser/conflict...
chrome/browser/conflicts/module_inspector_win.h:58: // The traits used on the
task that inspects the modules. It originally start
starts*

https://codereview.chromium.org/2721503003/diff/40001/chrome/browser/conflict...
chrome/browser/conflicts/module_inspector_win.h:61: base::TaskTraits
inspection_task_traits_;
+1 to using the new task scheduler!

https://codereview.chromium.org/2721503003/diff/40001/chrome/browser/conflict...
chrome/browser/conflicts/module_inspector_win.h:64: // where people keep there
files, c:\windows vs. d:\windows, etc.
their*

... and localization

[Patrick Monette, 2017-03-03 20:56:59]

Patchset #2 (id:60001) has been deleted

[Patrick Monette, 2017-03-03 22:16:30]

PTAnL!

https://codereview.chromium.org/2721503003/diff/40001/chrome/browser/conflict...
File chrome/browser/conflicts/module_database_win.h (right):

https://codereview.chromium.org/2721503003/diff/40001/chrome/browser/conflict...
chrome/browser/conflicts/module_database_win.h:27: class ModuleDatabase : public
ModuleInspector::Delegate {
On 2017/03/01 16:03:38, chrisha wrote:
> Is delegate the right pattern here? A delegate does work on behalf of a class,
> and is injected.
> 
> This seems more like we're an observer. And given that there's a single
function
> (OnModuleInspected), maybe we should just use a callback?

Using a callback sgtm. Done.

https://codereview.chromium.org/2721503003/diff/40001/chrome/browser/conflict...
File chrome/browser/conflicts/module_inspector_win.h (right):

https://codereview.chromium.org/2721503003/diff/40001/chrome/browser/conflict...
chrome/browser/conflicts/module_inspector_win.h:17: // to the delegate.
On 2017/03/01 16:03:38, chrisha wrote:
> Prefer callback to delegate?

Done.

https://codereview.chromium.org/2721503003/diff/40001/chrome/browser/conflict...
chrome/browser/conflicts/module_inspector_win.h:21: // is possible to increase
the priority level of these task by calling
On 2017/03/01 16:03:38, chrisha wrote:
> these tasks*

Done.

https://codereview.chromium.org/2721503003/diff/40001/chrome/browser/conflict...
chrome/browser/conflicts/module_inspector_win.h:22: //
IncreaseInspectionPriority().
On 2017/03/01 16:03:38, chrisha wrote:
> Can you comment on the threading model? That this class doesn't provide
locking,
> and the expectations on what gets called where? On what thread will the
> callbacks be posted? etc

Done.

https://codereview.chromium.org/2721503003/diff/40001/chrome/browser/conflict...
chrome/browser/conflicts/module_inspector_win.h:44: void
StartInspectingModule();
On 2017/03/01 16:03:38, chrisha wrote:
> StartInspectingModules? (plural)
> 
> This will kick off inspection of all queued modules, not just the first one.

I'd leave it as-is. This gets called once per module, and it's not part of the
public API.

https://codereview.chromium.org/2721503003/diff/40001/chrome/browser/conflict...
chrome/browser/conflicts/module_inspector_win.h:58: // The traits used on the
task that inspects the modules. It originally start
On 2017/03/01 16:03:38, chrisha wrote:
> starts*

Done.

https://codereview.chromium.org/2721503003/diff/40001/chrome/browser/conflict...
chrome/browser/conflicts/module_inspector_win.h:61: base::TaskTraits
inspection_task_traits_;
On 2017/03/01 16:03:38, chrisha wrote:
> +1 to using the new task scheduler!

Works great!

https://codereview.chromium.org/2721503003/diff/40001/chrome/browser/conflict...
chrome/browser/conflicts/module_inspector_win.h:64: // where people keep there
files, c:\windows vs. d:\windows, etc.
On 2017/03/01 16:03:38, chrisha wrote:
> their*
> 
> ... and localization

Done.

[chrisha, 2017-03-04 16:09:07]

lgtm!

https://codereview.chromium.org/2721503003/diff/80001/chrome/browser/conflict...
File chrome/browser/conflicts/module_info_win.cc (right):

https://codereview.chromium.org/2721503003/diff/80001/chrome/browser/conflict...
chrome/browser/conflicts/module_info_win.cc:74: std::tie(mik.module_path,
mik.module_size, mik.module_time_date_stamp);
TIL!

https://codereview.chromium.org/2721503003/diff/80001/chrome/browser/conflict...
File chrome/browser/conflicts/module_inspector_win.cc (right):

https://codereview.chromium.org/2721503003/diff/80001/chrome/browser/conflict...
chrome/browser/conflicts/module_inspector_win.cc:55:
base::PostTaskWithTraitsAndReplyWithResult(
That's convenient! Wasn't aware of this variant of PostTask.

https://codereview.chromium.org/2721503003/diff/80001/chrome/browser/conflict...
File chrome/browser/conflicts/module_inspector_win.h (right):

https://codereview.chromium.org/2721503003/diff/80001/chrome/browser/conflict...
chrome/browser/conflicts/module_inspector_win.h:20: // constructor on the
SequencedTaskRunner where it was created on.
where it was created.

or

on which it was created.

https://codereview.chromium.org/2721503003/diff/80001/chrome/browser/conflict...
chrome/browser/conflicts/module_inspector_win.h:59: // The modules are put in
queue until they get are sent for inspection.
remove "get"

[Patrick Monette, 2017-03-06 16:51:54]

Thanks!

https://codereview.chromium.org/2721503003/diff/80001/chrome/browser/conflict...
File chrome/browser/conflicts/module_inspector_win.h (right):

https://codereview.chromium.org/2721503003/diff/80001/chrome/browser/conflict...
chrome/browser/conflicts/module_inspector_win.h:20: // constructor on the
SequencedTaskRunner where it was created on.
On 2017/03/04 16:09:07, chrisha wrote:
> where it was created.
> 
> or
> 
> on which it was created.

Done.

https://codereview.chromium.org/2721503003/diff/80001/chrome/browser/conflict...
chrome/browser/conflicts/module_inspector_win.h:59: // The modules are put in
queue until they get are sent for inspection.
On 2017/03/04 16:09:07, chrisha wrote:
> remove "get"

Done.

[Patrick Monette, 2017-03-06 23:09:04]

The CQ bit was checked by pmonette@chromium.org

[Patrick Monette, 2017-03-06 23:09:04]

The patchset sent to the CQ was uploaded after l-g-t-m from chrisha@chromium.org
Link to the patchset: https://codereview.chromium.org/2721503003/#ps120001
(title: "Merge")

[commit-bot: I haz the power, 2017-03-06 23:09:47]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Patrick Monette, 2017-03-06 23:15:41]

The CQ bit was unchecked by pmonette@chromium.org

[Patrick Monette, 2017-03-06 23:17:20]

The CQ bit was checked by pmonette@chromium.org

[Patrick Monette, 2017-03-06 23:17:20]

The patchset sent to the CQ was uploaded after l-g-t-m from chrisha@chromium.org
Link to the patchset: https://codereview.chromium.org/2721503003/#ps140001
(title: "merge")

[commit-bot: I haz the power, 2017-03-06 23:18:01]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-07 00:42:17]

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

[commit-bot: I haz the power, 2017-03-07 00:42:18]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Patrick Monette, 2017-03-07 04:28:31]

The CQ bit was checked by pmonette@chromium.org

[commit-bot: I haz the power, 2017-03-07 04:29:28]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-07 07:53:05]

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

[commit-bot: I haz the power, 2017-03-07 07:53:06]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Patrick Monette, 2017-03-07 15:47:19]

The CQ bit was checked by pmonette@chromium.org

[commit-bot: I haz the power, 2017-03-07 15:47:50]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-07 17:15:35]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1488901639431030,
"parent_rev": "a11672fb4c998040224d7dc97e5bdc05d46a4145", "commit_rev":
"f693974068bced80bd750931026f248e5b9c6dff"}

[commit-bot: I haz the power, 2017-03-07 17:16:21]

Description was changed from

==========
Add ModuleInspector

ModuleInspector maintains a queue of modules to inspect and process
them on a background task runner one by one in order to minimize the
resource usage of this expensive operation.

BUG=690173
==========

to

==========
Add ModuleInspector

ModuleInspector maintains a queue of modules to inspect and process
them on a background task runner one by one in order to minimize the
resource usage of this expensive operation.

BUG=690173

Review-Url: https://codereview.chromium.org/2721503003
Cr-Commit-Position: refs/heads/master@{#455107}
Committed:
https://chromium.googlesource.com/chromium/src/+/f693974068bced80bd750931026f...
==========

[commit-bot: I haz the power, 2017-03-07 17:16:25]

Committed patchset #5 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/f693974068bced80bd750931026f...