Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1018713002:
Record the time to update top sites through db thread to understand if this is low and how slow it … (Closed)
DescriptionRecord the time to update top sites through db thread to understand if this is low and how slow it is in the wild.
BUG=223430
Committed: https://crrev.com/e2d3d2eb9e39868db7df9a68ac50aed6e053ca5c
Cr-Commit-Position: refs/heads/master@{#321642}
Patch Set 1 : #
Total comments: 9
Patch Set 2 : Address comments. #Patch Set 3 : Address comments & pretty print histogram xml. #
Total comments: 2
Patch Set 4 : Uses timeticks instead of time. #
Total comments: 1
Messages
Total messages: 22 (5 generated)
Patchset #1 (id:1) has been deleted
yiyaoliu@chromium.org changed reviewers: + rkaplow@chromium.org
PTAL, thanks!
https://codereview.chromium.org/1018713002/diff/20001/components/history/core... File components/history/core/browser/top_sites_backend.cc (right): https://codereview.chromium.org/1018713002/diff/20001/components/history/core... components/history/core/browser/top_sites_backend.cc:107: TRACE_EVENT0("startup", "history::TopSitesBackend::UpdateTopSitesOnDBThread"); trace event should even be before the !db check. https://codereview.chromium.org/1018713002/diff/20001/components/history/core... components/history/core/browser/top_sites_backend.cc:121: UMA_HISTOGRAM_TIMES("History.UpdateTopSitesOnDBThreadTimeConsumption", I would get rid of 'Consumption' FunctionTime is fine. https://codereview.chromium.org/1018713002/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1018713002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:11213: + history::TopSitesBackend::UpdateTopSitesOnDBThread to execute. (Excludes the no need for the () here https://codereview.chromium.org/1018713002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:11214: + case where local TopSitesDatabase db_ is unavailable, i.e. where update where "the" update.
https://codereview.chromium.org/1018713002/diff/20001/components/history/core... File components/history/core/browser/top_sites_backend.cc (right): https://codereview.chromium.org/1018713002/diff/20001/components/history/core... components/history/core/browser/top_sites_backend.cc:107: TRACE_EVENT0("startup", "history::TopSitesBackend::UpdateTopSitesOnDBThread"); On 2015/03/17 22:41:12, rkaplow wrote: > trace event should even be before the !db check. I was trying to differenciate the cases where io does happen and io doesn't. If io doesn't happen, then this function won't cost a lot at all. Or is it that tracing have to be put at the start of a function? https://codereview.chromium.org/1018713002/diff/20001/components/history/core... components/history/core/browser/top_sites_backend.cc:121: UMA_HISTOGRAM_TIMES("History.UpdateTopSitesOnDBThreadTimeConsumption", On 2015/03/17 22:41:12, rkaplow wrote: > I would get rid of 'Consumption' > FunctionTime is fine. Done. https://codereview.chromium.org/1018713002/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1018713002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:11213: + history::TopSitesBackend::UpdateTopSitesOnDBThread to execute. (Excludes the On 2015/03/17 22:41:12, rkaplow wrote: > no need for the () here Done. https://codereview.chromium.org/1018713002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:11214: + case where local TopSitesDatabase db_ is unavailable, i.e. where update On 2015/03/17 22:41:12, rkaplow wrote: > where "the" update. Done.
https://codereview.chromium.org/1018713002/diff/20001/components/history/core... File components/history/core/browser/top_sites_backend.cc (right): https://codereview.chromium.org/1018713002/diff/20001/components/history/core... components/history/core/browser/top_sites_backend.cc:107: TRACE_EVENT0("startup", "history::TopSitesBackend::UpdateTopSitesOnDBThread"); On 2015/03/18 19:38:05, yao wrote: > On 2015/03/17 22:41:12, rkaplow wrote: > > trace event should even be before the !db check. > > I was trying to differenciate the cases where io does happen and io doesn't. > If io doesn't happen, then this function won't cost a lot at all. > > Or is it that tracing have to be put at the start of a function? I think TRACE is supposed to be always at the start. The idea is to show whenever the method is called, and indeed in this case the method is called. If you do this you're hiding when this is happening which is misleading. One option is to have a method which is everything after the return, like an ExecuteUpdate() and you can add your metrics to that.
Changed the tracing place as you suggested. Also pretty printed histograms.xml, which touched a couple of unrelated histograms.
lgtm
yiyaoliu@chromium.org changed reviewers: + jwd@chromium.org, sky@chromium.org
@sky, could you do an owner's review for top_sites_backend.cc? @jwd, could you do an owner's review for histograms.xml? Thanks!
https://codereview.chromium.org/1018713002/diff/60001/components/history/core... File components/history/core/browser/top_sites_backend.cc (right): https://codereview.chromium.org/1018713002/diff/60001/components/history/core... components/history/core/browser/top_sites_backend.cc:111: base::Time begin_time = base::Time::Now(); Shouldn't we use timeticks for this sort of stuff?
https://codereview.chromium.org/1018713002/diff/60001/components/history/core... File components/history/core/browser/top_sites_backend.cc (right): https://codereview.chromium.org/1018713002/diff/60001/components/history/core... components/history/core/browser/top_sites_backend.cc:111: base::Time begin_time = base::Time::Now(); On 2015/03/20 14:58:22, sky wrote: > Shouldn't we use timeticks for this sort of stuff? Done.
LGTM
lgtm
The CQ bit was checked by yiyaoliu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rkaplow@chromium.org Link to the patchset: https://codereview.chromium.org/1018713002/#ps70001 (title: "Uses timeticks instead of time.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1018713002/70001
Message was sent while issue was closed.
Committed patchset #4 (id:70001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/e2d3d2eb9e39868db7df9a68ac50aed6e053ca5c Cr-Commit-Position: refs/heads/master@{#321642}
Message was sent while issue was closed.
https://codereview.chromium.org/1018713002/diff/70001/components/history/core... File components/history/core/browser/top_sites_backend.cc (right): https://codereview.chromium.org/1018713002/diff/70001/components/history/core... components/history/core/browser/top_sites_backend.cc:112: base::TimeTicks begin_time = base::TimeTicks::Now(); I was looking at TimeTicks and wonder if you should be using ThreadNow()? Seems as though that would give you a better approximation of actual time on this thread.
Message was sent while issue was closed.
On 2015/03/27 15:51:04, sky wrote: > https://codereview.chromium.org/1018713002/diff/70001/components/history/core... > File components/history/core/browser/top_sites_backend.cc (right): > > https://codereview.chromium.org/1018713002/diff/70001/components/history/core... > components/history/core/browser/top_sites_backend.cc:112: base::TimeTicks > begin_time = base::TimeTicks::Now(); > I was looking at TimeTicks and wonder if you should be using ThreadNow()? Seems > as though that would give you a better approximation of actual time on this > thread. I'm not too familiar with this. Since ThreadNow is not supported all the time, if we are switching to ThreadNow, we'll have to separate them into 2 different histograms? Combing ThreadNow() and Now() would be confusing I feel. (This is my current Cl https://codereview.chromium.org/1005873011/, to change it to the way you suggested in the email.)
Message was sent while issue was closed.
I feel like ThreadNow is more of what we want, but given this isn't supported on windows it doesn't seem worth it now. -Scott On Fri, Mar 27, 2015 at 11:46 AM, <yiyaoliu@chromium.org> wrote: > On 2015/03/27 15:51:04, sky wrote: > > https://codereview.chromium.org/1018713002/diff/70001/components/history/core... >> >> File components/history/core/browser/top_sites_backend.cc (right): > > > > https://codereview.chromium.org/1018713002/diff/70001/components/history/core... >> >> components/history/core/browser/top_sites_backend.cc:112: base::TimeTicks >> begin_time = base::TimeTicks::Now(); >> I was looking at TimeTicks and wonder if you should be using ThreadNow()? > > Seems >> >> as though that would give you a better approximation of actual time on >> this >> thread. > > > I'm not too familiar with this. Since ThreadNow is not supported all the > time, > if we are switching to ThreadNow, we'll have to separate them into 2 > different > histograms? Combing ThreadNow() and Now() would be confusing I feel. > > (This is my current Cl https://codereview.chromium.org/1005873011/, to > change it > to the way you suggested in the email.) > > https://codereview.chromium.org/1018713002/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
