Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2420133002:
Make module enumeration a low priority background task. (Closed)
DescriptionMake module enumeration a low priority background task.
This makes the default mode of module enumeration one that is performed slowly and continuously in the blocking pool post startup. This addresses the CPU/battery regression it recently introduced.
This also moves definitions for some preexisting privately defined histograms to the public repository.
BUG=644258
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng
Committed: https://crrev.com/6949a31c3ccf416a0e3423bb336e61165fcf5388
Cr-Commit-Position: refs/heads/master@{#426063}
Patch Set 1 #Patch Set 2 : Moar comments! #
Total comments: 14
Patch Set 3 : Address review comments. #Patch Set 4 : Fix small bug. #
Messages
Total messages: 33 (15 generated)
Description was changed from ========== Make module enumeration a low priority background task. This makes the default mode of module enumeration one that is performed slowly and continuously in the blocking pool post startup. This addresses the CPU/battery regression it recently introduced. BUG=644258 ========== to ========== Make module enumeration a low priority background task. This makes the default mode of module enumeration one that is performed slowly and continuously in the blocking pool post startup. This addresses the CPU/battery regression it recently introduced. BUG=644258 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ==========
chrisha@chromium.org changed reviewers: + pmonette@chromium.org
Pat, care to take a look?
chrisha@chromium.org changed reviewers: + rkaplow@chromium.org
+rkaplow for histograms.xml
https://codereview.chromium.org/2420133002/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2420133002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:6843: +</histogram> I added this one. The rest already existed, but in the internal histograms file.
Is there an easy way to add some unittests? https://codereview.chromium.org/2420133002/diff/20001/chrome/browser/win/enum... File chrome/browser/win/enumerate_modules_model.cc (right): https://codereview.chromium.org/2420133002/diff/20001/chrome/browser/win/enum... chrome/browser/win/enumerate_modules_model.cc:66: const base::TimeDelta kDefaultPerModuleDelay = base::TimeDelta::FromSeconds(1); constexpr https://codereview.chromium.org/2420133002/diff/20001/chrome/browser/win/enum... chrome/browser/win/enumerate_modules_model.cc:494: if (per_module_delay_.is_zero()) { Reverse the conditional for simpler code? (Let you remove the else{} and continue; statement.) https://codereview.chromium.org/2420133002/diff/20001/chrome/browser/win/enum... chrome/browser/win/enumerate_modules_model.cc:835: // Windows its an atomic write. Is this really safe?
https://codereview.chromium.org/2420133002/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2420133002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:6807: +<histogram name="Conflicts.ConfirmedBadModules"> should have unit="modules" or something https://codereview.chromium.org/2420133002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:6814: +<histogram name="Conflicts.EnumerateLoadedModules" units="ms"> I wish the name was EnumerateLoadedModulesTime or something, but not worth at this point I guess https://codereview.chromium.org/2420133002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:6854: +<histogram name="Conflicts.SuspectedBadModules"> unit here too
On 2016/10/14 22:45:55, Patrick Monette wrote: > Is there an easy way to add some unittests? No "easy" way, as this is really old code that doesn't have any existing unittests. But is possible... let me take a deeper look.
https://codereview.chromium.org/2420133002/diff/20001/chrome/browser/win/enum... File chrome/browser/win/enumerate_modules_model.cc (right): https://codereview.chromium.org/2420133002/diff/20001/chrome/browser/win/enum... chrome/browser/win/enumerate_modules_model.cc:66: const base::TimeDelta kDefaultPerModuleDelay = base::TimeDelta::FromSeconds(1); On 2016/10/14 22:45:55, Patrick Monette wrote: > constexpr Done. https://codereview.chromium.org/2420133002/diff/20001/chrome/browser/win/enum... chrome/browser/win/enumerate_modules_model.cc:494: if (per_module_delay_.is_zero()) { On 2016/10/14 22:45:55, Patrick Monette wrote: > Reverse the conditional for simpler code? (Let you remove the else{} and > continue; statement.) Done. https://codereview.chromium.org/2420133002/diff/20001/chrome/browser/win/enum... chrome/browser/win/enumerate_modules_model.cc:835: // Windows its an atomic write. On 2016/10/14 22:45:55, Patrick Monette wrote: > Is this really safe? Yeah, this is a standard trick. The variable is being used as a one-way switch. Writing can only happen on one thread (UI), and reading can only happen on one thread (a thread in the worker-pool). A 4-byte write/read is atomic, so the reader will only ever see the original value, or the zero value, and nothing else is possible. (We'd require a lock if we had multiple threads doing read-update-write logic, as they'd be racing with each other. It's even safe to have multiple writer threads as long as the value they're writing doesn't depend on the current value of the variable.) https://codereview.chromium.org/2420133002/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2420133002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:6807: +<histogram name="Conflicts.ConfirmedBadModules"> On 2016/10/17 14:10:15, rkaplow wrote: > should have unit="modules" or something Done. https://codereview.chromium.org/2420133002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:6814: +<histogram name="Conflicts.EnumerateLoadedModules" units="ms"> On 2016/10/17 14:10:15, rkaplow wrote: > I wish the name was EnumerateLoadedModulesTime or something, but not worth at > this point I guess Acknowledged. https://codereview.chromium.org/2420133002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:6854: +<histogram name="Conflicts.SuspectedBadModules"> On 2016/10/17 14:10:15, rkaplow wrote: > unit here too Done.
lgtm https://codereview.chromium.org/2420133002/diff/20001/chrome/browser/win/enum... File chrome/browser/win/enumerate_modules_model.cc (right): https://codereview.chromium.org/2420133002/diff/20001/chrome/browser/win/enum... chrome/browser/win/enumerate_modules_model.cc:835: // Windows its an atomic write. On 2016/10/17 15:32:21, chrisha (slow) wrote: > On 2016/10/14 22:45:55, Patrick Monette wrote: > > Is this really safe? > > Yeah, this is a standard trick. The variable is being used as a one-way switch. > Writing can only happen on one thread (UI), and reading can only happen on one > thread (a thread in the worker-pool). A 4-byte write/read is atomic, so the > reader will only ever see the original value, or the zero value, and nothing > else is possible. > > (We'd require a lock if we had multiple threads doing read-update-write logic, > as they'd be racing with each other. It's even safe to have multiple writer > threads as long as the value they're writing doesn't depend on the current value > of the variable.) Acknowledged.
lgtm since it's slightly confusing, I would add in the CL description that this adds the histogram descriptions for previously added histograms that were never added to the xml
Description was changed from ========== Make module enumeration a low priority background task. This makes the default mode of module enumeration one that is performed slowly and continuously in the blocking pool post startup. This addresses the CPU/battery regression it recently introduced. BUG=644258 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ========== to ========== Make module enumeration a low priority background task. This makes the default mode of module enumeration one that is performed slowly and continuously in the blocking pool post startup. This addresses the CPU/battery regression it recently introduced. This also moves definitions for some preexisting privately defined histograms to the public repository. BUG=644258 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ==========
The CQ bit was checked by chrisha@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Exceeded global retry quota
chrisha@chromium.org changed reviewers: + pkasting@chromium.org
pkasting: Care to take a look at conflicts_ui.cc?
LGTM
The CQ bit was checked by chrisha@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by chrisha@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pmonette@chromium.org, rkaplow@chromium.org, pkasting@chromium.org Link to the patchset: https://chromiumcodereview.appspot.com/2420133002/#ps60001 (title: "Fix small bug.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Make module enumeration a low priority background task. This makes the default mode of module enumeration one that is performed slowly and continuously in the blocking pool post startup. This addresses the CPU/battery regression it recently introduced. This also moves definitions for some preexisting privately defined histograms to the public repository. BUG=644258 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ========== to ========== Make module enumeration a low priority background task. This makes the default mode of module enumeration one that is performed slowly and continuously in the blocking pool post startup. This addresses the CPU/battery regression it recently introduced. This also moves definitions for some preexisting privately defined histograms to the public repository. BUG=644258 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Make module enumeration a low priority background task. This makes the default mode of module enumeration one that is performed slowly and continuously in the blocking pool post startup. This addresses the CPU/battery regression it recently introduced. This also moves definitions for some preexisting privately defined histograms to the public repository. BUG=644258 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ========== to ========== Make module enumeration a low priority background task. This makes the default mode of module enumeration one that is performed slowly and continuously in the blocking pool post startup. This addresses the CPU/battery regression it recently introduced. This also moves definitions for some preexisting privately defined histograms to the public repository. BUG=644258 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ==========
Description was changed from ========== Make module enumeration a low priority background task. This makes the default mode of module enumeration one that is performed slowly and continuously in the blocking pool post startup. This addresses the CPU/battery regression it recently introduced. This also moves definitions for some preexisting privately defined histograms to the public repository. BUG=644258 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ========== to ========== Make module enumeration a low priority background task. This makes the default mode of module enumeration one that is performed slowly and continuously in the blocking pool post startup. This addresses the CPU/battery regression it recently introduced. This also moves definitions for some preexisting privately defined histograms to the public repository. BUG=644258 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng Committed: https://chromium.googlesource.com/chromium/src/+/6949a31c3ccf416a0e3423bb336e... Cr-Commit-Position: refs/heads/master@{#426063} ==========
(Manually adding missing CQ information.) Patchset 4 (id:60001) landed as https://chromium.googlesource.com/chromium/src/+/6949a31c3ccf416a0e3423bb336e... Cr-Commit-Position: refs/heads/master@{#426063}
Message was sent while issue was closed.
Description was changed from ========== Make module enumeration a low priority background task. This makes the default mode of module enumeration one that is performed slowly and continuously in the blocking pool post startup. This addresses the CPU/battery regression it recently introduced. This also moves definitions for some preexisting privately defined histograms to the public repository. BUG=644258 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng Committed: https://chromium.googlesource.com/chromium/src/+/6949a31c3ccf416a0e3423bb336e... Cr-Commit-Position: refs/heads/master@{#426063} ========== to ========== Make module enumeration a low priority background task. This makes the default mode of module enumeration one that is performed slowly and continuously in the blocking pool post startup. This addresses the CPU/battery regression it recently introduced. This also moves definitions for some preexisting privately defined histograms to the public repository. BUG=644258 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng Committed: https://crrev.com/6949a31c3ccf416a0e3423bb336e61165fcf5388 Cr-Commit-Position: refs/heads/master@{#426063} ========== |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
