Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
782903002:
Breakdown ClientLoadedTime histograms into buckets based on number of clients. Fix RequestTimeThrot… (Closed)
|
Created:
6 years ago by aiolos (Not reviewing) Modified:
5 years, 11 months ago CC:
chromium-reviews, darin-cc_chromium.org, jam, asvitkine+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionBreakdown ClientLoadedTime histograms into buckets based on number of clients. Fix RequestTimeThrottled histogram.
BUG=424387
The current RequestThrottlingAndCoalescing finch trial data makes me want a break down for the number of tabs we're loading. Keeping the totals for each type of Clients, and adding in new histograms for a few interesting buckets (instead of breaking it down for each number of tabs like the SessionRestore histograms do.)
Committed: https://crrev.com/cafcab99f5ea6ebfb029c7f8a8da8f98cfe48ab1
Cr-Commit-Position: refs/heads/master@{#310432}
Patch Set 1 #
Total comments: 14
Patch Set 2 : Comments addressed. #
Total comments: 9
Patch Set 3 : Matt nits. #Patch Set 4 : Rebase #
Messages
Total messages: 19 (4 generated)
aiolos@chromium.org changed reviewers: + isherman@chromium.org, mmenke@chromium.org
LGTM % comments. https://codereview.chromium.org/782903002/diff/1/content/browser/loader/resou... File content/browser/loader/resource_scheduler.cc (right): https://codereview.chromium.org/782903002/diff/1/content/browser/loader/resou... content/browser/loader/resource_scheduler.cc:31: std::string suffix, nit: Why not const-ref? https://codereview.chromium.org/782903002/diff/1/tools/metrics/histograms/his... File tools/metrics/histograms/histograms.xml (left): https://codereview.chromium.org/782903002/diff/1/tools/metrics/histograms/his... tools/metrics/histograms/histograms.xml:29367: -<histogram name="ResourceScheduler.RequestTimeThrottled_Active"> Do these previous names already have a significant amount of data associated with them? If so, please mark them as <obsolete>, so the data can still be viewed on the dashboard, but doesn't clutter the UI by default.
https://codereview.chromium.org/782903002/diff/1/content/browser/loader/resou... File content/browser/loader/resource_scheduler.cc (right): https://codereview.chromium.org/782903002/diff/1/content/browser/loader/resou... content/browser/loader/resource_scheduler.cc:370: std::string num_clients = scheduler_->GetNumClientsString(); Per suggestion below, can make this a const char* https://codereview.chromium.org/782903002/diff/1/content/browser/loader/resou... content/browser/loader/resource_scheduler.cc:377: cur_time - last_active_switch_time_); Suggest putting "cur_time - last_active_switch_time_" in a temporary, to make it clearer 3 of this histograms contain the same value. https://codereview.chromium.org/782903002/diff/1/content/browser/loader/resou... content/browser/loader/resource_scheduler.cc:998: std::string ResourceScheduler::GetNumClientsString() { Can't we return a const char*? https://codereview.chromium.org/782903002/diff/1/content/browser/loader/resou... content/browser/loader/resource_scheduler.cc:998: std::string ResourceScheduler::GetNumClientsString() { +const. Or better, remove it from ResourceScheduler, and instead move it into an anonymous namespace in this file, and take the number of clients as a string. https://codereview.chromium.org/782903002/diff/1/content/browser/loader/resou... content/browser/loader/resource_scheduler.cc:999: if (num_clients_ == 1) I'd recommend just using client_map_.size() instead - it may cost more, but it's guaranteed to take constant time. It also has no chance of getting out of sync with the actual number of clients (Or at least much less chance...).
Sorry about the delay. https://codereview.chromium.org/782903002/diff/1/content/browser/loader/resou... File content/browser/loader/resource_scheduler.cc (right): https://codereview.chromium.org/782903002/diff/1/content/browser/loader/resou... content/browser/loader/resource_scheduler.cc:31: std::string suffix, On 2014/12/09 02:59:36, Ilya Sherman wrote: > nit: Why not const-ref? Done. I had changed it to std::string to simplify things since we should be passing a const std::string to FactoryTimeGet, but have no strong feelings either way. https://codereview.chromium.org/782903002/diff/1/content/browser/loader/resou... content/browser/loader/resource_scheduler.cc:370: std::string num_clients = scheduler_->GetNumClientsString(); On 2014/12/09 15:53:28, mmenke wrote: > Per suggestion below, can make this a const char* Done. https://codereview.chromium.org/782903002/diff/1/content/browser/loader/resou... content/browser/loader/resource_scheduler.cc:377: cur_time - last_active_switch_time_); On 2014/12/09 15:53:28, mmenke wrote: > Suggest putting "cur_time - last_active_switch_time_" in a temporary, to make it > clearer 3 of this histograms contain the same value. Done. https://codereview.chromium.org/782903002/diff/1/content/browser/loader/resou... content/browser/loader/resource_scheduler.cc:998: std::string ResourceScheduler::GetNumClientsString() { On 2014/12/09 15:53:28, mmenke wrote: > Can't we return a const char*? Done. https://codereview.chromium.org/782903002/diff/1/content/browser/loader/resou... content/browser/loader/resource_scheduler.cc:998: std::string ResourceScheduler::GetNumClientsString() { On 2014/12/09 15:53:28, mmenke wrote: > Can't we return a const char*? Done. https://codereview.chromium.org/782903002/diff/1/content/browser/loader/resou... content/browser/loader/resource_scheduler.cc:999: if (num_clients_ == 1) On 2014/12/09 15:53:28, mmenke wrote: > I'd recommend just using client_map_.size() instead - it may cost more, but it's > guaranteed to take constant time. It also has no chance of getting out of sync > with the actual number of clients (Or at least much less chance...). Done. https://codereview.chromium.org/782903002/diff/1/tools/metrics/histograms/his... File tools/metrics/histograms/histograms.xml (left): https://codereview.chromium.org/782903002/diff/1/tools/metrics/histograms/his... tools/metrics/histograms/histograms.xml:29367: -<histogram name="ResourceScheduler.RequestTimeThrottled_Active"> On 2014/12/09 02:59:36, Ilya Sherman (Away Dec22-Jan5) wrote: > Do these previous names already have a significant amount of data associated > with them? If so, please mark them as <obsolete>, so the data can still be > viewed on the dashboard, but doesn't clutter the UI by default. Nope, they don't.
(Still LGTM) https://codereview.chromium.org/782903002/diff/20001/content/browser/loader/r... File content/browser/loader/resource_scheduler.cc (right): https://codereview.chromium.org/782903002/diff/20001/content/browser/loader/r... content/browser/loader/resource_scheduler.cc:37: const char* histogram_suffix, I meant, why not pass the strings as "const std::string&" (by const ref) rather than "std::string" (by copy)? I do think that std::string, as you had before, or StringPiece is better than char*. But, I don't feel strongly about this.
LGTM https://codereview.chromium.org/782903002/diff/20001/content/browser/loader/r... File content/browser/loader/resource_scheduler.cc (right): https://codereview.chromium.org/782903002/diff/20001/content/browser/loader/r... content/browser/loader/resource_scheduler.cc:31: // If histogram_suffix is NULL or the empty string: nit: |histogram_suffix| https://codereview.chromium.org/782903002/diff/20001/content/browser/loader/r... File content/browser/loader/resource_scheduler.h (right): https://codereview.chromium.org/782903002/diff/20001/content/browser/loader/r... content/browser/loader/resource_scheduler.h:180: size_t GetNumClients() const; optional: Not sure having a method to do this gets us anything. https://codereview.chromium.org/782903002/diff/20001/content/browser/loader/r... content/browser/loader/resource_scheduler.h:226: size_t num_clients_; Not used any more.
https://codereview.chromium.org/782903002/diff/20001/content/browser/loader/r... File content/browser/loader/resource_scheduler.cc (right): https://codereview.chromium.org/782903002/diff/20001/content/browser/loader/r... content/browser/loader/resource_scheduler.cc:31: // If histogram_suffix is NULL or the empty string: On 2015/01/06 19:30:03, mmenke wrote: > nit: |histogram_suffix| Done. https://codereview.chromium.org/782903002/diff/20001/content/browser/loader/r... content/browser/loader/resource_scheduler.cc:37: const char* histogram_suffix, On 2015/01/06 02:19:14, Ilya Sherman wrote: > I meant, why not pass the strings as "const std::string&" (by const ref) rather > than "std::string" (by copy)? I do think that std::string, as you had before, > or StringPiece is better than char*. But, I don't feel strongly about this. Matt: you wanted const char* for the suffix in other parts of the code. Do you have a preference here? https://codereview.chromium.org/782903002/diff/20001/content/browser/loader/r... File content/browser/loader/resource_scheduler.h (right): https://codereview.chromium.org/782903002/diff/20001/content/browser/loader/r... content/browser/loader/resource_scheduler.h:180: size_t GetNumClients() const; On 2015/01/06 19:30:03, mmenke wrote: > optional: Not sure having a method to do this gets us anything. Done. https://codereview.chromium.org/782903002/diff/20001/content/browser/loader/r... content/browser/loader/resource_scheduler.h:226: size_t num_clients_; On 2015/01/06 19:30:03, mmenke wrote: > Not used any more. Done.
https://codereview.chromium.org/782903002/diff/20001/content/browser/loader/r... File content/browser/loader/resource_scheduler.cc (right): https://codereview.chromium.org/782903002/diff/20001/content/browser/loader/r... content/browser/loader/resource_scheduler.cc:37: const char* histogram_suffix, On 2015/01/07 22:28:15, aiolos wrote: > On 2015/01/06 02:19:14, Ilya Sherman wrote: > > I meant, why not pass the strings as "const std::string&" (by const ref) > rather > > than "std::string" (by copy)? I do think that std::string, as you had before, > > or StringPiece is better than char*. But, I don't feel strongly about this. > > Matt: you wanted const char* for the suffix in other parts of the code. Do you > have a preference here? I generally think the fewer strings we make the better, and making chars into strings when we call this method, only to append them together again seems like more work for every request, with no real advantage... So using const char* seems to make more sense, but it's not something I'm going to fight over. :)
On 2015/01/07 22:32:43, mmenke wrote: > https://codereview.chromium.org/782903002/diff/20001/content/browser/loader/r... > File content/browser/loader/resource_scheduler.cc (right): > > https://codereview.chromium.org/782903002/diff/20001/content/browser/loader/r... > content/browser/loader/resource_scheduler.cc:37: const char* histogram_suffix, > On 2015/01/07 22:28:15, aiolos wrote: > > On 2015/01/06 02:19:14, Ilya Sherman wrote: > > > I meant, why not pass the strings as "const std::string&" (by const ref) > > rather > > > than "std::string" (by copy)? I do think that std::string, as you had > before, > > > or StringPiece is better than char*. But, I don't feel strongly about this. > > > > Matt: you wanted const char* for the suffix in other parts of the code. Do you > > have a preference here? > > I generally think the fewer strings we make the better, and making chars into > strings when we call this method, only to append them together again seems like > more work for every request, with no real advantage... > > So using const char* seems to make more sense, but it's not something I'm going > to fight over. :) Sounds good. I'll leave it as is then.
The CQ bit was checked by aiolos@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/782903002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/10...) android_aosp on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...) android_arm64_dbg_recipe on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...) android_chromium_gn_compile_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...) android_chromium_gn_compile_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...) android_clang_dbg_recipe on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...) android_compile_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_compile...) chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_asan_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_gn_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_gn_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) win8_chromium_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...) win_chromium_compile_dbg on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) win_chromium_x64_rel_ng on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by aiolos@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/782903002/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/cafcab99f5ea6ebfb029c7f8a8da8f98cfe48ab1 Cr-Commit-Position: refs/heads/master@{#310432} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
