Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
144523004:
Randomize order in which ready component updates are applied. On demand updates (Closed)
|
Created:
6 years, 10 months ago by Bartek Nowierski Modified:
6 years, 9 months ago CC:
chromium-reviews, waffles, cpu_(ooo_6.6-7.5) Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionRandomize order in which ready component updates are applied. On demand updates still take precedence.
BUG=343686
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=255832
Patch Set 1 #Patch Set 2 : synced #
Total comments: 8
Patch Set 3 : Applied Sorin's comments #Patch Set 4 : synced #Patch Set 5 : synced #Patch Set 6 : synced #Patch Set 7 : synced #Messages
Total messages: 34 (0 generated)
Thank you Bartek. Important things first. 1. Are you running the unit tests for this? If yes, great. If not, I was happy to see the tests pass with this change. I expected them not to, which brings about the question, should we have a test for this feature? 2. We need a bug for this, please open one at crbug.com 3. Try the change by running "git cl try" Given the current structure of the code, I like this change achieves its purpose with minimal code changes. This is good for the moment. In the future, we might want to do this in a different way, a few ideas to consider, and I am sure there are others. 1. randomize the order of items in the update request instead of randomizing the item pick. This allows the server to control the order in which updates are applied. 2. shuffling the whole work_items_ container after the update check. The work_items_ is a vector but currently it is used as a queue (new items push at the back, and current items picked at the top). It's useful when debugging to know what gets run in which order. At one point, I am thinking about refactoring the code to expose one container that keeps track of what is registered with CUS and a queue that keeps track of the work items to update in the current cycle. 3. introducing some type of cost function that controls the order and the wait times between applying updates. For instance, small stuff gets done first and the wait between the updates is short, heavy items are done last, with longer waits. Thank you! https://codereview.chromium.org/144523004/diff/30001/chrome/browser/component... File chrome/browser/component_updater/component_updater_service.cc (right): https://codereview.chromium.org/144523004/diff/30001/chrome/browser/component... chrome/browser/component_updater/component_updater_service.cc:21: #include "base/strings/stringprintf.h" What is the reason for including this header? https://codereview.chromium.org/144523004/diff/30001/chrome/browser/component... chrome/browser/component_updater/component_updater_service.cc:578: std::vector<CrxUpdateItem*> ready; One idea to filter the work_items_ is by using std::copy_if and eventually a helper function that takes a predicate as an argument and returns a vector, as in: std::vector<CrxUpdateItem*> ready_items(Helper::FilterWorkItems(IsReady)); https://codereview.chromium.org/144523004/diff/30001/chrome/browser/component... chrome/browser/component_updater/component_updater_service.cc:585: if (item->on_demand) { the chrome style guide prefers not fully bracing statements such as the inner if in this case. https://codereview.chromium.org/144523004/diff/30001/chrome/browser/component... chrome/browser/component_updater/component_updater_service.cc:594: int r = rand(); I suggest using a function from base/rand_util.h and also paying attention to how long the types are.
> 1. Are you running the unit tests for this? If yes, great. If not, I was happy > to see the tests pass with this change. I expected them not to, which brings > about the question, should we have a test for this feature? Yes, unit tests pass. I am not a big fan of adding tests for a random selection feature. Sure, I can initialize a rand seed with a constant, figure out the sequence of random numbers for that constant and make the test match that sequence, but that doesn't test much and will break on every legitimate change of the code. > 2. We need a bug for this, please open one at http://crbug.com Filed. 343686 > 3. Try the change by running "git cl try" Ok, tried it. Not sure what output to expect, but it didn't spit out any errors.
https://codereview.chromium.org/144523004/diff/30001/chrome/browser/component... File chrome/browser/component_updater/component_updater_service.cc (right): https://codereview.chromium.org/144523004/diff/30001/chrome/browser/component... chrome/browser/component_updater/component_updater_service.cc:21: #include "base/strings/stringprintf.h" On 2014/02/11 22:10:23, Sorin Jianu wrote: > What is the reason for including this header? Whoops, sorry, I used it for printing some debug messages, that I removed, but forgot to remove the include. https://codereview.chromium.org/144523004/diff/30001/chrome/browser/component... chrome/browser/component_updater/component_updater_service.cc:578: std::vector<CrxUpdateItem*> ready; On 2014/02/11 22:10:23, Sorin Jianu wrote: > One idea to filter the work_items_ is by using std::copy_if and eventually a > helper function that takes a predicate as an argument and returns a vector, as > in: > > std::vector<CrxUpdateItem*> ready_items(Helper::FilterWorkItems(IsReady)); I tried it, but for some reason copy_if doesn't seem to exist. It says that copy_if isn't in std, despite I include <algorithm> https://codereview.chromium.org/144523004/diff/30001/chrome/browser/component... chrome/browser/component_updater/component_updater_service.cc:585: if (item->on_demand) { On 2014/02/11 22:10:23, Sorin Jianu wrote: > the chrome style guide prefers not fully bracing statements such as the inner if > in this case. Done. https://codereview.chromium.org/144523004/diff/30001/chrome/browser/component... chrome/browser/component_updater/component_updater_service.cc:594: int r = rand(); On 2014/02/11 22:10:23, Sorin Jianu wrote: > I suggest using a function from base/rand_util.h and also paying attention to > how long the types are. Done.
The CQ bit was checked by bartekn@google.com
The CQ bit was unchecked by bartekn@google.com
The CQ bit was checked by bartekn@chromium.org
The CQ bit was unchecked by bartekn@chromium.org
The CQ bit was checked by bartekn@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
No LGTM from a valid reviewer yet. Only full committers are accepted. Even if an LGTM may have been provided, it was from a non-committer or a lowly provisional committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
lgtm Thank you, I was certain I had approved this CL. I apologize.
The CQ bit was checked by bartekn@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bartekn@chromium.org/144523004/310001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on win_rel for step(s) app_list_unittests, ash_unittests, aura_unittests, cacheinvalidation_unittests, cc_unittests, check_deps, check_deps2git, chrome_elf_unittests, chromedriver_unittests, components_unittests, compositor_unittests, content_browsertests, content_unittests, crypto_unittests, events_unittests, gfx_unittests, google_apis_unittests, gpu_unittests, installer_util_unittests, ipc_tests, jingle_unittests, media_unittests, mini_installer_test, nacl_integration, ppapi_unittests, printing_unittests, remoting_unittests, sql_unittests, sync_integration_tests, sync_unit_tests, telemetry_perf_unittests, telemetry_unittests, views_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
The CQ bit was checked by bartekn@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bartekn@chromium.org/144523004/330001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on win_rel for step(s) unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
The CQ bit was checked by bartekn@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bartekn@chromium.org/144523004/350001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bartekn@chromium.org/144523004/350001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bartekn@chromium.org/144523004/350001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on mac_rel for step(s) telemetry_perf_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
Sounds like the trybots have been acting up.
The CQ bit was checked by sorin@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bartekn@chromium.org/144523004/350001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on mac_rel for step(s) telemetry_perf_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
The CQ bit was checked by sorin@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bartekn@chromium.org/144523004/350001
Message was sent while issue was closed.
Change committed as 255832
Message was sent while issue was closed.
A revert of this CL has been created in https://codereview.chromium.org/192283003/ by bartekn@chromium.org. The reason for reverting is: We need to guarantee that specific security components (i.e. CRLSet, Flash, and Recovery) always come first.. |
