Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2184023002:
Report the duration of sampled tasks with histogram. (Closed)
DescriptionReport the duration of sampled tasks with histogram.
BUG=630688
Committed: https://crrev.com/1aca73dda839f813d2a4d6fbcf654ed488a197dd
Cr-Commit-Position: refs/heads/master@{#409606}
Patch Set 1 #
Total comments: 41
Patch Set 2 : Modified according to the reviews --v1 #Patch Set 3 : Formatting, comment, and typo.. #
Total comments: 20
Patch Set 4 : Correcting the nits. #Patch Set 5 : changed unit in histograms.xml #Patch Set 6 : Edited comments #
Total comments: 1
Patch Set 7 : Delete blank line #Patch Set 8 : Rename TaskDurationReporeter->TaskTimeReporter #Patch Set 9 : scheduler_export #
Total comments: 8
Patch Set 10 : Remove the sampling part. Change bin number to 50. #
Total comments: 2
Patch Set 11 : Directly call from renderer_scheduler_impl.cc #
Messages
Total messages: 64 (33 generated)
sunyunjia@chromium.org changed reviewers: + majidvp@chromium.org
Try my first CL
Try my first CL
Great first CL! See my comments below. https://codereview.chromium.org/2184023002/diff/1/components/scheduler/BUILD.gn File components/scheduler/BUILD.gn (right): https://codereview.chromium.org/2184023002/diff/1/components/scheduler/BUILD.... components/scheduler/BUILD.gn:6: import("//testing/test.gni") You should not need to import 'test.gni'. To add a dependency, add the target in the deps. https://codereview.chromium.org/2184023002/diff/1/components/scheduler/BUILD.... components/scheduler/BUILD.gn:169: "//testing/gtest", Add you dependencies here if necessary. The testing/gtest is already here so I don't think you need to add it. https://codereview.chromium.org/2184023002/diff/1/components/scheduler/render... File components/scheduler/renderer/renderer_scheduler_impl.h (right): https://codereview.chromium.org/2184023002/diff/1/components/scheduler/render... components/scheduler/renderer/renderer_scheduler_impl.h:142: I think this is is not longer needed. https://codereview.chromium.org/2184023002/diff/1/components/scheduler/render... File components/scheduler/renderer/renderer_task_duration.cc (right): https://codereview.chromium.org/2184023002/diff/1/components/scheduler/render... components/scheduler/renderer/renderer_task_duration.cc:6: #include "components/scheduler/renderer/renderer_task_duration.h" The class's own header should be included first. Then followed by a blank line and other includes. See: https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes https://codereview.chromium.org/2184023002/diff/1/components/scheduler/render... components/scheduler/renderer/renderer_task_duration.cc:10: : sample_count_(0) { The initializer list should be on the same line as it falls below the 80 characters limit. https://codereview.chromium.org/2184023002/diff/1/components/scheduler/render... components/scheduler/renderer/renderer_task_duration.cc:13: RendererTaskDuration::~RendererTaskDuration() { Move closing brace to the same line for this empty function i.e., {} Same for above. https://google.github.io/styleguide/cppguide.html#Horizontal_Whitespace https://codereview.chromium.org/2184023002/diff/1/components/scheduler/render... components/scheduler/renderer/renderer_task_duration.cc:18: if (sample_count_ % kSampleInterval == 0) { Given that we reset the sample count to 0. Should this be just sample_count_ == kSampleInterval ? https://codereview.chromium.org/2184023002/diff/1/components/scheduler/render... components/scheduler/renderer/renderer_task_duration.cc:19: UMA_HISTOGRAM_TIMES("RendererScheduler.TaskDuration", UMA_HISTGRAM_TIMES range is 1ms - 10seconds. This means that all long tasks larger than 10ms will get into the same bucket. I think this is good enough but if we want a better resolution for long tasks we may want to consider a larger range. I think it is good to 2x check with tdresser@. https://codereview.chromium.org/2184023002/diff/1/components/scheduler/render... components/scheduler/renderer/renderer_task_duration.cc:20: duration); The line break is not needed. Falls within the 80 characters limits. https://codereview.chromium.org/2184023002/diff/1/components/scheduler/render... File components/scheduler/renderer/renderer_task_duration.h (right): https://codereview.chromium.org/2184023002/diff/1/components/scheduler/render... components/scheduler/renderer/renderer_task_duration.h:11: class RendererTaskDuration { I think the convention is to have a blank line before class declaration but I couldn't find it in the style guide. So I am fine either way. https://codereview.chromium.org/2184023002/diff/1/components/scheduler/render... components/scheduler/renderer/renderer_task_duration.h:11: class RendererTaskDuration { I think a better name may be a TaskDurationReporter. Also please add a class comment explaining what this class does and in particular the sampling interval. https://codereview.chromium.org/2184023002/diff/1/components/scheduler/render... components/scheduler/renderer/renderer_task_duration.h:16: static const int kSampleInterval = 100; This constant does not need to be public. Please move to the implementation file. https://codereview.chromium.org/2184023002/diff/1/components/scheduler/render... components/scheduler/renderer/renderer_task_duration.h:19: }; This class is not meant to be copies/moved so please prevent these by using DISALLOW_COPY_AND_ASSIGN macro. https://codereview.chromium.org/2184023002/diff/1/components/scheduler/render... File components/scheduler/renderer/renderer_task_duration_unittest.cc (right): https://codereview.chromium.org/2184023002/diff/1/components/scheduler/render... components/scheduler/renderer/renderer_task_duration_unittest.cc:8: #include "base/metrics/histogram_macros.h" histogram_macros.h seems to be an unnecessary include. https://codereview.chromium.org/2184023002/diff/1/components/scheduler/render... components/scheduler/renderer/renderer_task_duration_unittest.cc:9: #include "testing/gmock/include/gmock/gmock.h" ditto gmock.h https://codereview.chromium.org/2184023002/diff/1/components/scheduler/render... components/scheduler/renderer/renderer_task_duration_unittest.cc:14: TEST_F(RendererTaskDurationTest, LoggingDuration) { Using a text fixture is not necessary here. You can use a plain old TEST. This make the above typedef unnecessary as well. https://codereview.chromium.org/2184023002/diff/1/components/scheduler/render... components/scheduler/renderer/renderer_task_duration_unittest.cc:18: for (int i = 1; i <= rtd.kSampleInterval + 1; ++i) { Please spell out 100 in the test rather than using the constant from the code that is being tested. Otherwise the test will not verify the expected definition. https://codereview.chromium.org/2184023002/diff/1/components/scheduler/render... components/scheduler/renderer/renderer_task_duration_unittest.cc:20: int msecond = 10 * 1000 / rtd.kSampleInterval * num + 1; s/msceond/milliseconds/ https://codereview.chromium.org/2184023002/diff/1/components/scheduler/render... components/scheduler/renderer/renderer_task_duration_unittest.cc:20: int msecond = 10 * 1000 / rtd.kSampleInterval * num + 1; This expression seems a bit too overcomplicated for what you want to achieve. Is it trying to make sure different task duration fall into different buckets. For this test we don't have to have a different interval for each task, just two different intervals one for 100th task and one for the rest. This should also make the assertion simpler to read. https://codereview.chromium.org/2184023002/diff/1/components/scheduler/render... components/scheduler/renderer/renderer_task_duration_unittest.cc:24: tester.ExpectUniqueSample("RendererScheduler.TaskDuration", 1, 1); Why are we asserting for 1 here? It is hard to correlate that with duration of 100th sample. See above for a suggestion of a simpler way to do this which should help make the relation more clear. https://codereview.chromium.org/2184023002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2184023002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:46336: + <owner>sunyunjia@chromium.org</owner> Please add tdresser@chromium as the owner as well
sunyunjia@chromium.org changed reviewers: + tdresser@chromium.org
Hi Tim, This is the patch for bug 630688. Is UMA_HISTOGRAM_TIMES([1msec,10sec], 50bins) enough for the tasks we observe? https://codereview.chromium.org/2184023002/diff/1/components/scheduler/BUILD.gn File components/scheduler/BUILD.gn (right): https://codereview.chromium.org/2184023002/diff/1/components/scheduler/BUILD.... components/scheduler/BUILD.gn:6: import("//testing/test.gni") On 2016/07/27 13:50:59, majidvp wrote: > You should not need to import 'test.gni'. > To add a dependency, add the target in the deps. Done. https://codereview.chromium.org/2184023002/diff/1/components/scheduler/BUILD.... components/scheduler/BUILD.gn:169: "//testing/gtest", On 2016/07/27 13:50:59, majidvp wrote: > Add you dependencies here if necessary. The testing/gtest is > already here so I don't think you need to add it. Acknowledged. https://codereview.chromium.org/2184023002/diff/1/components/scheduler/render... File components/scheduler/renderer/renderer_scheduler_impl.h (right): https://codereview.chromium.org/2184023002/diff/1/components/scheduler/render... components/scheduler/renderer/renderer_scheduler_impl.h:142: On 2016/07/27 13:50:59, majidvp wrote: > I think this is is not longer needed. Done. https://codereview.chromium.org/2184023002/diff/1/components/scheduler/render... File components/scheduler/renderer/renderer_task_duration.cc (right): https://codereview.chromium.org/2184023002/diff/1/components/scheduler/render... components/scheduler/renderer/renderer_task_duration.cc:6: #include "components/scheduler/renderer/renderer_task_duration.h" On 2016/07/27 13:50:59, majidvp wrote: > The class's own header should be included first. Then followed by a blank > line and other includes. > > See: > https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes Done. https://codereview.chromium.org/2184023002/diff/1/components/scheduler/render... components/scheduler/renderer/renderer_task_duration.cc:10: : sample_count_(0) { On 2016/07/27 13:50:59, majidvp wrote: > The initializer list should be on the same line as it falls below the 80 > characters limit. Done. https://codereview.chromium.org/2184023002/diff/1/components/scheduler/render... components/scheduler/renderer/renderer_task_duration.cc:13: RendererTaskDuration::~RendererTaskDuration() { On 2016/07/27 13:50:59, majidvp wrote: > Move closing brace to the same line for this empty function i.e., {} > Same for above. > > https://google.github.io/styleguide/cppguide.html#Horizontal_Whitespace Done. https://codereview.chromium.org/2184023002/diff/1/components/scheduler/render... components/scheduler/renderer/renderer_task_duration.cc:18: if (sample_count_ % kSampleInterval == 0) { On 2016/07/27 13:50:59, majidvp wrote: > Given that we reset the sample count to 0. > Should this be just sample_count_ == kSampleInterval ? Done. https://codereview.chromium.org/2184023002/diff/1/components/scheduler/render... components/scheduler/renderer/renderer_task_duration.cc:20: duration); On 2016/07/27 13:50:59, majidvp wrote: > The line break is not needed. Falls within the 80 characters limits. Done. https://codereview.chromium.org/2184023002/diff/1/components/scheduler/render... File components/scheduler/renderer/renderer_task_duration.h (right): https://codereview.chromium.org/2184023002/diff/1/components/scheduler/render... components/scheduler/renderer/renderer_task_duration.h:11: class RendererTaskDuration { On 2016/07/27 13:50:59, majidvp wrote: > I think the convention is to have a blank line before class declaration but I > couldn't find it in > the style guide. So I am fine either way. Done. https://codereview.chromium.org/2184023002/diff/1/components/scheduler/render... components/scheduler/renderer/renderer_task_duration.h:11: class RendererTaskDuration { On 2016/07/27 13:50:59, majidvp wrote: > I think a better name may be a TaskDurationReporter. > Also please add a class comment explaining what this class does and > in particular the sampling interval. Done. https://codereview.chromium.org/2184023002/diff/1/components/scheduler/render... components/scheduler/renderer/renderer_task_duration.h:16: static const int kSampleInterval = 100; On 2016/07/27 13:50:59, majidvp wrote: > This constant does not need to be public. Please move to the implementation > file. Done. https://codereview.chromium.org/2184023002/diff/1/components/scheduler/render... components/scheduler/renderer/renderer_task_duration.h:19: }; On 2016/07/27 13:50:59, majidvp wrote: > This class is not meant to be copies/moved so please prevent these by using > DISALLOW_COPY_AND_ASSIGN macro. Done. https://codereview.chromium.org/2184023002/diff/1/components/scheduler/render... File components/scheduler/renderer/renderer_task_duration_unittest.cc (right): https://codereview.chromium.org/2184023002/diff/1/components/scheduler/render... components/scheduler/renderer/renderer_task_duration_unittest.cc:8: #include "base/metrics/histogram_macros.h" On 2016/07/27 13:50:59, majidvp wrote: > histogram_macros.h seems to be an unnecessary include. Done. https://codereview.chromium.org/2184023002/diff/1/components/scheduler/render... components/scheduler/renderer/renderer_task_duration_unittest.cc:9: #include "testing/gmock/include/gmock/gmock.h" On 2016/07/27 13:50:59, majidvp wrote: > ditto gmock.h Done. https://codereview.chromium.org/2184023002/diff/1/components/scheduler/render... components/scheduler/renderer/renderer_task_duration_unittest.cc:14: TEST_F(RendererTaskDurationTest, LoggingDuration) { On 2016/07/27 13:50:59, majidvp wrote: > Using a text fixture is not necessary here. You can use a plain old TEST. > This make the above typedef unnecessary as well. Done. https://codereview.chromium.org/2184023002/diff/1/components/scheduler/render... components/scheduler/renderer/renderer_task_duration_unittest.cc:18: for (int i = 1; i <= rtd.kSampleInterval + 1; ++i) { On 2016/07/27 13:50:59, majidvp wrote: > Please spell out 100 in the test rather than using the constant from the code > that is being tested. Otherwise the test will not verify the expected > definition. Done. https://codereview.chromium.org/2184023002/diff/1/components/scheduler/render... components/scheduler/renderer/renderer_task_duration_unittest.cc:20: int msecond = 10 * 1000 / rtd.kSampleInterval * num + 1; On 2016/07/27 13:50:59, majidvp wrote: > s/msceond/milliseconds/ Done. https://codereview.chromium.org/2184023002/diff/1/components/scheduler/render... components/scheduler/renderer/renderer_task_duration_unittest.cc:20: int msecond = 10 * 1000 / rtd.kSampleInterval * num + 1; On 2016/07/27 13:50:59, majidvp wrote: > This expression seems a bit too overcomplicated for what you want to achieve. > Is it trying to make sure different task duration fall into different buckets. > > > For this test we don't have to have a different interval for each task, > just two different intervals one for 100th task and one for the rest. > This should also make the assertion simpler to read. > Done. https://codereview.chromium.org/2184023002/diff/1/components/scheduler/render... components/scheduler/renderer/renderer_task_duration_unittest.cc:24: tester.ExpectUniqueSample("RendererScheduler.TaskDuration", 1, 1); On 2016/07/27 13:50:59, majidvp wrote: > Why are we asserting for 1 here? It is hard to correlate that with duration of > 100th sample. > See above for a suggestion of a simpler way to do this which should help make > the relation > more clear. Done. https://codereview.chromium.org/2184023002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2184023002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:46336: + <owner>sunyunjia@chromium.org</owner> On 2016/07/27 13:51:00, majidvp wrote: > Please add tdresser@chromium as the owner as well Done.
LGTM with nits. You'll need a reviewer for histograms.xml, who will have comments about the number of buckets etc. I think this is a reasonable use of a 100 bucket metric though. I'd rather decrease the sample rate and have 100 buckets than have 50 buckets. https://codereview.chromium.org/2184023002/diff/40001/components/scheduler/re... File components/scheduler/renderer/renderer_scheduler_impl.h (right): https://codereview.chromium.org/2184023002/diff/40001/components/scheduler/re... components/scheduler/renderer/renderer_scheduler_impl.h:475: TaskDurationReporter duration_reporter_; I think we can put this in MainThreadOnly(), to make it clear what thread this is accessed from. https://codereview.chromium.org/2184023002/diff/40001/components/scheduler/re... File components/scheduler/renderer/task_duration_reporter.cc (right): https://codereview.chromium.org/2184023002/diff/40001/components/scheduler/re... components/scheduler/renderer/task_duration_reporter.cc:11: const int kSampleInterval = 100; This shouldn't be indented, should it? Play around with "git cl format", or integrating clang format into your editor. https://codereview.chromium.org/2184023002/diff/40001/components/scheduler/re... components/scheduler/renderer/task_duration_reporter.cc:23: UMA_HISTOGRAM_TIMES("RendererScheduler.TaskDuration", duration); For input latency, we use UMA_HISTOGRAM_CUSTOM_COUNTS( \ name, (end.event_time - start.event_time).InMicroseconds(), 1, 1000000, \ 100); That would probably be reasonable for this metric as well. I think we do want to register tasks shorter than 1ms. Whoever reviews histograms.xml will have feedback on whether or not that's reasonable. https://codereview.chromium.org/2184023002/diff/40001/components/scheduler/re... File components/scheduler/renderer/task_duration_reporter.h (right): https://codereview.chromium.org/2184023002/diff/40001/components/scheduler/re... components/scheduler/renderer/task_duration_reporter.h:17: void ReportSampleDuration(base::TimeDelta duration); Maybe ReportTaskDuration? https://codereview.chromium.org/2184023002/diff/40001/components/scheduler/re... File components/scheduler/renderer/task_duration_reporter_unittest.cc (right): https://codereview.chromium.org/2184023002/diff/40001/components/scheduler/re... components/scheduler/renderer/task_duration_reporter_unittest.cc:14: TaskDurationReporter tdr; task_duration_reporter We try to avoid short acronyms in general. https://codereview.chromium.org/2184023002/diff/40001/components/scheduler/re... components/scheduler/renderer/task_duration_reporter_unittest.cc:16: int millisecond = 5 * 1000; Settings this to 5 * 1000 seems a bit odd. I'd probably just go with 1 millisecond most of the type, and then 2 milliseconds on the 100th run, or something like that. https://codereview.chromium.org/2184023002/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2184023002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:46339: + The duration of every 100th task queued in the scheduler to see the Might be worth commenting that this is queued in the _renderer_ scheduler.
https://codereview.chromium.org/2184023002/diff/40001/components/scheduler/re... File components/scheduler/renderer/task_duration_reporter.cc (right): https://codereview.chromium.org/2184023002/diff/40001/components/scheduler/re... components/scheduler/renderer/task_duration_reporter.cc:14: TaskDurationReporter::TaskDurationReporter() : sample_count_(0) {} nit: same as above. Should not be indented. https://codereview.chromium.org/2184023002/diff/40001/components/scheduler/re... File components/scheduler/renderer/task_duration_reporter.h (right): https://codereview.chromium.org/2184023002/diff/40001/components/scheduler/re... components/scheduler/renderer/task_duration_reporter.h:12: // Report the task duration in a histogram for every 100th task nit: comment should end with a period. https://codereview.chromium.org/2184023002/diff/40001/components/scheduler/re... File components/scheduler/renderer/task_duration_reporter_unittest.cc (right): https://codereview.chromium.org/2184023002/diff/40001/components/scheduler/re... components/scheduler/renderer/task_duration_reporter_unittest.cc:16: int millisecond = 5 * 1000; nit: it is better to use a descriptive name such as "duration_ms" instead of just millisecond. In fact you can initialize duration here and not have two variables.
sunyunjia@chromium.org changed reviewers: + isherman@chromium.org
PTAL https://codereview.chromium.org/2184023002/diff/40001/components/scheduler/re... File components/scheduler/renderer/renderer_scheduler_impl.h (right): https://codereview.chromium.org/2184023002/diff/40001/components/scheduler/re... components/scheduler/renderer/renderer_scheduler_impl.h:475: TaskDurationReporter duration_reporter_; On 2016/07/27 18:04:25, tdresser wrote: > I think we can put this in MainThreadOnly(), to make it clear what thread this > is accessed from. Done. https://codereview.chromium.org/2184023002/diff/40001/components/scheduler/re... File components/scheduler/renderer/task_duration_reporter.cc (right): https://codereview.chromium.org/2184023002/diff/40001/components/scheduler/re... components/scheduler/renderer/task_duration_reporter.cc:11: const int kSampleInterval = 100; On 2016/07/27 18:04:25, tdresser wrote: > This shouldn't be indented, should it? Play around with "git cl format", or > integrating clang format into your editor. Done. https://codereview.chromium.org/2184023002/diff/40001/components/scheduler/re... components/scheduler/renderer/task_duration_reporter.cc:14: TaskDurationReporter::TaskDurationReporter() : sample_count_(0) {} On 2016/07/27 18:34:02, majidvp wrote: > nit: same as above. Should not be indented. Done. https://codereview.chromium.org/2184023002/diff/40001/components/scheduler/re... components/scheduler/renderer/task_duration_reporter.cc:23: UMA_HISTOGRAM_TIMES("RendererScheduler.TaskDuration", duration); On 2016/07/27 18:04:25, tdresser wrote: > For input latency, we use > > UMA_HISTOGRAM_CUSTOM_COUNTS( \ > name, (end.event_time - start.event_time).InMicroseconds(), 1, 1000000, \ > 100); > > That would probably be reasonable for this metric as well. I think we do want to > register tasks shorter than 1ms. > > Whoever reviews histograms.xml will have feedback on whether or not that's > reasonable. Done. https://codereview.chromium.org/2184023002/diff/40001/components/scheduler/re... File components/scheduler/renderer/task_duration_reporter.h (right): https://codereview.chromium.org/2184023002/diff/40001/components/scheduler/re... components/scheduler/renderer/task_duration_reporter.h:12: // Report the task duration in a histogram for every 100th task On 2016/07/27 18:34:03, majidvp wrote: > nit: comment should end with a period. Done. https://codereview.chromium.org/2184023002/diff/40001/components/scheduler/re... components/scheduler/renderer/task_duration_reporter.h:17: void ReportSampleDuration(base::TimeDelta duration); On 2016/07/27 18:04:25, tdresser wrote: > Maybe ReportTaskDuration? Done. https://codereview.chromium.org/2184023002/diff/40001/components/scheduler/re... File components/scheduler/renderer/task_duration_reporter_unittest.cc (right): https://codereview.chromium.org/2184023002/diff/40001/components/scheduler/re... components/scheduler/renderer/task_duration_reporter_unittest.cc:14: TaskDurationReporter tdr; On 2016/07/27 18:04:25, tdresser wrote: > task_duration_reporter > > We try to avoid short acronyms in general. Done. https://codereview.chromium.org/2184023002/diff/40001/components/scheduler/re... components/scheduler/renderer/task_duration_reporter_unittest.cc:16: int millisecond = 5 * 1000; On 2016/07/27 18:04:25, tdresser wrote: > Settings this to 5 * 1000 seems a bit odd. > > I'd probably just go with 1 millisecond most of the type, and then 2 > milliseconds on the 100th run, or something like that. Done. https://codereview.chromium.org/2184023002/diff/40001/components/scheduler/re... components/scheduler/renderer/task_duration_reporter_unittest.cc:16: int millisecond = 5 * 1000; On 2016/07/27 18:34:03, majidvp wrote: > nit: it is better to use a descriptive name such as > "duration_ms" instead of just millisecond. In fact you > can initialize duration here and not have two variables. Done. https://codereview.chromium.org/2184023002/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2184023002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:46339: + The duration of every 100th task queued in the scheduler to see the On 2016/07/27 18:04:25, tdresser wrote: > Might be worth commenting that this is queued in the _renderer_ scheduler. Done.
tdresser@chromium.org changed reviewers: + panicker@chromium.org
Please wait for panicker@ to take a look as well.
https://codereview.chromium.org/2184023002/diff/100001/components/scheduler/r... File components/scheduler/renderer/renderer_scheduler_impl.h (right): https://codereview.chromium.org/2184023002/diff/100001/components/scheduler/r... components/scheduler/renderer/renderer_scheduler_impl.h:475: nit: frivolous blank line
LGTM. Nit: Might be worth renaming to TaskTimeReporter vs TaskDurationReporter - incase we want to add additional dereived metrics instrumentation here.
The CQ bit was checked by sunyunjia@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tdresser@chromium.org, panicker@chromium.org Link to the patchset: https://codereview.chromium.org/2184023002/#ps140001 (title: "Rename TaskDurationReporeter->TaskTimeReporter")
The CQ bit was unchecked by sunyunjia@chromium.org
The CQ bit was checked by sunyunjia@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 sunyunjia@chromium.org
The CQ bit was checked by sunyunjia@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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Description was changed from ========== Added a class to report the duration of sampled tasks with histogram. BUG=630688 ========== to ========== Add a class to report the duration of sampled tasks with histogram. BUG=630688 ==========
sunyunjia@chromium.org changed reviewers: + skyostil@chromium.org
Hi Ilya and Sami, Please do the owner review for this patch. Thanks!
https://codereview.chromium.org/2184023002/diff/160001/components/scheduler/r... File components/scheduler/renderer/task_time_reporter.cc (right): https://codereview.chromium.org/2184023002/diff/160001/components/scheduler/r... components/scheduler/renderer/task_time_reporter.cc:22: duration.InMicroseconds(), 1, 1000000, 100); Do you really need 100 buckets, or would 50 suffice? https://codereview.chromium.org/2184023002/diff/160001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2184023002/diff/160001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:46361: + the distribution of the task duration. Would it be worthwhile to sample a random task among the 100, rather than always choosing the 100th? For example, the first handful of tasks handled by the scheduler are presumably a fairly stable set, and would never be reported on by this metric as currently implemented. Actually, taking a step back: Why did you choose to throttle this metric down to reporting on 1 out of 100 tasks, rather than for every task? Also, is this information already available from the jank profiler data?
https://codereview.chromium.org/2184023002/diff/160001/components/scheduler/r... File components/scheduler/renderer/task_time_reporter.cc (right): https://codereview.chromium.org/2184023002/diff/160001/components/scheduler/r... components/scheduler/renderer/task_time_reporter.cc:22: duration.InMicroseconds(), 1, 1000000, 100); On 2016/07/28 00:31:30, Ilya Sherman wrote: > Do you really need 100 buckets, or would 50 suffice? (Expanding this comment, now that I've read through the review history some:) What is your target granularity at which to see tasks? What sort of questions are you fundamentally trying to answer? 50 buckets tends to be a fairly good default choice; and I didn't see any clear indication in the review discussion so far as to what tradeoffs are being considered in choosing 100 buckets. (Why am I harping on this point? Histogram buckets take up memory, both on the client and on the server. For any given histogram this isn't a big difference; but across all of our histograms, it really does make a difference. And, this histogram will likely be reported with most buckets filled on most clients, so it might actually impact the server-side storage quite a bit.) FWIW, I suspect that downsampling to 1 out of 100 tasks will not tend to affect very much which buckets are filled on a given client ... but maybe you've tested, and observed a difference?
skyostil@chromium.org changed reviewers: + brianderson@chromium.org
https://codereview.chromium.org/2184023002/diff/160001/components/scheduler/r... File components/scheduler/renderer/task_time_reporter.cc (right): https://codereview.chromium.org/2184023002/diff/160001/components/scheduler/r... components/scheduler/renderer/task_time_reporter.cc:20: if (sample_count_ == kSampleInterval) { +brianderson@, didn't we figure out that reducing the histogram sampling rate doesn't actually reduce data volume? If so, we could just record the histogram directly in the calling code.
https://codereview.chromium.org/2184023002/diff/160001/components/scheduler/r... File components/scheduler/renderer/task_time_reporter.cc (right): https://codereview.chromium.org/2184023002/diff/160001/components/scheduler/r... components/scheduler/renderer/task_time_reporter.cc:20: if (sample_count_ == kSampleInterval) { On 2016/07/28 11:20:26, Sami wrote: > +brianderson@, didn't we figure out that reducing the histogram sampling rate > doesn't actually reduce data volume? If so, we could just record the histogram > directly in the calling code. Yeah, sounds like that's the case. https://codereview.chromium.org/2184023002/diff/160001/components/scheduler/r... components/scheduler/renderer/task_time_reporter.cc:22: duration.InMicroseconds(), 1, 1000000, 100); On 2016/07/28 00:38:20, Ilya Sherman wrote: > On 2016/07/28 00:31:30, Ilya Sherman wrote: > > Do you really need 100 buckets, or would 50 suffice? > > (Expanding this comment, now that I've read through the review history some:) > > What is your target granularity at which to see tasks? What sort of questions > are you fundamentally trying to answer? 50 buckets tends to be a fairly good > default choice; and I didn't see any clear indication in the review discussion > so far as to what tradeoffs are being considered in choosing 100 buckets. (Why > am I harping on this point? Histogram buckets take up memory, both on the > client and on the server. For any given histogram this isn't a big difference; > but across all of our histograms, it really does make a difference. And, this > histogram will likely be reported with most buckets filled on most clients, so > it might actually impact the server-side storage quite a bit.) > > FWIW, I suspect that downsampling to 1 out of 100 tasks will not tend to affect > very much which buckets are filled on a given client ... but maybe you've > tested, and observed a difference? Nope, you're right, in terms of memory used, downsampling won't matter. I assume that histogram recording is fast enough that we don't need to worry about the performance of incrementing a histogram, even if we are doing it every task? I think 100 buckets would be a reasonable choice here. Either that, or we'll need multiple histograms, but I think this is more straight forward. There are a variety of questions we want to answer here. • What's the distribution of short tasks? We want detail here in the 10 microsecond – 50ms range. We could drop a few buckets off the bottom here I suppose. • What's the distribution of long tasks? We want detail here in the 50ms – 1s range. • How does the number of short tasks (<50ms) compare to the number of long tasks (>50ms)? Seeing the full distribution in detail will make us more sensitive to changes in the distribution, as well as giving us more insight into the magnitude of tasks that we see on the web.
https://codereview.chromium.org/2184023002/diff/160001/components/scheduler/r... File components/scheduler/renderer/task_time_reporter.cc (right): https://codereview.chromium.org/2184023002/diff/160001/components/scheduler/r... components/scheduler/renderer/task_time_reporter.cc:20: if (sample_count_ == kSampleInterval) { On 2016/07/28 12:15:50, tdresser wrote: > On 2016/07/28 11:20:26, Sami wrote: > > +brianderson@, didn't we figure out that reducing the histogram sampling rate > > doesn't actually reduce data volume? If so, we could just record the histogram > > directly in the calling code. > > Yeah, sounds like that's the case. The other reason that we considered down-sampling was to avoid the CPU cost. Looking at the histogram recording code it does a binary-search to find the buckets and three atomic increments. This is compared to an increments & a branch in down-sampled version. To be honest I am not sure what the impact that difference makes so I suggest we check both against a micro-benchmark on Android and if we don't see much difference we can just record every time.
On 2016/07/28 13:30:11, majidvp wrote: > https://codereview.chromium.org/2184023002/diff/160001/components/scheduler/r... > File components/scheduler/renderer/task_time_reporter.cc (right): > > https://codereview.chromium.org/2184023002/diff/160001/components/scheduler/r... > components/scheduler/renderer/task_time_reporter.cc:20: if (sample_count_ == > kSampleInterval) { > On 2016/07/28 12:15:50, tdresser wrote: > > On 2016/07/28 11:20:26, Sami wrote: > > > +brianderson@, didn't we figure out that reducing the histogram sampling > rate > > > doesn't actually reduce data volume? If so, we could just record the > histogram > > > directly in the calling code. > > > > Yeah, sounds like that's the case. > > The other reason that we considered down-sampling was to avoid the CPU cost. > Looking at the histogram recording code it does a binary-search to find the > buckets and three atomic increments. This is compared to an increments & a > branch in down-sampled version. > > To be honest I am not sure what the impact that difference makes so > I suggest we check both against a micro-benchmark on Android and if we > don't see much difference we can just record every time. Sounds good.
https://codereview.chromium.org/2184023002/diff/160001/components/scheduler/r... File components/scheduler/renderer/task_time_reporter.cc (right): https://codereview.chromium.org/2184023002/diff/160001/components/scheduler/r... components/scheduler/renderer/task_time_reporter.cc:22: duration.InMicroseconds(), 1, 1000000, 100); On 2016/07/28 12:15:50, tdresser wrote: > On 2016/07/28 00:38:20, Ilya Sherman wrote: > > On 2016/07/28 00:31:30, Ilya Sherman wrote: > > > Do you really need 100 buckets, or would 50 suffice? > > > > (Expanding this comment, now that I've read through the review history some:) > > > > What is your target granularity at which to see tasks? What sort of questions > > are you fundamentally trying to answer? 50 buckets tends to be a fairly good > > default choice; and I didn't see any clear indication in the review discussion > > so far as to what tradeoffs are being considered in choosing 100 buckets. > (Why > > am I harping on this point? Histogram buckets take up memory, both on the > > client and on the server. For any given histogram this isn't a big > difference; > > but across all of our histograms, it really does make a difference. And, this > > histogram will likely be reported with most buckets filled on most clients, so > > it might actually impact the server-side storage quite a bit.) > > > > FWIW, I suspect that downsampling to 1 out of 100 tasks will not tend to > affect > > very much which buckets are filled on a given client ... but maybe you've > > tested, and observed a difference? > > Nope, you're right, in terms of memory used, downsampling won't matter. I assume > that histogram recording is fast enough that we don't need to worry about the > performance of incrementing a histogram, even if we are doing it every task? > > I think 100 buckets would be a reasonable choice here. Either that, or we'll > need multiple histograms, but I think this is more straight forward. > > There are a variety of questions we want to answer here. > • What's the distribution of short tasks? > We want detail here in the 10 microsecond – 50ms range. > We could drop a few buckets off the bottom here I suppose. > • What's the distribution of long tasks? > We want detail here in the 50ms – 1s range. > • How does the number of short tasks (<50ms) compare to the > number of long tasks (>50ms)? > > Seeing the full distribution in detail will make us more sensitive to changes in > the distribution, as well as giving us more insight into the magnitude of tasks > that we see on the web. > Let's be really concrete here. With 50 buckets, you'd have these bucket boundaries: 1us 2us 3us 4us 5us 7us 9us 12us 16us 21us 28us 37us 49us 65us 86us 113us 149us 196us 258us 340us 448us 590us 777us 1ms 1ms 1ms 2ms 3ms 4ms 5ms 7ms 9ms 12ms 16ms 21ms 27ms 36ms 48ms 63ms 83ms 110ms 145ms 191ms 252ms 332ms 437ms 576ms 759ms 1s With 100 buckets, you'd have these: 1us 2us 3us 4us 5us 6us 7us 8us 9us 10us 11us 13us 15us 17us 19us 22us 25us 28us 32us 36us 41us 47us 53us 60us 68us 77us 88us 100us 114us 130us 148us 168us 191us 217us 247us 281us 320us 364us 414us 471us 536us 610us 695us 791us 901us 1ms 1ms 1ms 1ms 1ms 1ms 2ms 2ms 2ms 3ms 3ms 4ms 4ms 5ms 6ms 7ms 8ms 9ms 10ms 12ms 13ms 15ms 17ms 20ms 23ms 26ms 30ms 34ms 38ms 44ms 50ms 57ms 65ms 74ms 84ms 96ms 109ms 125ms 142ms 162ms 184ms 210ms 239ms 272ms 310ms 353ms 402ms 458ms 522ms 594ms 677ms 771ms 878ms 1s With 75, you'd have these: 1us 2us 3us 4us 5us 6us 7us 8us 10us 12us 14us 17us 20us 24us 29us 35us 42us 50us 60us 72us 86us 103us 123us 147us 175us 209us 249us 297us 354us 422us 504us 601us 717us 855us 1ms 1ms 1ms 1ms 2ms 2ms 2ms 3ms 4ms 4ms 5ms 7ms 8ms 10ms 12ms 14ms 17ms 20ms 24ms 29ms 34ms 41ms 49ms 59ms 70ms 84ms 100ms 120ms 143ms 170ms 203ms 243ms 290ms 346ms 413ms 493ms 588ms 702ms 838ms 1000ms Where, specifically, do you not get quite enough resolution with 50 buckets? Are there values between 50 and 100 that might give you the desired resolution? Because this histogram is going to be emitted to very often, it will account for a fairly sizable footprint of our data storage on the server side, so it's worth trying to optimize the number of buckets somewhat. That said, if you really need the resolution that 100 buckets provides, then maybe that's the right value. I agree that it's better to have a single histogram that captures the full range, rather than multiple histograms.
On 2016/07/28 17:47:41, Ilya Sherman wrote: > https://codereview.chromium.org/2184023002/diff/160001/components/scheduler/r... > File components/scheduler/renderer/task_time_reporter.cc (right): > > https://codereview.chromium.org/2184023002/diff/160001/components/scheduler/r... > components/scheduler/renderer/task_time_reporter.cc:22: > duration.InMicroseconds(), 1, 1000000, 100); > On 2016/07/28 12:15:50, tdresser wrote: > > On 2016/07/28 00:38:20, Ilya Sherman wrote: > > > On 2016/07/28 00:31:30, Ilya Sherman wrote: > > > > Do you really need 100 buckets, or would 50 suffice? > > > > > > (Expanding this comment, now that I've read through the review history > some:) > > > > > > What is your target granularity at which to see tasks? What sort of > questions > > > are you fundamentally trying to answer? 50 buckets tends to be a fairly > good > > > default choice; and I didn't see any clear indication in the review > discussion > > > so far as to what tradeoffs are being considered in choosing 100 buckets. > > (Why > > > am I harping on this point? Histogram buckets take up memory, both on the > > > client and on the server. For any given histogram this isn't a big > > difference; > > > but across all of our histograms, it really does make a difference. And, > this > > > histogram will likely be reported with most buckets filled on most clients, > so > > > it might actually impact the server-side storage quite a bit.) > > > > > > FWIW, I suspect that downsampling to 1 out of 100 tasks will not tend to > > affect > > > very much which buckets are filled on a given client ... but maybe you've > > > tested, and observed a difference? > > > > Nope, you're right, in terms of memory used, downsampling won't matter. I > assume > > that histogram recording is fast enough that we don't need to worry about the > > performance of incrementing a histogram, even if we are doing it every task? > > > > I think 100 buckets would be a reasonable choice here. Either that, or we'll > > need multiple histograms, but I think this is more straight forward. > > > > There are a variety of questions we want to answer here. > > • What's the distribution of short tasks? > > We want detail here in the 10 microsecond – 50ms range. > > We could drop a few buckets off the bottom here I suppose. > > • What's the distribution of long tasks? > > We want detail here in the 50ms – 1s range. > > • How does the number of short tasks (<50ms) compare to the > > number of long tasks (>50ms)? > > > > Seeing the full distribution in detail will make us more sensitive to changes > in > > the distribution, as well as giving us more insight into the magnitude of > tasks > > that we see on the web. > > > > Let's be really concrete here. With 50 buckets, you'd have these bucket > boundaries: > > 1us 2us 3us 4us 5us 7us 9us 12us 16us 21us 28us 37us 49us 65us 86us 113us 149us > 196us 258us 340us 448us 590us 777us 1ms 1ms 1ms 2ms 3ms 4ms 5ms 7ms 9ms 12ms > 16ms 21ms 27ms 36ms 48ms 63ms 83ms 110ms 145ms 191ms 252ms 332ms 437ms 576ms > 759ms 1s > > With 100 buckets, you'd have these: > > 1us 2us 3us 4us 5us 6us 7us 8us 9us 10us 11us 13us 15us 17us 19us 22us 25us 28us > 32us 36us 41us 47us 53us 60us 68us 77us 88us 100us 114us 130us 148us 168us 191us > 217us 247us 281us 320us 364us 414us 471us 536us 610us 695us 791us 901us 1ms 1ms > 1ms 1ms 1ms 1ms 2ms 2ms 2ms 3ms 3ms 4ms 4ms 5ms 6ms 7ms 8ms 9ms 10ms 12ms 13ms > 15ms 17ms 20ms 23ms 26ms 30ms 34ms 38ms 44ms 50ms 57ms 65ms 74ms 84ms 96ms 109ms > 125ms 142ms 162ms 184ms 210ms 239ms 272ms 310ms 353ms 402ms 458ms 522ms 594ms > 677ms 771ms 878ms 1s > > With 75, you'd have these: > > 1us 2us 3us 4us 5us 6us 7us 8us 10us 12us 14us 17us 20us 24us 29us 35us 42us > 50us 60us 72us 86us 103us 123us 147us 175us 209us 249us 297us 354us 422us 504us > 601us 717us 855us 1ms 1ms 1ms 1ms 2ms 2ms 2ms 3ms 4ms 4ms 5ms 7ms 8ms 10ms 12ms > 14ms 17ms 20ms 24ms 29ms 34ms 41ms 49ms 59ms 70ms 84ms 100ms 120ms 143ms 170ms > 203ms 243ms 290ms 346ms 413ms 493ms 588ms 702ms 838ms 1000ms > > Where, specifically, do you not get quite enough resolution with 50 buckets? > Are there values between 50 and 100 that might give you the desired resolution? > Because this histogram is going to be emitted to very often, it will account for > a fairly sizable footprint of our data storage on the server side, so it's worth > trying to optimize the number of buckets somewhat. That said, if you really > need the resolution that 100 buckets provides, then maybe that's the right > value. I agree that it's better to have a single histogram that captures the > full range, rather than multiple histograms. Thanks for making this so concrete. 50 buckets gives us ~10 buckets for describing the distribution of long tasks, which is pretty coarse. However, it isn't that unreasonable. We can always land with 50 buckets, and then replace the metric with a version with 100 buckets if it seems necessary. SGTM to land with only 50 buckets.
The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The bucket number is reduced to 50. To compare the sampling version and non-sampling version, we ran two benchmarks: page_cycler.top_10_mobile and thread_times.simple_mobile_sites. There are improvements and regressions, but they all fall in the delta and the difference is not significant. So for now we remove the sampling part. We can always add the down sampling back later if we see obvious regression after landing. The result can be viewed here (may need to download the html file and the other folder): https://drive.google.com/open?id=0B_svnnFdGmtuU1JlemZDZ3VzaFE
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
metrics LGTM, thanks. https://codereview.chromium.org/2184023002/diff/180001/components/scheduler/r... File components/scheduler/renderer/task_time_reporter.cc (right): https://codereview.chromium.org/2184023002/diff/180001/components/scheduler/r... components/scheduler/renderer/task_time_reporter.cc:16: duration.InMicroseconds(), 1, 1000000, 50); Optional nit: It seems rather heavy-weight to define an entire class for this one macro call.
https://codereview.chromium.org/2184023002/diff/180001/components/scheduler/r... File components/scheduler/renderer/task_time_reporter.cc (right): https://codereview.chromium.org/2184023002/diff/180001/components/scheduler/r... components/scheduler/renderer/task_time_reporter.cc:16: duration.InMicroseconds(), 1, 1000000, 50); On 2016/08/03 00:54:46, Ilya Sherman wrote: > Optional nit: It seems rather heavy-weight to define an entire class for this > one macro call. I agree -- could you move the macro to the call site?
The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
I just removed the class. Now it is directly calling from renderer_scheduler_impl.
Thanks, lgtm.
sunyunjia@: this CL no longer adds a new class so please update the CL description to reflect this before committing. I see that we no longer have the unit test. It is probably fine given that this just adds a simple histogram now.
Description was changed from ========== Add a class to report the duration of sampled tasks with histogram. BUG=630688 ========== to ========== Report the duration of sampled tasks with histogram. BUG=630688 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run
Dry run: 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
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by sunyunjia@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tdresser@chromium.org, panicker@chromium.org, isherman@chromium.org Link to the patchset: https://codereview.chromium.org/2184023002/#ps200001 (title: "Directly call from renderer_scheduler_impl.cc")
The CQ bit was unchecked by sunyunjia@chromium.org
The CQ bit was checked by sunyunjia@chromium.org
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 ========== Report the duration of sampled tasks with histogram. BUG=630688 ========== to ========== Report the duration of sampled tasks with histogram. BUG=630688 ==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== Report the duration of sampled tasks with histogram. BUG=630688 ========== to ========== Report the duration of sampled tasks with histogram. BUG=630688 Committed: https://crrev.com/1aca73dda839f813d2a4d6fbcf654ed488a197dd Cr-Commit-Position: refs/heads/master@{#409606} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/1aca73dda839f813d2a4d6fbcf654ed488a197dd Cr-Commit-Position: refs/heads/master@{#409606} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
