Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2658583005:
[system-health] Only flush system cache on mobile (Closed)
DescriptionOnly flush system cache on mobile. Flushing the cache on
desktop increases runtime significantly.
BUG=685240
Review-Url: https://codereview.chromium.org/2658583005
Cr-Commit-Position: refs/heads/master@{#446396}
Committed: https://chromium.googlesource.com/chromium/src/+/ce9c223faabd70c29759bc6f5d8844f44b40cede
Patch Set 1 #
Messages
Total messages: 17 (6 generated)
Description was changed from ========== Only flush system cache on mobile BUG=685240 ========== to ========== Only flush system cache on mobile. Flushing the cache on desktop increases runtime significantly. BUG=685240 ==========
hjd@chromium.org changed reviewers: + nednguyen@google.com, perezju@chromium.org
ptal, thanks!
On 2017/01/26 16:29:21, hjd wrote: > ptal, thanks! lgtm
On 2017/01/26 17:21:28, nednguyen wrote: > On 2017/01/26 16:29:21, hjd wrote: > > ptal, thanks! > > lgtm Would this cause the bisect reproducible problem on desktop? cc'ed Simon
On 2017/01/26 17:22:37, nednguyen wrote: > On 2017/01/26 17:21:28, nednguyen wrote: > > On 2017/01/26 16:29:21, hjd wrote: > > > ptal, thanks! > > > > lgtm > > Would this cause the bisect reproducible problem on desktop? > > cc'ed Simon The bisect reproducible problem wasn't observed on desktop afaik (I mentioned desktop in the CL message but I think that was wrong)
The CQ bit was checked by hjd@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/01/26 17:28:36, commit-bot: I haz the power wrote: > CQ is trying da patch. Follow status at > > https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or... That's right, it was system_health.memory_mobile we noticed wasn't repro'ing.
On 2017/01/26 17:31:14, shatch wrote: > On 2017/01/26 17:28:36, commit-bot: I haz the power wrote: > > CQ is trying da patch. Follow status at > > > > > https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or... > > That's right, it was system_health.memory_mobile we noticed wasn't repro'ing. the fix lgtm, thanks Hector! Re: mobile vs desktop, note that our observations are probably heavily biased by the fact that we have automated alerts on mobile but not on desktop.
On 2017/01/26 18:09:36, perezju wrote: > On 2017/01/26 17:31:14, shatch wrote: > > On 2017/01/26 17:28:36, commit-bot: I haz the power wrote: > > > CQ is trying da patch. Follow status at > > > > > > > > > https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or... > > > > That's right, it was system_health.memory_mobile we noticed wasn't repro'ing. > > the fix lgtm, thanks Hector! > > Re: mobile vs desktop, note that our observations are probably heavily biased by > the fact that we have automated alerts on mobile but not on desktop. CC primiano@ because he is planning to enable alerts on memory desktop bot
primiano@chromium.org changed reviewers: + primiano@chromium.org
We havent' enabled anything yet, so SGTM to proceed. Just +erikchen FYI as this might be one factor to keep in mind when evaluating noise of the benchmarks Only one note about the current CL: the commit message should contain the title as well. What gets committed is only what you see in the description, which right now is: ----- Only flush system cache on mobile. Flushing the cache on desktop increases runtime significantly. BUG=685240 ----- Instead it should be: ----- [system-health] Only flush system cache on mobile Only flush system cache on mobile. Flushing the cache on desktop increases runtime significantly. BUG=685240 ----- Also, as a general comment, commit titles should be understandable by anybody in chrome. Not sure if anybody knows what system health is. What you really want to clarify here is: - the fact that you are not touch production code of chrome - the fact that you are touching benchmarks code So i'd label this: telemetry: Flush caches only on mobile versions of system health benchmark
CQ is committing da patch.
Bot data: {"patchset_id": 1, "attempt_start_ts": 1485451697886930, "parent_rev":
"2288cb3fae6baa710b0228f292e3f368c32edca1", "commit_rev":
"ce9c223faabd70c29759bc6f5d8844f44b40cede"}
Message was sent while issue was closed.
Description was changed from ========== Only flush system cache on mobile. Flushing the cache on desktop increases runtime significantly. BUG=685240 ========== to ========== Only flush system cache on mobile. Flushing the cache on desktop increases runtime significantly. BUG=685240 Review-Url: https://codereview.chromium.org/2658583005 Cr-Commit-Position: refs/heads/master@{#446396} Committed: https://chromium.googlesource.com/chromium/src/+/ce9c223faabd70c29759bc6f5d88... ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1) as https://chromium.googlesource.com/chromium/src/+/ce9c223faabd70c29759bc6f5d88...
Message was sent while issue was closed.
On 2017/01/26 19:38:42, commit-bot: I haz the power wrote: > Committed patchset #1 (id:1) as > https://chromium.googlesource.com/chromium/src/+/ce9c223faabd70c29759bc6f5d88... Sorry, I had left work already and the CQ bit was checked :( |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
