Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
692723002:
Add histograms to gather ResourceScheduler information. (Closed)
|
Created:
6 years, 1 month ago by aiolos (Not reviewing) Modified:
6 years, 1 month ago CC:
chromium-reviews, darin-cc_chromium.org, jam, asvitkine+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionAdd histograms to gather ResourceScheduler information.
BUG=424387
Committed: https://crrev.com/da52c1a80687b90a0bdcc9443228a59e8c140f3b
Cr-Commit-Position: refs/heads/master@{#302645}
Patch Set 1 #
Total comments: 30
Patch Set 2 : Comments addressed. #
Total comments: 10
Patch Set 3 : Second round of comments. #
Total comments: 15
Patch Set 4 : Matt nits. #Patch Set 5 : Rebasing for histograms.xml file. #
Total comments: 4
Patch Set 6 : Ilya nits. #
Messages
Total messages: 20 (2 generated)
aiolos@chromium.org changed reviewers: + isherman@chromium.org, mmenke@chromium.org
I'm adding ResourceScheduler histograms to see when an active/background client finishes loading after the RS has seen it as well. I'm assuming it would be better to send them separately, but if that isn't the case, I can add it to this CL.
Histograms LGTM % nits. https://codereview.chromium.org/692723002/diff/1/content/browser/loader/resou... File content/browser/loader/resource_scheduler.cc (right): https://codereview.chromium.org/692723002/diff/1/content/browser/loader/resou... content/browser/loader/resource_scheduler.cc:150: "ResourceScheduler.%sRequestTimeDeferred", Optional nit: I'd recommend having the Active vs. Background string appear as a suffix, so that the related histograms are alphabetized to be right next to each other. https://codereview.chromium.org/692723002/diff/1/tools/metrics/histograms/his... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/692723002/diff/1/tools/metrics/histograms/his... tools/metrics/histograms/histograms.xml:28078: + when it removes it's throttle in a user observable client. nit: "it's" -> "its" https://codereview.chromium.org/692723002/diff/1/tools/metrics/histograms/his... tools/metrics/histograms/histograms.xml:28078: + when it removes it's throttle in a user observable client. nit: "user observable" -> "user-observable"
https://codereview.chromium.org/692723002/diff/1/content/browser/loader/resou... File content/browser/loader/resource_scheduler.cc (right): https://codereview.chromium.org/692723002/diff/1/content/browser/loader/resou... content/browser/loader/resource_scheduler.cc:134: request_seen_(base::TimeTicks::Now()) { Should include base/time.h (Timer, included in the header, includes it, but seems like we should be including it explicitly) https://codereview.chromium.org/692723002/diff/1/content/browser/loader/resou... content/browser/loader/resource_scheduler.cc:144: if (deferred_ && request_->status().is_success()) { If request_->status() isn't success, should we even be recording anything? That means something else cancelled the request, or it failed. Will presumably be deleted on resume. Rare case, so maybe we don't really care anyways. Hrm...Also, I wonder if this is a bug. If a request is paused, and then cancelled, I can't remember if it has to be resumed or not (If a request is paused while passing around the ERR_ABORTED result, it does have to be resumed). https://codereview.chromium.org/692723002/diff/1/content/browser/loader/resou... content/browser/loader/resource_scheduler.cc:148: base::TimeDelta time_deferred = base::TimeTicks::Now() - request_seen_; This this the time we want? We could measure time we actually delayed the request actually paused instead (Time between when WillStartRequest() was called, and we called controller()->Resume(), if we paused the request, or 0 if we chose not to pause it). It is a less direct measure than the time we *tried* to delay the request for, but it is closed to our practical impact. Not sure how much difference there'll be between the two histograms - probably not much. https://codereview.chromium.org/692723002/diff/1/content/browser/loader/resou... content/browser/loader/resource_scheduler.cc:150: "ResourceScheduler.%sRequestTimeDeferred", On 2014/10/30 03:18:11, Ilya Sherman wrote: > Optional nit: I'd recommend having the Active vs. Background string appear as a > suffix, so that the related histograms are alphabetized to be right next to each > other. It also makes the histograms more searchable. https://codereview.chromium.org/692723002/diff/1/content/browser/loader/resou... content/browser/loader/resource_scheduler.cc:156: 100, Do we really need 100 buckets? Think 50 should be fine. This is just about reducing size of the histogram in memory. https://codereview.chromium.org/692723002/diff/1/content/browser/loader/resou... content/browser/loader/resource_scheduler.cc:210: base::TimeTicks request_seen_; "request_seen_" makes me think "Did we see the request", not "when was this request seen". Suggest calling it time_seen_... Or could just use request_->creation_time() (Throttles are created right after the URLRequest is scheduled, so that should basically be the same thing). https://codereview.chromium.org/692723002/diff/1/content/browser/loader/resou... content/browser/loader/resource_scheduler.cc:1016: ClientMap::iterator client_it = client_map_.find(client_id); const_iterator https://codereview.chromium.org/692723002/diff/1/content/browser/loader/resou... content/browser/loader/resource_scheduler.cc:1018: return false; When there's no client, we just let requests through the scheduler, so this seems the wrong bucket to put that case into. Also, when a client goes away, are its requests typically deleted first, or the client? https://codereview.chromium.org/692723002/diff/1/content/browser/loader/resou... content/browser/loader/resource_scheduler.cc:1019: } don't use braces when both condition and body of an if take up one line. https://codereview.chromium.org/692723002/diff/1/content/browser/loader/resou... content/browser/loader/resource_scheduler.cc:1020: return client_it->second->is_active(); Formatting here is off https://codereview.chromium.org/692723002/diff/1/content/browser/loader/resou... File content/browser/loader/resource_scheduler.h (right): https://codereview.chromium.org/692723002/diff/1/content/browser/loader/resou... content/browser/loader/resource_scheduler.h:193: bool IsClientActive(ClientId client_id); const https://codereview.chromium.org/692723002/diff/1/tools/metrics/histograms/his... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/692723002/diff/1/tools/metrics/histograms/his... tools/metrics/histograms/histograms.xml:28078: + when it removes it's throttle in a user observable client. "Removes its throttle" is a bit confusing - throttles live for the lifetime of a request. Maybe "stops throttling the request?"
Perhaps I should also keep track of whether the client has switched from active/background and filter those out? (ie, we might not want to count time that a request was deferred while its tab was background if we switch to that tab before the request is started.) Thoughts? https://codereview.chromium.org/692723002/diff/1/content/browser/loader/resou... File content/browser/loader/resource_scheduler.cc (right): https://codereview.chromium.org/692723002/diff/1/content/browser/loader/resou... content/browser/loader/resource_scheduler.cc:144: if (deferred_ && request_->status().is_success()) { On 2014/10/30 15:14:19, mmenke wrote: > If request_->status() isn't success, should we even be recording anything? That > means something else cancelled the request, or it failed. Will presumably be > deleted on resume. Rare case, so maybe we don't really care anyways. Easy enough to change. > > Hrm...Also, I wonder if this is a bug. If a request is paused, and then > cancelled, I can't remember if it has to be resumed or not (If a request is > paused while passing around the ERR_ABORTED result, it does have to be resumed). Hmm. Good question. I'm not sure. https://codereview.chromium.org/692723002/diff/1/content/browser/loader/resou... content/browser/loader/resource_scheduler.cc:148: base::TimeDelta time_deferred = base::TimeTicks::Now() - request_seen_; On 2014/10/30 15:14:19, mmenke wrote: > This this the time we want? We could measure time we actually delayed the > request actually paused instead (Time between when WillStartRequest() was > called, and we called controller()->Resume(), if we paused the request, or 0 if > we chose not to pause it). > > It is a less direct measure than the time we *tried* to delay the request for, > but it is closed to our practical impact. > > Not sure how much difference there'll be between the two histograms - probably > not much. I can see value from both options. I'm guessing we probably don't need both, although knowing how much impact we're actually having on requests wouldn't be a bad thing. Would it make sense to have histograms for both for this experiment just to see, and then take out whichever one we feel is less helpful afterward?
https://codereview.chromium.org/692723002/diff/1/content/browser/loader/resou... File content/browser/loader/resource_scheduler.cc (right): https://codereview.chromium.org/692723002/diff/1/content/browser/loader/resou... content/browser/loader/resource_scheduler.cc:144: if (deferred_ && request_->status().is_success()) { On 2014/10/30 16:58:47, aiolos wrote: > On 2014/10/30 15:14:19, mmenke wrote: > > If request_->status() isn't success, should we even be recording anything? > That > > means something else cancelled the request, or it failed. Will presumably be > > deleted on resume. Rare case, so maybe we don't really care anyways. > > Easy enough to change. > > > > Hrm...Also, I wonder if this is a bug. If a request is paused, and then > > cancelled, I can't remember if it has to be resumed or not (If a request is > > paused while passing around the ERR_ABORTED result, it does have to be > resumed). > > Hmm. Good question. I'm not sure. Glancing at the loader, looks like we're fine. https://codereview.chromium.org/692723002/diff/1/content/browser/loader/resou... content/browser/loader/resource_scheduler.cc:148: base::TimeDelta time_deferred = base::TimeTicks::Now() - request_seen_; On 2014/10/30 16:58:47, aiolos wrote: > On 2014/10/30 15:14:19, mmenke wrote: > > This this the time we want? We could measure time we actually delayed the > > request actually paused instead (Time between when WillStartRequest() was > > called, and we called controller()->Resume(), if we paused the request, or 0 > if > > we chose not to pause it). > > > > It is a less direct measure than the time we *tried* to delay the request for, > > but it is closed to our practical impact. > > > > Not sure how much difference there'll be between the two histograms - probably > > not much. > > I can see value from both options. I'm guessing we probably don't need both, > although knowing how much impact we're actually having on requests wouldn't be a > bad thing. Would it make sense to have histograms for both for this experiment > just to see, and then take out whichever one we feel is less helpful afterward? I'm fine with adding both, and a TODO to remove one of them fairly promptly. Definitely agree we don't need/want both longer term. Nice to know if there's any meaningful difference between the two. If not, can throw one away pretty quickly. If not, can figure out if the difference is relevant, and then get rid of the one we decide we care less about.
On 2014/10/30 16:58:47, aiolos wrote: > Perhaps I should also keep track of whether the client has switched from > active/background and filter those out? (ie, we might not want to count time > that a request was deferred while its tab was background if we switch to that > tab before the request is started.) Thoughts? Forgot to respond to this. If you mean cases where the client switched during the lifetime of a request, that sounds reasonable. If you mean a client that switched at any point, I suspect most hidden clients start out as visible, with the exception of during tab restore.
https://codereview.chromium.org/692723002/diff/1/content/browser/loader/resou... File content/browser/loader/resource_scheduler.cc (right): https://codereview.chromium.org/692723002/diff/1/content/browser/loader/resou... content/browser/loader/resource_scheduler.cc:134: request_seen_(base::TimeTicks::Now()) { On 2014/10/30 15:14:19, mmenke wrote: > Should include base/time.h (Timer, included in the header, includes it, but > seems like we should be including it explicitly) Done. https://codereview.chromium.org/692723002/diff/1/content/browser/loader/resou... content/browser/loader/resource_scheduler.cc:148: base::TimeDelta time_deferred = base::TimeTicks::Now() - request_seen_; On 2014/10/30 17:30:40, mmenke wrote: > On 2014/10/30 16:58:47, aiolos wrote: > > On 2014/10/30 15:14:19, mmenke wrote: > > > This this the time we want? We could measure time we actually delayed the > > > request actually paused instead (Time between when WillStartRequest() was > > > called, and we called controller()->Resume(), if we paused the request, or 0 > > if > > > we chose not to pause it). > > > > > > It is a less direct measure than the time we *tried* to delay the request > for, > > > but it is closed to our practical impact. > > > > > > Not sure how much difference there'll be between the two histograms - > probably > > > not much. > > > > I can see value from both options. I'm guessing we probably don't need both, > > although knowing how much impact we're actually having on requests wouldn't be > a > > bad thing. Would it make sense to have histograms for both for this experiment > > just to see, and then take out whichever one we feel is less helpful > afterward? > > I'm fine with adding both, and a TODO to remove one of them fairly promptly. > Definitely agree we don't need/want both longer term. > > Nice to know if there's any meaningful difference between the two. If not, can > throw one away pretty quickly. If not, can figure out if the difference is > relevant, and then get rid of the one we decide we care less about. Done. https://codereview.chromium.org/692723002/diff/1/content/browser/loader/resou... content/browser/loader/resource_scheduler.cc:150: "ResourceScheduler.%sRequestTimeDeferred", On 2014/10/30 15:14:19, mmenke wrote: > On 2014/10/30 03:18:11, Ilya Sherman wrote: > > Optional nit: I'd recommend having the Active vs. Background string appear as > a > > suffix, so that the related histograms are alphabetized to be right next to > each > > other. > > It also makes the histograms more searchable. Done. https://codereview.chromium.org/692723002/diff/1/content/browser/loader/resou... content/browser/loader/resource_scheduler.cc:156: 100, On 2014/10/30 15:14:19, mmenke wrote: > Do we really need 100 buckets? Think 50 should be fine. This is just about > reducing size of the histogram in memory. Done. https://codereview.chromium.org/692723002/diff/1/content/browser/loader/resou... content/browser/loader/resource_scheduler.cc:210: base::TimeTicks request_seen_; On 2014/10/30 15:14:19, mmenke wrote: > "request_seen_" makes me think "Did we see the request", not "when was this > request seen". > > Suggest calling it time_seen_... Or could just use request_->creation_time() > (Throttles are created right after the URLRequest is scheduled, so that should > basically be the same thing). Acknowledged. https://codereview.chromium.org/692723002/diff/1/content/browser/loader/resou... content/browser/loader/resource_scheduler.cc:1016: ClientMap::iterator client_it = client_map_.find(client_id); On 2014/10/30 15:14:19, mmenke wrote: > const_iterator Done. https://codereview.chromium.org/692723002/diff/1/content/browser/loader/resou... content/browser/loader/resource_scheduler.cc:1018: return false; On 2014/10/30 15:14:19, mmenke wrote: > When there's no client, we just let requests through the scheduler, so this > seems the wrong bucket to put that case into. > > Also, when a client goes away, are its requests typically deleted first, or the > client? I made a separate histogram for those requests plus requests from clients that have switched states since the request was issued. I'm debating changing this to add a histogram that gets counts from all requests instead. ie: ResourceScheduler.RequestTimeDeferred_Active ResourceScheduler.RequestTimeDeferred_Background ResourceScheduler.RequestTimeDeferred_Other vs ResourceScheduler.RequestTimeDeferred ResourceScheduler.RequestTimeDeferred_Active ResourceScheduler.RequestTimeDeferred_Background Thoughts? https://codereview.chromium.org/692723002/diff/1/content/browser/loader/resou... content/browser/loader/resource_scheduler.cc:1019: } On 2014/10/30 15:14:19, mmenke wrote: > don't use braces when both condition and body of an if take up one line. Done. https://codereview.chromium.org/692723002/diff/1/content/browser/loader/resou... content/browser/loader/resource_scheduler.cc:1020: return client_it->second->is_active(); On 2014/10/30 15:14:19, mmenke wrote: > Formatting here is off Done. https://codereview.chromium.org/692723002/diff/1/tools/metrics/histograms/his... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/692723002/diff/1/tools/metrics/histograms/his... tools/metrics/histograms/histograms.xml:28078: + when it removes it's throttle in a user observable client. On 2014/10/30 03:18:11, Ilya Sherman wrote: > nit: "user observable" -> "user-observable" Done. https://codereview.chromium.org/692723002/diff/1/tools/metrics/histograms/his... tools/metrics/histograms/histograms.xml:28078: + when it removes it's throttle in a user observable client. On 2014/10/30 15:14:19, mmenke wrote: > "Removes its throttle" is a bit confusing - throttles live for the lifetime of a > request. Maybe "stops throttling the request?" Done.
https://codereview.chromium.org/692723002/diff/20001/content/browser/loader/r... File content/browser/loader/resource_scheduler.cc (right): https://codereview.chromium.org/692723002/diff/20001/content/browser/loader/r... content/browser/loader/resource_scheduler.cc:123: ResourceScheduler::ClientState client_state, Optional nit: Suggest not taking this as an argument, but instead calling scheduler->GetClientState here - think that, as implemented, when the request wants the client state and what it does with it is more a feature of the request than something the scheduler should care about. https://codereview.chromium.org/692723002/diff/20001/content/browser/loader/r... content/browser/loader/resource_scheduler.cc:146: if (request_->status().is_success()) { nit: Suggest making this an early return - early returns are generally preferred, particularly when there's a bunch of code below them, making indentation a little harder to follow. https://codereview.chromium.org/692723002/diff/20001/content/browser/loader/r... content/browser/loader/resource_scheduler.cc:152: : current_state == ACTIVE ? "Active" : "Background"; Suggest a method or function for this, or splitting this into multiple statements. As-is, it's pretty hard to follow. https://codereview.chromium.org/692723002/diff/20001/content/browser/loader/r... content/browser/loader/resource_scheduler.cc:157: "RequestTimeDeferred", client_state, time - time_deferred_); If !deferred_, we should be recording a 0, I believe. https://codereview.chromium.org/692723002/diff/20001/content/browser/loader/r... content/browser/loader/resource_scheduler.cc:1074: void ResourceScheduler::PostHistogram(const char* base_name, This should be in an anonymous namespace up top.
https://codereview.chromium.org/692723002/diff/20001/content/browser/loader/r... File content/browser/loader/resource_scheduler.cc (right): https://codereview.chromium.org/692723002/diff/20001/content/browser/loader/r... content/browser/loader/resource_scheduler.cc:123: ResourceScheduler::ClientState client_state, On 2014/11/03 16:05:55, mmenke wrote: > Optional nit: Suggest not taking this as an argument, but instead calling > scheduler->GetClientState here - think that, as implemented, when the request > wants the client state and what it does with it is more a feature of the request > than something the scheduler should care about. Done. https://codereview.chromium.org/692723002/diff/20001/content/browser/loader/r... content/browser/loader/resource_scheduler.cc:146: if (request_->status().is_success()) { On 2014/11/03 16:05:55, mmenke wrote: > nit: Suggest making this an early return - early returns are generally > preferred, particularly when there's a bunch of code below them, making > indentation a little harder to follow. Done. https://codereview.chromium.org/692723002/diff/20001/content/browser/loader/r... content/browser/loader/resource_scheduler.cc:152: : current_state == ACTIVE ? "Active" : "Background"; On 2014/11/03 16:05:54, mmenke wrote: > Suggest a method or function for this, or splitting this into multiple > statements. As-is, it's pretty hard to follow. Done. https://codereview.chromium.org/692723002/diff/20001/content/browser/loader/r... content/browser/loader/resource_scheduler.cc:157: "RequestTimeDeferred", client_state, time - time_deferred_); On 2014/11/03 16:05:55, mmenke wrote: > If !deferred_, we should be recording a 0, I believe. Done. https://codereview.chromium.org/692723002/diff/20001/content/browser/loader/r... content/browser/loader/resource_scheduler.cc:1074: void ResourceScheduler::PostHistogram(const char* base_name, On 2014/11/03 16:05:55, mmenke wrote: > This should be in an anonymous namespace up top. Done.
Ilya: The histograms changed since you gave an LGTM. PTAL.
Ilya: The histograms changed since you gave an LGTM. PTAL.
LGTM, just nits. https://codereview.chromium.org/692723002/diff/40001/content/browser/loader/r... File content/browser/loader/resource_scheduler.cc (right): https://codereview.chromium.org/692723002/diff/40001/content/browser/loader/r... content/browser/loader/resource_scheduler.cc:43: } // namespace nit: Blank line before end of namespace. https://codereview.chromium.org/692723002/diff/40001/content/browser/loader/r... content/browser/loader/resource_scheduler.cc:48: static const size_t kMaxNumThrottledRequestsPerClient = 1; Optional nit: When there's an anonymous namespace, the preference seems to be to not use static before variable declarations, and instead move them into the anonymous namespace. https://codereview.chromium.org/692723002/diff/40001/content/browser/loader/r... content/browser/loader/resource_scheduler.cc:154: client_state_on_creation_ = scheduler_->GetClientState(client_id_); optional nit: I think it's preferred to have "client_state_on_creation_(scheduler->GetClientState(client_id)," in the initializer list, and make client_state_on_creation_ const. If you really prefer this, though, that's fine. https://codereview.chromium.org/692723002/diff/40001/content/browser/loader/r... content/browser/loader/resource_scheduler.cc:171: client_state = "Background"; nit: Use braces on all blocks of an if statements when any component of it takes more than one lines (ie, this should be "if () {...} else if () {...}", modulo linebreaks) https://codereview.chromium.org/692723002/diff/40001/content/browser/loader/r... content/browser/loader/resource_scheduler.cc:180: base::TimeDelta::FromMicroseconds(0)); nit: Per above comment, should use braces here. https://codereview.chromium.org/692723002/diff/40001/content/browser/loader/r... content/browser/loader/resource_scheduler.cc:234: net::URLRequest* request_; While you're here, can client_id_ and request_ be const? https://codereview.chromium.org/692723002/diff/40001/content/browser/loader/r... content/browser/loader/resource_scheduler.cc:1092: } // namespace content nit: Blank line before end of namespace. https://codereview.chromium.org/692723002/diff/40001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/692723002/diff/40001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:28120: + states since the request was made or a request without a Client. optional: Worth mentioning this isn't perfectly accurate, here or in the code? If a tab goes background -> active -> background while a resource loads, it will be in the Background histogram. I think this is probably not worth fixing, but is worth noting somewhere.
https://codereview.chromium.org/692723002/diff/40001/content/browser/loader/r... File content/browser/loader/resource_scheduler.cc (right): https://codereview.chromium.org/692723002/diff/40001/content/browser/loader/r... content/browser/loader/resource_scheduler.cc:43: } // namespace On 2014/11/03 19:01:19, mmenke wrote: > nit: Blank line before end of namespace. Done. https://codereview.chromium.org/692723002/diff/40001/content/browser/loader/r... content/browser/loader/resource_scheduler.cc:154: client_state_on_creation_ = scheduler_->GetClientState(client_id_); On 2014/11/03 19:01:19, mmenke wrote: > optional nit: I think it's preferred to have > "client_state_on_creation_(scheduler->GetClientState(client_id)," in the > initializer list, and make client_state_on_creation_ const. > > If you really prefer this, though, that's fine. Done. https://codereview.chromium.org/692723002/diff/40001/content/browser/loader/r... content/browser/loader/resource_scheduler.cc:171: client_state = "Background"; On 2014/11/03 19:01:20, mmenke wrote: > nit: Use braces on all blocks of an if statements when any component of it > takes more than one lines (ie, this should be "if () {...} else if () {...}", > modulo linebreaks) Done. https://codereview.chromium.org/692723002/diff/40001/content/browser/loader/r... content/browser/loader/resource_scheduler.cc:180: base::TimeDelta::FromMicroseconds(0)); On 2014/11/03 19:01:19, mmenke wrote: > nit: Per above comment, should use braces here. Done. https://codereview.chromium.org/692723002/diff/40001/content/browser/loader/r... content/browser/loader/resource_scheduler.cc:234: net::URLRequest* request_; On 2014/11/03 19:01:19, mmenke wrote: > While you're here, can client_id_ and request_ be const? request_ can't be const because of the usage of url_request's SetPriority, but I changed client_id_. https://codereview.chromium.org/692723002/diff/40001/content/browser/loader/r... content/browser/loader/resource_scheduler.cc:1092: } // namespace content On 2014/11/03 19:01:19, mmenke wrote: > nit: Blank line before end of namespace. Done. https://codereview.chromium.org/692723002/diff/40001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/692723002/diff/40001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:28120: + states since the request was made or a request without a Client. On 2014/11/03 19:01:20, mmenke wrote: > optional: Worth mentioning this isn't perfectly accurate, here or in the code? > If a tab goes background -> active -> background while a resource loads, it will > be in the Background histogram. I think this is probably not worth fixing, but > is worth noting somewhere. I think it would be worthwhile putting it in both places, but agree that we don't care about fixing. Done.
histograms still lgtm https://codereview.chromium.org/692723002/diff/80001/content/browser/loader/r... File content/browser/loader/resource_scheduler.cc (right): https://codereview.chromium.org/692723002/diff/80001/content/browser/loader/r... content/browser/loader/resource_scheduler.cc:34: base::StringPrintf("ResourceScheduler.%s_%s", base_name, suffix); Optional nit: Out of curiousity, why are you using an underscore rather than a dot for the second separator? https://codereview.chromium.org/692723002/diff/80001/content/browser/loader/r... content/browser/loader/resource_scheduler.cc:184: PostHistogram("RequestTimeDeferred", In general, it's nice not to have to repeat histogram names, to reduce the risk of typos sneaking in to just one of the code paths. I'll defer to your judgement here, but something to think about.
https://codereview.chromium.org/692723002/diff/80001/content/browser/loader/r... File content/browser/loader/resource_scheduler.cc (right): https://codereview.chromium.org/692723002/diff/80001/content/browser/loader/r... content/browser/loader/resource_scheduler.cc:34: base::StringPrintf("ResourceScheduler.%s_%s", base_name, suffix); On 2014/11/03 23:03:45, Ilya Sherman wrote: > Optional nit: Out of curiousity, why are you using an underscore rather than a > dot for the second separator? It's the form I saw used with the only other histograms I've messed with (SessionRestore.AllTabsLoaded_1, etc). I have no preference I was just trying to be consistent with what I knew. After looking at other histograms, the dot does seem to be used more often. Changing. https://codereview.chromium.org/692723002/diff/80001/content/browser/loader/r... content/browser/loader/resource_scheduler.cc:184: PostHistogram("RequestTimeDeferred", On 2014/11/03 23:03:45, Ilya Sherman wrote: > In general, it's nice not to have to repeat histogram names, to reduce the risk > of typos sneaking in to just one of the code paths. I'll defer to your > judgement here, but something to think about. Done.
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/692723002/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/da52c1a80687b90a0bdcc9443228a59e8c140f3b Cr-Commit-Position: refs/heads/master@{#302645} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
