Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
364913007:
metrics: Add random delays to perf collection (Closed)
|
Created:
6 years, 5 months ago by Simon Que Modified:
6 years, 5 months ago CC:
chromium-reviews, asvitkine+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
Descriptionmetrics: Add random delays to perf collection
When collecting upon resume from suspend or session restore, don't always
collect right away. Introduce a random delay before collecting.
BUG=chromium:358778
TEST=Do the following:
- Add logging to ParseProtoIfValid().
- Set kResumeSamplingFactor=1 and kRestoreSessionSamplingFactor=1. This makes
collection happen 100% of the time.
- Set kPerfProfilingIntervalMs=20000. This makes the periodic collection happen once every 20-second interval.
- Trigger both types of collections, resume and restore:
= Suspend and resume the system.
= Open some tabs in ChromeOS, log out of Chrome, and then log back in so that
the session is restored.
- Should see the logging trace displayed in /var/log/ui/ui.LATEST after a
random delay.
- Should also see the periodic collections take place.
= Open an incognito window. It should continue to attempt to collect. (Add a trace to ScheduleIntervalCollection to see that this is the case).
Signed-off-by: Simon Que <sque@chromium.org>;
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=283674
Patch Set 1 #
Total comments: 9
Patch Set 2 : Pass delays as TimeDeltas; Renamed functions; Disable all timers if logged out; Do not collect when… #
Total comments: 1
Patch Set 3 : Use only one timer #
Total comments: 4
Patch Set 4 : Schedule new collection before early return #
Total comments: 1
Patch Set 5 : Schedule next interval collection if collection never takes place #
Total comments: 2
Patch Set 6 : Call ScheduleIntervalCollection at the beginning #
Messages
Total messages: 19 (0 generated)
https://codereview.chromium.org/364913007/diff/1/chrome/browser/metrics/perf_... File chrome/browser/metrics/perf_provider_chromeos.cc (right): https://codereview.chromium.org/364913007/diff/1/chrome/browser/metrics/perf_... chrome/browser/metrics/perf_provider_chromeos.cc:262: interval_timer_.Stop(); Should this stop the other timers as well? In fact, do you really need multiple timers, or can you just use a single timer for all three use cases? https://codereview.chromium.org/364913007/diff/1/chrome/browser/metrics/perf_... File chrome/browser/metrics/perf_provider_chromeos.h (right): https://codereview.chromium.org/364913007/diff/1/chrome/browser/metrics/perf_... chrome/browser/metrics/perf_provider_chromeos.h:88: // Collects perf data after a resume. |sleep_duration_ms| is the duration the nit: Perhaps name the variable "suspend_duration", then? https://codereview.chromium.org/364913007/diff/1/chrome/browser/metrics/perf_... chrome/browser/metrics/perf_provider_chromeos.h:91: void DoResumeCollection(uint64 sleep_duration_ms, nit: Perhaps something more like "CollectPerfDataAfterResume" (and a similar change below)? https://codereview.chromium.org/364913007/diff/1/chrome/browser/metrics/perf_... chrome/browser/metrics/perf_provider_chromeos.h:92: uint64 time_after_resume_ms); Please pass these as TimeDeltas.
https://codereview.chromium.org/364913007/diff/1/chrome/browser/metrics/perf_... File chrome/browser/metrics/perf_provider_chromeos.cc (right): https://codereview.chromium.org/364913007/diff/1/chrome/browser/metrics/perf_... chrome/browser/metrics/perf_provider_chromeos.cc:262: interval_timer_.Stop(); On 2014/07/03 00:12:47, Ilya Sherman wrote: > Should this stop the other timers as well? In fact, do you really need multiple > timers, or can you just use a single timer for all three use cases? Done. I think the three timers are needed so that they run independently. https://codereview.chromium.org/364913007/diff/1/chrome/browser/metrics/perf_... File chrome/browser/metrics/perf_provider_chromeos.h (right): https://codereview.chromium.org/364913007/diff/1/chrome/browser/metrics/perf_... chrome/browser/metrics/perf_provider_chromeos.h:88: // Collects perf data after a resume. |sleep_duration_ms| is the duration the On 2014/07/03 00:12:48, Ilya Sherman wrote: > nit: Perhaps name the variable "suspend_duration", then? Done. https://codereview.chromium.org/364913007/diff/1/chrome/browser/metrics/perf_... chrome/browser/metrics/perf_provider_chromeos.h:91: void DoResumeCollection(uint64 sleep_duration_ms, On 2014/07/03 00:12:48, Ilya Sherman wrote: > nit: Perhaps something more like "CollectPerfDataAfterResume" (and a similar > change below)? Done. https://codereview.chromium.org/364913007/diff/1/chrome/browser/metrics/perf_... chrome/browser/metrics/perf_provider_chromeos.h:92: uint64 time_after_resume_ms); On 2014/07/03 00:12:48, Ilya Sherman wrote: > Please pass these as TimeDeltas. Done.
https://codereview.chromium.org/364913007/diff/1/chrome/browser/metrics/perf_... File chrome/browser/metrics/perf_provider_chromeos.cc (right): https://codereview.chromium.org/364913007/diff/1/chrome/browser/metrics/perf_... chrome/browser/metrics/perf_provider_chromeos.cc:262: interval_timer_.Stop(); On 2014/07/07 19:45:26, Simon Que wrote: > On 2014/07/03 00:12:47, Ilya Sherman wrote: > > Should this stop the other timers as well? In fact, do you really need > multiple > > timers, or can you just use a single timer for all three use cases? > > Done. I think the three timers are needed so that they run independently. Why do the timers need to run independently? https://codereview.chromium.org/364913007/diff/20001/chrome/browser/metrics/p... File chrome/browser/metrics/perf_provider_chromeos.cc (right): https://codereview.chromium.org/364913007/diff/20001/chrome/browser/metrics/p... chrome/browser/metrics/perf_provider_chromeos.cc:216: collection_delay)); By the way, a timer set to run in 1250ms is not guaranteed to run precisely 1250ms later -- it might be delayed, e.g. because the UI thread is busy with other tasks. Is that something you want to account for in the metrics? If not, I'd recommend at least adding a comment to the proto file that this could be a source of bias.
On 2014/07/08 00:06:40, Ilya Sherman wrote: > https://codereview.chromium.org/364913007/diff/1/chrome/browser/metrics/perf_... > File chrome/browser/metrics/perf_provider_chromeos.cc (right): > > https://codereview.chromium.org/364913007/diff/1/chrome/browser/metrics/perf_... > chrome/browser/metrics/perf_provider_chromeos.cc:262: interval_timer_.Stop(); > On 2014/07/07 19:45:26, Simon Que wrote: > > On 2014/07/03 00:12:47, Ilya Sherman wrote: > > > Should this stop the other timers as well? In fact, do you really need > > multiple > > > timers, or can you just use a single timer for all three use cases? > > > > Done. I think the three timers are needed so that they run independently. > > Why do the timers need to run independently? Can you suggest a way to do it with one timer? I am thinking there could be a queue of collection events but this will require some more logic. > https://codereview.chromium.org/364913007/diff/20001/chrome/browser/metrics/p... > File chrome/browser/metrics/perf_provider_chromeos.cc (right): > > https://codereview.chromium.org/364913007/diff/20001/chrome/browser/metrics/p... > chrome/browser/metrics/perf_provider_chromeos.cc:216: collection_delay)); > By the way, a timer set to run in 1250ms is not guaranteed to run precisely > 1250ms later -- it might be delayed, e.g. because the UI thread is busy with > other tasks. Is that something you want to account for in the metrics? If not, > I'd recommend at least adding a comment to the proto file that this could be a > source of bias. I'll give it a try...
On 2014/07/09 23:30:15, Simon Que wrote: > On 2014/07/08 00:06:40, Ilya Sherman wrote: > > > https://codereview.chromium.org/364913007/diff/1/chrome/browser/metrics/perf_... > > File chrome/browser/metrics/perf_provider_chromeos.cc (right): > > > > > https://codereview.chromium.org/364913007/diff/1/chrome/browser/metrics/perf_... > > chrome/browser/metrics/perf_provider_chromeos.cc:262: interval_timer_.Stop(); > > On 2014/07/07 19:45:26, Simon Que wrote: > > > On 2014/07/03 00:12:47, Ilya Sherman wrote: > > > > Should this stop the other timers as well? In fact, do you really need > > > multiple > > > > timers, or can you just use a single timer for all three use cases? > > > > > > Done. I think the three timers are needed so that they run independently. > > > > Why do the timers need to run independently? > > Can you suggest a way to do it with one timer? I am thinking there could be a > queue of collection events but this will require some more logic. Let me clarify my question: Suppose that the computer just woke up from sleep. Why not cancel any periodic timer that had been going, collect the data after sleep, and then schedule a new periodic collection? Why do you need a queue or multiple timers or anything more complex like that?
On 2014/07/09 23:34:22, Ilya Sherman wrote: > On 2014/07/09 23:30:15, Simon Que wrote: > > On 2014/07/08 00:06:40, Ilya Sherman wrote: > > > > > > https://codereview.chromium.org/364913007/diff/1/chrome/browser/metrics/perf_... > > > File chrome/browser/metrics/perf_provider_chromeos.cc (right): > > > > > > > > > https://codereview.chromium.org/364913007/diff/1/chrome/browser/metrics/perf_... > > > chrome/browser/metrics/perf_provider_chromeos.cc:262: > interval_timer_.Stop(); > > > On 2014/07/07 19:45:26, Simon Que wrote: > > > > On 2014/07/03 00:12:47, Ilya Sherman wrote: > > > > > Should this stop the other timers as well? In fact, do you really need > > > > multiple > > > > > timers, or can you just use a single timer for all three use cases? > > > > > > > > Done. I think the three timers are needed so that they run independently. > > > > > > Why do the timers need to run independently? > > > > Can you suggest a way to do it with one timer? I am thinking there could be a > > queue of collection events but this will require some more logic. > > Let me clarify my question: Suppose that the computer just woke up from sleep. > Why not cancel any periodic timer that had been going, collect the data after > sleep, and then schedule a new periodic collection? Why do you need a queue or > multiple timers or anything more complex like that? OK how about this scenario... User logs in, restores previous session, and there is a session restore collection scheduled 8 seconds later. User suspends and resumes. Now it's time to schedule a resume collection, but the timer is already running for the session restore. What should be the behavior?
On 2014/07/10 01:03:58, Simon Que wrote: > On 2014/07/09 23:34:22, Ilya Sherman wrote: > > On 2014/07/09 23:30:15, Simon Que wrote: > > > On 2014/07/08 00:06:40, Ilya Sherman wrote: > > > > > > > > > > https://codereview.chromium.org/364913007/diff/1/chrome/browser/metrics/perf_... > > > > File chrome/browser/metrics/perf_provider_chromeos.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/364913007/diff/1/chrome/browser/metrics/perf_... > > > > chrome/browser/metrics/perf_provider_chromeos.cc:262: > > interval_timer_.Stop(); > > > > On 2014/07/07 19:45:26, Simon Que wrote: > > > > > On 2014/07/03 00:12:47, Ilya Sherman wrote: > > > > > > Should this stop the other timers as well? In fact, do you really > need > > > > > multiple > > > > > > timers, or can you just use a single timer for all three use cases? > > > > > > > > > > Done. I think the three timers are needed so that they run > independently. > > > > > > > > Why do the timers need to run independently? > > > > > > Can you suggest a way to do it with one timer? I am thinking there could be > a > > > queue of collection events but this will require some more logic. > > > > Let me clarify my question: Suppose that the computer just woke up from sleep. > > > Why not cancel any periodic timer that had been going, collect the data after > > sleep, and then schedule a new periodic collection? Why do you need a queue > or > > multiple timers or anything more complex like that? > > OK how about this scenario... > > User logs in, restores previous session, and there is a session restore > collection scheduled 8 seconds later. > > User suspends and resumes. Now it's time to schedule a resume collection, but > the timer is already running for the session restore. > > What should be the behavior? I would expect the session restore timer to be cancelled, since its no longer relevant, and instead for the resume timer to replace it. In general, I'd expect the periodic collection timer to be active when no other time supercedes it; and otherwise, the timer from the most recent trigger event to be active.
On 2014/07/11 01:12:44, Ilya Sherman wrote: > I would expect the session restore timer to be cancelled, since its no longer > relevant, and instead for the resume timer to replace it. In general, I'd > expect the periodic collection timer to be active when no other time supercedes > it; and otherwise, the timer from the most recent trigger event to be active. Done.
https://codereview.chromium.org/364913007/diff/40001/chrome/browser/metrics/p... File chrome/browser/metrics/perf_provider_chromeos.cc (right): https://codereview.chromium.org/364913007/diff/40001/chrome/browser/metrics/p... chrome/browser/metrics/perf_provider_chromeos.cc:236: timer_.Stop(); It looks like if the early-return below is reached, then no periodic samples will be collected. I think you probably want to move this down below the final early return. https://codereview.chromium.org/364913007/diff/40001/chrome/browser/metrics/p... chrome/browser/metrics/perf_provider_chromeos.cc:404: ScheduleIntervalCollection(); What if there was an early return from CollectIfNecessary? It looks like that will suspend collection indefinitely. Is that intentional?
https://codereview.chromium.org/364913007/diff/40001/chrome/browser/metrics/p... File chrome/browser/metrics/perf_provider_chromeos.cc (right): https://codereview.chromium.org/364913007/diff/40001/chrome/browser/metrics/p... chrome/browser/metrics/perf_provider_chromeos.cc:236: timer_.Stop(); On 2014/07/12 03:11:56, Ilya Sherman wrote: > It looks like if the early-return below is reached, then no periodic samples > will be collected. I think you probably want to move this down below the final > early return. Done. https://codereview.chromium.org/364913007/diff/40001/chrome/browser/metrics/p... chrome/browser/metrics/perf_provider_chromeos.cc:404: ScheduleIntervalCollection(); On 2014/07/12 03:11:56, Ilya Sherman wrote: > What if there was an early return from CollectIfNecessary? It looks like that > will suspend collection indefinitely. Is that intentional? Done.
https://codereview.chromium.org/364913007/diff/60001/chrome/browser/metrics/p... File chrome/browser/metrics/perf_provider_chromeos.cc (right): https://codereview.chromium.org/364913007/diff/60001/chrome/browser/metrics/p... chrome/browser/metrics/perf_provider_chromeos.cc:375: ScheduleIntervalCollection(); What if there was an early return from CollectIfNecessary? It looks like that will suspend collection indefinitely. Is that intentional?
On 2014/07/15 04:29:23, Ilya Sherman wrote: > https://codereview.chromium.org/364913007/diff/60001/chrome/browser/metrics/p... > File chrome/browser/metrics/perf_provider_chromeos.cc (right): > > https://codereview.chromium.org/364913007/diff/60001/chrome/browser/metrics/p... > chrome/browser/metrics/perf_provider_chromeos.cc:375: > ScheduleIntervalCollection(); > What if there was an early return from CollectIfNecessary? It looks like that > will suspend collection indefinitely. Is that intentional? Done. I tested the case where an incognito window gets opened, and it will keep trying.
https://codereview.chromium.org/364913007/diff/80001/chrome/browser/metrics/p... File chrome/browser/metrics/perf_provider_chromeos.cc (right): https://codereview.chromium.org/364913007/diff/80001/chrome/browser/metrics/p... chrome/browser/metrics/perf_provider_chromeos.cc:310: ScheduleIntervalCollection(); Why not just call this at the beginning of CollectIfNecessary()? Then you'd only need to call it from one place...
https://codereview.chromium.org/364913007/diff/80001/chrome/browser/metrics/p... File chrome/browser/metrics/perf_provider_chromeos.cc (right): https://codereview.chromium.org/364913007/diff/80001/chrome/browser/metrics/p... chrome/browser/metrics/perf_provider_chromeos.cc:310: ScheduleIntervalCollection(); On 2014/07/16 22:01:19, Ilya Sherman wrote: > Why not just call this at the beginning of CollectIfNecessary()? Then you'd > only need to call it from one place... Done.
LGTM.
The CQ bit was checked by sque@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sque@chromium.org/364913007/100001
Message was sent while issue was closed.
Change committed as 283674 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
