Implement UMA metric JankyIntervalsPerThirtySeconds |
||||||||||
Issue description
,
Jul 24
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/6973776c884d8e5e37cb8d3c100e30c01907caf2 commit 6973776c884d8e5e37cb8d3c100e30c01907caf2 Author: Erik Chen <erikchen@chromium.org> Date: Tue Jul 24 00:34:18 2018 Implement skeleton for responsiveness::Watcher. This class will eventually be responsible for forwarding task and event metadata to the Calculator. This class spans the UI and IO threads, so the threading is tricky. This CL sets up the structure of the class, including all the of threading details, and reentrancy handling. Future CLs will hook up tasks and events. Bug: 859155 Change-Id: I7e8706e82dd07ae54ae9b150a6db7a8113ffdc9b Reviewed-on: https://chromium-review.googlesource.com/1144226 Reviewed-by: Alexander Timin <altimin@chromium.org> Reviewed-by: Avi Drissman <avi@chromium.org> Reviewed-by: Timothy Dresser <tdresser@chromium.org> Commit-Queue: Erik Chen <erikchen@chromium.org> Cr-Commit-Position: refs/heads/master@{#577372} [modify] https://crrev.com/6973776c884d8e5e37cb8d3c100e30c01907caf2/content/browser/BUILD.gn [modify] https://crrev.com/6973776c884d8e5e37cb8d3c100e30c01907caf2/content/browser/scheduler/responsiveness/calculator.h [modify] https://crrev.com/6973776c884d8e5e37cb8d3c100e30c01907caf2/content/browser/scheduler/responsiveness/calculator_unittest.cc [add] https://crrev.com/6973776c884d8e5e37cb8d3c100e30c01907caf2/content/browser/scheduler/responsiveness/watcher.cc [add] https://crrev.com/6973776c884d8e5e37cb8d3c100e30c01907caf2/content/browser/scheduler/responsiveness/watcher.h [add] https://crrev.com/6973776c884d8e5e37cb8d3c100e30c01907caf2/content/browser/scheduler/responsiveness/watcher_unittest.cc [modify] https://crrev.com/6973776c884d8e5e37cb8d3c100e30c01907caf2/content/test/BUILD.gn
,
Jul 30
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a9533519851b83d4117567ac911e75653fce86e3 commit a9533519851b83d4117567ac911e75653fce86e3 Author: Erik Chen <erikchen@chromium.org> Date: Mon Jul 30 17:48:22 2018 Implement responsiveness::MessageLoopObserver. This CL adds the class MessageLoopObserver, which forwards events from the UI and IO thread message loop task runners to the responsiveness Watcher and Calculator. Bug: 859155 Change-Id: Ie492f1fbc26940007cb303bdc5cdbdb59d887c19 Reviewed-on: https://chromium-review.googlesource.com/1149042 Commit-Queue: Erik Chen <erikchen@chromium.org> Reviewed-by: Avi Drissman <avi@chromium.org> Reviewed-by: Gabriel Charette <gab@chromium.org> Reviewed-by: Timothy Dresser <tdresser@chromium.org> Cr-Commit-Position: refs/heads/master@{#579083} [modify] https://crrev.com/a9533519851b83d4117567ac911e75653fce86e3/base/message_loop/message_loop.cc [modify] https://crrev.com/a9533519851b83d4117567ac911e75653fce86e3/base/message_loop/message_loop.h [modify] https://crrev.com/a9533519851b83d4117567ac911e75653fce86e3/base/message_loop/message_loop_current.cc [modify] https://crrev.com/a9533519851b83d4117567ac911e75653fce86e3/base/message_loop/message_loop_current.h [modify] https://crrev.com/a9533519851b83d4117567ac911e75653fce86e3/base/message_loop/message_loop_task_runner.cc [modify] https://crrev.com/a9533519851b83d4117567ac911e75653fce86e3/base/message_loop/message_loop_task_runner.h [modify] https://crrev.com/a9533519851b83d4117567ac911e75653fce86e3/base/pending_task.h [modify] https://crrev.com/a9533519851b83d4117567ac911e75653fce86e3/content/browser/BUILD.gn [add] https://crrev.com/a9533519851b83d4117567ac911e75653fce86e3/content/browser/scheduler/responsiveness/message_loop_observer.cc [add] https://crrev.com/a9533519851b83d4117567ac911e75653fce86e3/content/browser/scheduler/responsiveness/message_loop_observer.h [modify] https://crrev.com/a9533519851b83d4117567ac911e75653fce86e3/content/browser/scheduler/responsiveness/watcher.cc [modify] https://crrev.com/a9533519851b83d4117567ac911e75653fce86e3/content/browser/scheduler/responsiveness/watcher.h [modify] https://crrev.com/a9533519851b83d4117567ac911e75653fce86e3/content/browser/scheduler/responsiveness/watcher_unittest.cc
,
Aug 2
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/7d5f0ee834dc7404788b4f521515738f9de482b6 commit 7d5f0ee834dc7404788b4f521515738f9de482b6 Author: erikchen <erikchen@chromium.org> Date: Thu Aug 02 22:15:40 2018 Add interface and Mac implementation for responsiveness::NativeEventObserver. NativeEventObserver forwards will_run and did_run callbacks from the native event processor to responsiveness::Watcher. This CL also adds the macOS implementation. This requires modifying BrowserCrApplication and ShellCrApplication to support a new protocol: NativeEventProcessor, which allows observers to register for event will_run and did_run callbacks. Bug: 859155 Change-Id: Ie2db48efb4a93377ad54e91cbdb376d990b25f11 Reviewed-on: https://chromium-review.googlesource.com/1157235 Commit-Queue: Erik Chen <erikchen@chromium.org> Reviewed-by: Avi Drissman <avi@chromium.org> Reviewed-by: Alexander Timin <altimin@chromium.org> Reviewed-by: Timothy Dresser <tdresser@chromium.org> Cr-Commit-Position: refs/heads/master@{#580345} [modify] https://crrev.com/7d5f0ee834dc7404788b4f521515738f9de482b6/chrome/browser/chrome_browser_application_mac.mm [modify] https://crrev.com/7d5f0ee834dc7404788b4f521515738f9de482b6/content/browser/BUILD.gn [modify] https://crrev.com/7d5f0ee834dc7404788b4f521515738f9de482b6/content/browser/scheduler/responsiveness/calculator.cc [modify] https://crrev.com/7d5f0ee834dc7404788b4f521515738f9de482b6/content/browser/scheduler/responsiveness/calculator.h [modify] https://crrev.com/7d5f0ee834dc7404788b4f521515738f9de482b6/content/browser/scheduler/responsiveness/calculator_unittest.cc [modify] https://crrev.com/7d5f0ee834dc7404788b4f521515738f9de482b6/content/browser/scheduler/responsiveness/message_loop_observer.cc [modify] https://crrev.com/7d5f0ee834dc7404788b4f521515738f9de482b6/content/browser/scheduler/responsiveness/message_loop_observer.h [add] https://crrev.com/7d5f0ee834dc7404788b4f521515738f9de482b6/content/browser/scheduler/responsiveness/native_event_observer.cc [add] https://crrev.com/7d5f0ee834dc7404788b4f521515738f9de482b6/content/browser/scheduler/responsiveness/native_event_observer.h [add] https://crrev.com/7d5f0ee834dc7404788b4f521515738f9de482b6/content/browser/scheduler/responsiveness/native_event_observer_browsertest.mm [add] https://crrev.com/7d5f0ee834dc7404788b4f521515738f9de482b6/content/browser/scheduler/responsiveness/native_event_observer_mac.mm [modify] https://crrev.com/7d5f0ee834dc7404788b4f521515738f9de482b6/content/browser/scheduler/responsiveness/watcher.cc [modify] https://crrev.com/7d5f0ee834dc7404788b4f521515738f9de482b6/content/browser/scheduler/responsiveness/watcher.h [modify] https://crrev.com/7d5f0ee834dc7404788b4f521515738f9de482b6/content/browser/scheduler/responsiveness/watcher_unittest.cc [modify] https://crrev.com/7d5f0ee834dc7404788b4f521515738f9de482b6/content/public/browser/BUILD.gn [add] https://crrev.com/7d5f0ee834dc7404788b4f521515738f9de482b6/content/public/browser/native_event_processor_mac.h [add] https://crrev.com/7d5f0ee834dc7404788b4f521515738f9de482b6/content/public/browser/native_event_processor_observer_mac.h [add] https://crrev.com/7d5f0ee834dc7404788b4f521515738f9de482b6/content/public/browser/native_event_processor_observer_mac.mm [modify] https://crrev.com/7d5f0ee834dc7404788b4f521515738f9de482b6/content/shell/browser/shell_application_mac.mm [modify] https://crrev.com/7d5f0ee834dc7404788b4f521515738f9de482b6/content/test/BUILD.gn
,
Aug 7
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/17a922fba5b6ef0e399b3c26b6b21ac49e125502 commit 17a922fba5b6ef0e399b3c26b6b21ac49e125502 Author: erikchen <erikchen@chromium.org> Date: Tue Aug 07 19:29:43 2018 Implement native event observer for linux. This CL uses ui::PlatformEventSource observer API to hook into pre and post native event dispatch on Linux. Timestamps for X11 events are in X11 server time. The implementation makes a round-trip to the X11 server to obtain a delta, and then uses that delta with base::TimeTicks::Now() to convert to a base::TimeTicks. The overhead from the rount-trip seems reasonable. 99th percentile X11 RTT is 3ms. 99.9th percentile is 16ms. 99.99th percentile is 40ms. These include the time for syscalls to base::TimeTicks::Now(). To further reduce overhead, this CL changes the UMA metric to only be computed with 1/1000 frequency, to avoid two calls to base::TimeTicks::Now(). Note: The implementation of native event observer will be turned on via Finch experiment to measure overhead. Change-Id: I786537f093e862a7a19630c9150606a2eb8267d9 Bug: 859155 Reviewed-on: https://chromium-review.googlesource.com/1162658 Reviewed-by: Timothy Dresser <tdresser@chromium.org> Reviewed-by: Avi Drissman <avi@chromium.org> Reviewed-by: Alexander Timin <altimin@chromium.org> Reviewed-by: Thomas Anderson <thomasanderson@chromium.org> Commit-Queue: Erik Chen <erikchen@chromium.org> Cr-Commit-Position: refs/heads/master@{#581304} [modify] https://crrev.com/17a922fba5b6ef0e399b3c26b6b21ac49e125502/content/browser/BUILD.gn [modify] https://crrev.com/17a922fba5b6ef0e399b3c26b6b21ac49e125502/content/browser/scheduler/responsiveness/native_event_observer.cc [modify] https://crrev.com/17a922fba5b6ef0e399b3c26b6b21ac49e125502/content/browser/scheduler/responsiveness/native_event_observer.h [modify] https://crrev.com/17a922fba5b6ef0e399b3c26b6b21ac49e125502/ui/events/platform/x11/x11_event_source.cc [modify] https://crrev.com/17a922fba5b6ef0e399b3c26b6b21ac49e125502/ui/events/platform/x11/x11_event_source.h
,
Aug 13
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/cfb4c383327a56ac99f8a4757ed6f05a978da4d0 commit cfb4c383327a56ac99f8a4757ed6f05a978da4d0 Author: erikchen <erikchen@chromium.org> Date: Mon Aug 13 20:49:28 2018 Add experiment to turn on responsiveness calculator. The logic is implemented for Windows, macOS and Linux. It may add non-neglible overhead, since it adds calls to base::TimeTicks::Now() to every posted task. It will be turned on behind an experiment so that we can measure the impact. Bug: 859155 Change-Id: I47c2a4629470d892dcb416a3b2121ef37b87ac8c Reviewed-on: https://chromium-review.googlesource.com/1172913 Reviewed-by: Avi Drissman <avi@chromium.org> Commit-Queue: Erik Chen <erikchen@chromium.org> Cr-Commit-Position: refs/heads/master@{#582697} [modify] https://crrev.com/cfb4c383327a56ac99f8a4757ed6f05a978da4d0/content/browser/browser_main_loop.cc [modify] https://crrev.com/cfb4c383327a56ac99f8a4757ed6f05a978da4d0/content/browser/browser_main_loop.h
,
Aug 17
50/50 A/B experiment is running on Canary and Dev channels for desktop platforms. Windows Canary heartbeat: https://uma.googleplex.com/p/chrome/variations/?sid=49580788f9d0c43f7986791aa4f948ea Only statistically significant change is 0.35% regression in 25th percentile of Scheduling.Renderer.DrawInterval2. macOS Canary heartbeat:https://uma.googleplex.com/p/chrome/variations/?sid=4cf332f0a7395e57cd7f2f0db13abaa6 Statistically significant changes: 17% regression in 99th percentile of Memory.Total.PrivateMemoryFootprint. [seems susipicious. noise?] Linux/CrOS Dev hasn't been pushed out yet. Note: I foolishly forgot to hook up the UMA metric itself [all the logic/math is hooked up]. Adding that now. I don't expect that to affect performance, as it's a single UMA metric emission per 30 seconds.
,
Aug 17
wittman@ mentioned that he could use SSM to compare aggregated CPU samples of the two branches of the experiment next week. That seems likely to produce the most accurate estimate for overhead of this code.
,
Aug 19
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/05d67f1110be0e890974e790478be63f00f1255d commit 05d67f1110be0e890974e790478be63f00f1255d Author: erikchen <erikchen@chromium.org> Date: Sun Aug 19 15:32:16 2018 Add UMA metric Browser.Responsiveness.JankyIntervalsPerThirtySeconds All the logic was hooked up for calculation of the metric but the UMA metric itself was missing. Bug: 859155 Change-Id: Iab4e3c726e2627d1f1325ee30bebd1ee9eae6f76 Reviewed-on: https://chromium-review.googlesource.com/1179985 Reviewed-by: Ilya Sherman <isherman@chromium.org> Reviewed-by: Timothy Dresser <tdresser@chromium.org> Commit-Queue: Erik Chen <erikchen@chromium.org> Cr-Commit-Position: refs/heads/master@{#584337} [modify] https://crrev.com/05d67f1110be0e890974e790478be63f00f1255d/content/browser/scheduler/responsiveness/calculator.cc [modify] https://crrev.com/05d67f1110be0e890974e790478be63f00f1255d/tools/metrics/histograms/histograms.xml
,
Aug 20
We have two days' worth of SSM data processed for this experiment now. Results for 70.0.3524.0 on Windows: https://b204354250-dev-wittman-dot-uma-hrd.googleplex.com/p/chrome/callstacks/?sid=ee82292076e17d1eed301b8f86dfcae2 This is a custom dashboard with experiment arms mapped to OS flavor, plus diff coloring based on relative difference to discount noise in the data.
,
Aug 20
I have looked at lot at the sample profiles around MessageLoop recently and have *never* seen TimeTicks::Now() in there. So either it's only a problem on Android or it's only a problem on Win/Mac in the long tail (and sample profiling will not capture it). As such, I'm not convinced #10 tells us much.
,
Aug 20
Based on the coloring, it looks like the following lines of code: https://cs.chromium.org/chromium/src/base/message_loop/message_loop.cc?type=cs&q=message_loop.cc&sq=package:chromium&g=0&l=429 [iterating through observer list] is way more expensive than I would have thought, mostly due to creating WeakReferenceOwners::GetRef(). This should be easy to fix -- we can just avoid using an observer list.
,
Aug 20
Interesting, looks like that's pretty bad in general (5% of UI thread samples) : https://b204354250-dev-wittman-dot-uma-hrd.googleplex.com/p/chrome/callstacks/?sid=6d2f2ff252fe9dad65ce509d6fd7cf29 MemoryPressureListener's ObserverList interestingly being one of the most affected users. Can we improve ObserverList itself?
,
Aug 20
+ tapted -- does the new observerlist implementation have performance degradations or has this always been slow?
,
Aug 21
It's always been slow. The new obseverlist hasn't landed yet. (Also when it lands, existing observers are unaffected by any changes). I wrote a perftest for the "checked" implementation but it was too boring. Like you noticed in #c12, observer list iteration is massively dominated by the `malloc` needed to take out a weak reference to the observer list before starting each iteration over the list. The UAF check adds only ~tens of nanoseconds. I while back, I went down the garden path of how a WeakPtr implementation would go without having to use `malloc` and ended up with https://chromium-review.googlesource.com/c/chromium/src/+/1077950 It _does_ actually work, but it becomes "very" single threaded. WeakPtr is only semi-single-threaded: It's possible to create refs on one thread and pass them to another, so long as that ref is sequence-safe when _accessed_, and the object is not deleted until there are no refs. The "no-malloc" implementation I had couldn't do this (and I don't think we want multiple WeakPtr implementations and a way to cater for ObserverLists that are "strictly" bound to one thread). There might be a sneaky way to avoid the malloc in each ObserverList iteration. Like, if the ObserverList held a WeakPtr "loop" to itself that it did at construction time. Or used scoped_refptr rather than WeakPtr to refer to itself.
,
Aug 21
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/59dc683c4b0b1f01f8ed43a5e7ebba6d64ccd843 commit 59dc683c4b0b1f01f8ed43a5e7ebba6d64ccd843 Author: Erik Chen <erikchen@chromium.org> Date: Tue Aug 21 19:10:39 2018 Stop using observer list for TaskObservers. Iteration through observer list has been observed to add significant overhead to task processing. See https://bugs.chromium.org/p/chromium/issues/detail?id=859155#c12 for more details. Switching to a std::vector should minimize this overhead. Bug: 859155 Change-Id: I8c8851bfbb404afb948072984d399ea22882f125 Reviewed-on: https://chromium-review.googlesource.com/1182298 Reviewed-by: Gabriel Charette <gab@chromium.org> Commit-Queue: Erik Chen <erikchen@chromium.org> Cr-Commit-Position: refs/heads/master@{#584872} [modify] https://crrev.com/59dc683c4b0b1f01f8ed43a5e7ebba6d64ccd843/base/message_loop/message_loop.cc [modify] https://crrev.com/59dc683c4b0b1f01f8ed43a5e7ebba6d64ccd843/base/message_loop/message_loop.h
,
Sep 4
Hi wittman! Could we get another comparison of the two branches of the Finch experiment? I think that c#16 should have addressed the performance issues but want to confirm.
,
Sep 4
Sure, I'll try to run that tomorrow.
,
Sep 7
Apologies, forgot to update this bug after running the analysis. We have one additional day of data on the same dashboard. Here's 71.0.3541.0 canary: https://b204354250-dev-wittman-dot-uma-hrd.googleplex.com/p/chrome/callstacks/?sid=ece35977f162dc535357e05e61f13c7d
,
Sep 12
Awesome. We're seeing about 0.7% difference now, and the bulk of difference comes from emplacing onto a std::stack in Watcher::WillRunTask. Should be an easy fix. """ currently_running_metadata->emplace(task); """ https://cs.chromium.org/chromium/src/content/browser/scheduler/responsiveness/watcher.cc?type=cs&sq=package:chromium&g=0&l=159
,
Sep 12
,
Sep 13
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/7afcf404e94fdb656f79229d1d3593eb08adc5d2 commit 7afcf404e94fdb656f79229d1d3593eb08adc5d2 Author: erikchen <erikchen@chromium.org> Date: Thu Sep 13 14:51:50 2018 Switch from stack -> vector in implementation of responsiveness watcher. UMA sampling profiler shows that std::stack::emplace_back is responsible for the majority of overhead from the responsiveness calculator. Switching to a std::vector should speed this up dramatically. Bug: 859155 Change-Id: I05dd3912d7a0cedca43c190dec206560a0a546a3 Reviewed-on: https://chromium-review.googlesource.com/1222631 Reviewed-by: Timothy Dresser <tdresser@chromium.org> Commit-Queue: Erik Chen <erikchen@chromium.org> Cr-Commit-Position: refs/heads/master@{#591008} [modify] https://crrev.com/7afcf404e94fdb656f79229d1d3593eb08adc5d2/content/browser/scheduler/responsiveness/watcher.cc [modify] https://crrev.com/7afcf404e94fdb656f79229d1d3593eb08adc5d2/content/browser/scheduler/responsiveness/watcher.h
,
Sep 18
wittman: Hopefully last time -- could we get another sampling profiler comparison of the two arms of the experiment?
,
Sep 19
Here's the comparison for 71.0.3553.2 canary for 9/17: https://b204354250-dev-wittman-dot-uma-hrd.googleplex.com/p/chrome/callstacks/?sid=eba16fadc02546424933ac3b54c1e08b
,
Sep 19
Thank you! Total overhead in Watcher::WillRusnTask() [the bulk of overhead of the UMA metric] is now at 0.36%. Most of that overhead is attributed to Watcher::WillRunTask and not base::TimeTicks::Now() or std::vector::emplace_back(). There's not much else going on in the function, so I suspect this is either sampling noise or cost of missed branch predictions. Either way, we're starting to get into the weeds of performance improvements.
,
Sep 19
Sweet! Thanks Erik for landing this with such care, excited to get the metric running on all channels :)!
,
Sep 24
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/bb79f418a4d48b238291e3b920cf2a5e97bf2e3e commit bb79f418a4d48b238291e3b920cf2a5e97bf2e3e Author: erikchen <erikchen@chromium.org> Date: Mon Sep 24 17:08:02 2018 Add support for native event processor observer to headless mode. The hooks are currently required by the jank calculator. They will eventually be the mechanism that the browser task scheduler hooks into native events. Bug: 859155 Change-Id: I4b633c7b350c673c7b59cdcfe3a115ae74e5bacb Reviewed-on: https://chromium-review.googlesource.com/1238777 Reviewed-by: Sami Kyöstilä <skyostil@chromium.org> Commit-Queue: Erik Chen <erikchen@chromium.org> Cr-Commit-Position: refs/heads/master@{#593575} [modify] https://crrev.com/bb79f418a4d48b238291e3b920cf2a5e97bf2e3e/headless/lib/browser/headless_shell_application_mac.mm
,
Sep 25
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/943b1bcef6834a45d04a06eed4be8a09e467d82b commit 943b1bcef6834a45d04a06eed4be8a09e467d82b Author: erikchen <erikchen@chromium.org> Date: Tue Sep 25 20:21:47 2018 Fix data race for SetAddQueueTimeToTasks(). Switch to an atomic variable to ensure that getter and setter do not have a data race. Bug: 859155 Change-Id: Ib5ab1968cc8e25ab363fb4ed0b3f2e756d9cc984 Reviewed-on: https://chromium-review.googlesource.com/1240634 Commit-Queue: Erik Chen <erikchen@chromium.org> Reviewed-by: Gabriel Charette <gab@chromium.org> Cr-Commit-Position: refs/heads/master@{#594079} [modify] https://crrev.com/943b1bcef6834a45d04a06eed4be8a09e467d82b/base/message_loop/message_loop_task_runner.cc [modify] https://crrev.com/943b1bcef6834a45d04a06eed4be8a09e467d82b/base/message_loop/message_loop_task_runner.h
,
Sep 27
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/3225e1d5ea4626a770d835dd03a25b6c095b58b5 commit 3225e1d5ea4626a770d835dd03a25b6c095b58b5 Author: erikchen <erikchen@chromium.org> Date: Thu Sep 27 20:38:38 2018 Use accurate X11 event timestamp computation. X events have a timestamp which is only well defined relative to the X11 Server time. The previous computation for timestamp for X11 events was making the assumption that Server time and Chrome time were the same. This assumption is not always true -- this is likely the root cause of "bad" timestamps observed in https://bugs.chromium.org/p/chromium/issues/detail?id=650338#c1 This CL changes event timestamp computation to make a roundtrip to the X11 Server to get an accurate base::TimeTicks. This logic was lifted out of the responsiveness calculator, which was already doing this computation. The latter will subsequently be changed to use the computation in this CL. Change-Id: I963019cd8bfb8ce14e06b3743a159c9c85f2cb82 Bug: 859155 Reviewed-on: https://chromium-review.googlesource.com/1249383 Reviewed-by: Thomas Anderson <thomasanderson@chromium.org> Reviewed-by: Avi Drissman <avi@chromium.org> Commit-Queue: Erik Chen <erikchen@chromium.org> Cr-Commit-Position: refs/heads/master@{#594844} [modify] https://crrev.com/3225e1d5ea4626a770d835dd03a25b6c095b58b5/ui/events/event_unittest.cc [modify] https://crrev.com/3225e1d5ea4626a770d835dd03a25b6c095b58b5/ui/events/platform/x11/x11_event_source.cc [modify] https://crrev.com/3225e1d5ea4626a770d835dd03a25b6c095b58b5/ui/events/platform/x11/x11_event_source.h [modify] https://crrev.com/3225e1d5ea4626a770d835dd03a25b6c095b58b5/ui/events/x/events_x.cc [modify] https://crrev.com/3225e1d5ea4626a770d835dd03a25b6c095b58b5/ui/events/x/events_x_unittest.cc [modify] https://crrev.com/3225e1d5ea4626a770d835dd03a25b6c095b58b5/ui/events/x/events_x_utils.cc [modify] https://crrev.com/3225e1d5ea4626a770d835dd03a25b6c095b58b5/ui/events/x/events_x_utils.h
,
Sep 28
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/dad88f97d6b272bc9ba751e89bcd50dac3a6abfc commit dad88f97d6b272bc9ba751e89bcd50dac3a6abfc Author: Findit <findit-for-me@appspot.gserviceaccount.com> Date: Fri Sep 28 02:07:52 2018 Revert "Use accurate X11 event timestamp computation." This reverts commit 3225e1d5ea4626a770d835dd03a25b6c095b58b5. Reason for revert: Findit (https://goo.gl/kROfz5) identified CL at revision 594844 as the culprit for flakes in the build cycles as shown on: https://findit-for-me.appspot.com/waterfall/flake/flake-culprit?key=ag9zfmZpbmRpdC1mb3ItbWVyQwsSDEZsYWtlQ3VscHJpdCIxY2hyb21pdW0vMzIyNWUxZDVlYTQ2MjZhNzcwZDgzNWRkMDNhMjViNmMwOTViNThiNQw Sample Failed Build: https://ci.chromium.org/buildbot/chromium.linux/Linux%20Tests/72634 Sample Failed Step: events_unittests Sample Flaky Test: X11EventTest.AutoRepeat Original change's description: > Use accurate X11 event timestamp computation. > > X events have a timestamp which is only well defined relative to the X11 Server > time. The previous computation for timestamp for X11 events was making the > assumption that Server time and Chrome time were the same. This assumption is > not always true -- this is likely the root cause of "bad" timestamps observed in > https://bugs.chromium.org/p/chromium/issues/detail?id=650338#c1 > > This CL changes event timestamp computation to make a roundtrip to the X11 > Server to get an accurate base::TimeTicks. This logic was lifted out of the > responsiveness calculator, which was already doing this computation. The latter > will subsequently be changed to use the computation in this CL. > > Change-Id: I963019cd8bfb8ce14e06b3743a159c9c85f2cb82 > Bug: 859155 > Reviewed-on: https://chromium-review.googlesource.com/1249383 > Reviewed-by: Thomas Anderson <thomasanderson@chromium.org> > Reviewed-by: Avi Drissman <avi@chromium.org> > Commit-Queue: Erik Chen <erikchen@chromium.org> > Cr-Commit-Position: refs/heads/master@{#594844} Change-Id: I911f8bd268739b5e91c550e3f6a6186ae4dfbecb No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: 859155 , 890121 Reviewed-on: https://chromium-review.googlesource.com/1250210 Cr-Commit-Position: refs/heads/master@{#594958} [modify] https://crrev.com/dad88f97d6b272bc9ba751e89bcd50dac3a6abfc/ui/events/event_unittest.cc [modify] https://crrev.com/dad88f97d6b272bc9ba751e89bcd50dac3a6abfc/ui/events/platform/x11/x11_event_source.cc [modify] https://crrev.com/dad88f97d6b272bc9ba751e89bcd50dac3a6abfc/ui/events/platform/x11/x11_event_source.h [modify] https://crrev.com/dad88f97d6b272bc9ba751e89bcd50dac3a6abfc/ui/events/x/events_x.cc [modify] https://crrev.com/dad88f97d6b272bc9ba751e89bcd50dac3a6abfc/ui/events/x/events_x_unittest.cc [modify] https://crrev.com/dad88f97d6b272bc9ba751e89bcd50dac3a6abfc/ui/events/x/events_x_utils.cc [modify] https://crrev.com/dad88f97d6b272bc9ba751e89bcd50dac3a6abfc/ui/events/x/events_x_utils.h
,
Oct 1
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/2c72cd9089bc3278bf3d2852c971325a9c07f097 commit 2c72cd9089bc3278bf3d2852c971325a9c07f097 Author: erikchen <erikchen@chromium.org> Date: Mon Oct 01 15:29:12 2018 [Reland 1] Use accurate X11 event timestamp computation."" The original CL didn't allow negative time skew from the time server. While this is likely correct, this caused unit tests to fail as they relied on negative time skew. This CL allows negative time skew, and also removes another source of non-determinism from the tests. Original change's description: > Use accurate X11 event timestamp computation. > > X events have a timestamp which is only well defined relative to the X11 Server > time. The previous computation for timestamp for X11 events was making the > assumption that Server time and Chrome time were the same. This assumption is > not always true -- this is likely the root cause of "bad" timestamps observed in > https://bugs.chromium.org/p/chromium/issues/detail?id=650338#c1 > > This CL changes event timestamp computation to make a roundtrip to the X11 > Server to get an accurate base::TimeTicks. This logic was lifted out of the > responsiveness calculator, which was already doing this computation. The latter > will subsequently be changed to use the computation in this CL. > > Change-Id: I963019cd8bfb8ce14e06b3743a159c9c85f2cb82 > Bug: 859155 > Reviewed-on: https://chromium-review.googlesource.com/1249383 > Reviewed-by: Thomas Anderson <thomasanderson@chromium.org> > Reviewed-by: Avi Drissman <avi@chromium.org> > Commit-Queue: Erik Chen <erikchen@chromium.org> > Cr-Commit-Position: refs/heads/master@{#594844} Change-Id: I6595f6aa163a18c9724c50546ac8c84bc5c84a03 Bug: 859155 Reviewed-on: https://chromium-review.googlesource.com/1252685 Reviewed-by: Thomas Anderson <thomasanderson@chromium.org> Reviewed-by: Avi Drissman <avi@chromium.org> Commit-Queue: Erik Chen <erikchen@chromium.org> Cr-Commit-Position: refs/heads/master@{#595454} [modify] https://crrev.com/2c72cd9089bc3278bf3d2852c971325a9c07f097/ui/events/event_unittest.cc [modify] https://crrev.com/2c72cd9089bc3278bf3d2852c971325a9c07f097/ui/events/platform/x11/x11_event_source.cc [modify] https://crrev.com/2c72cd9089bc3278bf3d2852c971325a9c07f097/ui/events/platform/x11/x11_event_source.h [modify] https://crrev.com/2c72cd9089bc3278bf3d2852c971325a9c07f097/ui/events/x/events_x.cc [modify] https://crrev.com/2c72cd9089bc3278bf3d2852c971325a9c07f097/ui/events/x/events_x_unittest.cc [modify] https://crrev.com/2c72cd9089bc3278bf3d2852c971325a9c07f097/ui/events/x/events_x_utils.cc [modify] https://crrev.com/2c72cd9089bc3278bf3d2852c971325a9c07f097/ui/events/x/events_x_utils.h
,
Oct 12
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/3f51ab68a022dd6091a2a570424357e848752d00 commit 3f51ab68a022dd6091a2a570424357e848752d00 Author: erikchen <erikchen@chromium.org> Date: Fri Oct 12 18:49:49 2018 Enable jank calculator by default. It was previously on an A/B experiment to assess the performance overhead. This overhead is <1%, so I'm turning the feature on by default. Bug: 859155 Change-Id: I555637a294cf8c5d7ce97f57b8aebe7f2fb73734 Reviewed-on: https://chromium-review.googlesource.com/c/1277574 Commit-Queue: Erik Chen <erikchen@chromium.org> Reviewed-by: Timothy Dresser <tdresser@chromium.org> Reviewed-by: Avi Drissman <avi@chromium.org> Cr-Commit-Position: refs/heads/master@{#599310} [modify] https://crrev.com/3f51ab68a022dd6091a2a570424357e848752d00/content/browser/browser_main_loop.cc
,
Oct 15
The metric is now implemented and enabled everywhere on Desktop. The metric is not implemented on Android. From a discussion I had with skyostil@ On Android, we can divide events into three categories: (1) framework events (2) Chrome-related java events (3) Chrome-related tasks. skyostil's team is working on sending type (2) events through the task scheduler. Once this happens, both (2) and (3) type events will be captured by the responsiveness calculator. We don't know how often type (1) events occur, nor how long they take. The implementation of the responsiveness calculator doesn't need to capture all events to calculate jank, it just needs to capture most events [time intervals are marked as janky, not events, so we just need sufficient events to know whether a given time interval is janky]. Once this is implemented, we can turn on the jank calculator for Android.
,
Oct 15
Blocking on bug to unify C++ and Java tasks as a part of the UI thread scheduler effort.
,
Oct 17
> commit 2c72cd9089bc3278bf3d2852c971325a9c07f097 > Author: erikchen <erikchen@chromium.org> > Date: Mon Oct 01 15:29:12 2018 > [Reland 1] Use accurate X11 event timestamp computation."" It seems the previous commit caused some issues. If you have an input element, like a textarea which is focused, the shortcuts Ctrl+L, Ctrl+W, Ctrl+T, etc. doesn't work. You need to first unfocus the element to make them work. You can reproduce it with the attached example. I did a bisect and this was the result: You are probably looking for a change made after 595453 (known good), but no later than 595454 (first known bad). CHANGELOG URL: https://chromium.googlesource.com/chromium/src/+log/30e11c9e9b9f5ab322a5c5fdb107e62b44d6215b..2c72cd9089bc3278bf3d2852c971325a9c07f097
,
Oct 17
I cannot reproduce the issue described in c#35.
,
Oct 17
I'm on linux BTW, just in case it's a Linux only issue.
,
Oct 17
I tested on Linux.
,
Oct 22
+ fsamuel We have evidence from slow reports that the timestamp on native events is not reliable on Windows, macOS and Linux. The symptoms are similar in all cases -- JankyIntervalsPerThirtySeconds reports a large value for jank even though the UI/IO threads are clearly processing events with no issues. In all cases, there are many unfinished InputLatency::MouseMove events. [wall duration >> 100 ms]. InputLatency::MouseMove is a synthetic trace event that is calculated from several factors, including INPUT_EVENT_LATENCY_ORIGINAL_COMPONENT. This appears to do some rough sanity checks and then spits out the event's creation time. https://cs.chromium.org/chromium/src/content/browser/renderer_host/input/render_widget_host_latency_tracker.cc?type=cs&sq=package:chromium&g=0&l=140 I was also able to repro on macOS locally without doing anything special -- just building and running Chrome. I saw that sometimes, (base::Time::Now() - event.creation_time).InMilliseconds() would return 3.7 * 10^9 [I don't know what's special about that number. It doesn't appear to be related to unix or windows epoch. It's ~42 days]. It's possible that there's something in Chrome that attempts to synthesize/dispatch native events but is using the wrong timestamp. Regardless, I think the right solution is to measure execution time of native events instead of queue_time + execution time. This is a simple change, prevents this entire category of problems, and only loses out on queuing time of native events. I generally expect that to be small.
,
Oct 23
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/33b360718ad98e4343e7f5711aed5d62a97b33a4 commit 33b360718ad98e4343e7f5711aed5d62a97b33a4 Author: Erik Chen <erikchen@chromium.org> Date: Tue Oct 23 15:18:04 2018 Remove a source of noise from JankyIntervalsPerThirtySeconds. This CL changes the JankyIntervals UMA metric to use execution start time rather than queue time for native events. The only way to obtain the queue time is to look at the timestamp field on the native event [which is not set by Chrome]. We're seeing that this value is likely noisy on Windows, macOS and Linux. A future CL will remove the now-redundant |creation_time| parameter on DidRunEventOnUIThread. Bug: 859155 Change-Id: I4d24e7bfd923301fa03e1e83fc85bb62c76ce791 TBR: tdresser@chromium.org Reviewed-on: https://chromium-review.googlesource.com/c/1294307 Commit-Queue: Erik Chen <erikchen@chromium.org> Reviewed-by: Erik Chen <erikchen@chromium.org> Cr-Commit-Position: refs/heads/master@{#601955} [modify] https://crrev.com/33b360718ad98e4343e7f5711aed5d62a97b33a4/content/browser/scheduler/responsiveness/watcher.cc [modify] https://crrev.com/33b360718ad98e4343e7f5711aed5d62a97b33a4/content/browser/scheduler/responsiveness/watcher.h [modify] https://crrev.com/33b360718ad98e4343e7f5711aed5d62a97b33a4/content/browser/scheduler/responsiveness/watcher_unittest.cc
,
Oct 25
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/2b4696db161a85f9a9716ae045a76bed5f0dcef5 commit 2b4696db161a85f9a9716ae045a76bed5f0dcef5 Author: Erik Chen <erikchen@chromium.org> Date: Thu Oct 25 22:06:05 2018 Refactor Watcher::DidRunEventOnUIThread to not take a creation time. The creation time of native events is no longer used because there is known noise on Windows, macOS and Linux. This CL is a refactor with no intended behavior change. Change-Id: I9eb860fb27b6642be25dfbbaa9d5615d0b619073 Bug: 859155 Reviewed-on: https://chromium-review.googlesource.com/c/1296619 Reviewed-by: Timothy Dresser <tdresser@chromium.org> Reviewed-by: Avi Drissman <avi@chromium.org> Commit-Queue: Avi Drissman <avi@chromium.org> Cr-Commit-Position: refs/heads/master@{#602891} [modify] https://crrev.com/2b4696db161a85f9a9716ae045a76bed5f0dcef5/content/browser/scheduler/responsiveness/native_event_observer.cc [modify] https://crrev.com/2b4696db161a85f9a9716ae045a76bed5f0dcef5/content/browser/scheduler/responsiveness/native_event_observer.h [modify] https://crrev.com/2b4696db161a85f9a9716ae045a76bed5f0dcef5/content/browser/scheduler/responsiveness/native_event_observer_browsertest.mm [modify] https://crrev.com/2b4696db161a85f9a9716ae045a76bed5f0dcef5/content/browser/scheduler/responsiveness/native_event_observer_browsertest_win.cc [modify] https://crrev.com/2b4696db161a85f9a9716ae045a76bed5f0dcef5/content/browser/scheduler/responsiveness/native_event_observer_mac.mm [modify] https://crrev.com/2b4696db161a85f9a9716ae045a76bed5f0dcef5/content/browser/scheduler/responsiveness/watcher.cc [modify] https://crrev.com/2b4696db161a85f9a9716ae045a76bed5f0dcef5/content/browser/scheduler/responsiveness/watcher.h [modify] https://crrev.com/2b4696db161a85f9a9716ae045a76bed5f0dcef5/content/browser/scheduler/responsiveness/watcher_unittest.cc [modify] https://crrev.com/2b4696db161a85f9a9716ae045a76bed5f0dcef5/content/public/browser/native_event_processor_observer_mac.h [modify] https://crrev.com/2b4696db161a85f9a9716ae045a76bed5f0dcef5/content/public/browser/native_event_processor_observer_mac.mm
,
Oct 25
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/3280705f888d406c966e1164cdbb84cf43ac9cb2 commit 3280705f888d406c966e1164cdbb84cf43ac9cb2 Author: erikchen <erikchen@chromium.org> Date: Thu Oct 25 22:44:43 2018 Add startup/not-startup versions of jank metric. This allows Chromium developers to more easily distinguish between improvements that reduce start-up jank, and improvements that reduce non-startup jank. This CL removes the expiry from the Jank metrics as they are providing high utility for slow reports, and are being added as a heartbeat metric. Bug: 859155 Change-Id: Ida8bf44219717ba19c2eb13fefdec09d42f2569f Reviewed-on: https://chromium-review.googlesource.com/c/1299295 Commit-Queue: Erik Chen <erikchen@chromium.org> Reviewed-by: Alexei Svitkine <asvitkine@chromium.org> Cr-Commit-Position: refs/heads/master@{#602907} [modify] https://crrev.com/3280705f888d406c966e1164cdbb84cf43ac9cb2/content/browser/scheduler/responsiveness/calculator.cc [modify] https://crrev.com/3280705f888d406c966e1164cdbb84cf43ac9cb2/content/browser/scheduler/responsiveness/calculator.h [modify] https://crrev.com/3280705f888d406c966e1164cdbb84cf43ac9cb2/tools/metrics/histograms/histograms.xml
,
Nov 1
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/5939ee60ce59d0263a6fa2f37f7d410aa63a45a8 commit 5939ee60ce59d0263a6fa2f37f7d410aa63a45a8 Author: Erik Chen <erikchen@chromium.org> Date: Thu Nov 01 20:01:50 2018 Enable measurement of JankyIntervalsPerThirtySeconds on Android. The metric will only measure tasks posted on the UI and IO tasks, not native events. Based on Slow Reports on Desktop, this seems likely to be sufficient to detect jank in the wild on Android as well. Bug: 859155 Change-Id: Ia573e6311d9315597334108955f215be61fc3b07 Reviewed-on: https://chromium-review.googlesource.com/c/1312681 Reviewed-by: Alexei Svitkine <asvitkine@chromium.org> Reviewed-by: Etienne Pierre-Doray <etiennep@chromium.org> Reviewed-by: Timothy Dresser <tdresser@chromium.org> Reviewed-by: Avi Drissman <avi@chromium.org> Commit-Queue: Erik Chen <erikchen@chromium.org> Cr-Commit-Position: refs/heads/master@{#604681} [modify] https://crrev.com/5939ee60ce59d0263a6fa2f37f7d410aa63a45a8/content/browser/browser_main_loop.cc [modify] https://crrev.com/5939ee60ce59d0263a6fa2f37f7d410aa63a45a8/tools/metrics/histograms/histograms.xml
,
Nov 6
@erikchen:
The new NativeEventProcessor protocol [1] is problematic for Chromium Embedded Framework (CEF) because CEF/Chromium is built as a dylib and our users provide their own NSApplication implementation in a separate client application executable. In the past, the only protocol that Chromium required was CrAppProtocol/CrAppControlProtocol [2]. This was OK for CEF because CrAppControlProtocol uses only basic data types and the naming is Chromium-specific (with the Cr prefix). The CEF header can therefore expose the following declarations:
// Copy of definition from base/message_loop/message_pump_mac.h.
@protocol CrAppProtocol
// Must return true if -[NSApplication sendEvent:] is currently on the stack.
- (BOOL)isHandlingSendEvent;
@end
// Copy of definition from base/mac/scoped_sending_event.h.
@protocol CrAppControlProtocol<CrAppProtocol>
- (void)setHandlingSendEvent:(BOOL)handlingSendEvent;
@end
The NativeEventProcessor protocol and associated content::NativeEventProcessorObserver interface don't match this naming pattern (missing the Cr prefix and adding the content namespace). Other than that, I think redeclaring the NativeEventProcessorObserver class in the CEF header will be OK because it's a pure virtual interface.
Would you be open to adding the Cr prefix and removing the content namespace for consistency with CrAppProtocol? The declarations would then look like this:
class CrNativeEventProcessorObserver {
public:
// Called right before a native event is run.
virtual void WillRunNativeEvent(const void* opaque_identifier) = 0;
// Called right after a native event is run.
virtual void DidRunNativeEvent(const void* opaque_identifier) = 0;
};
@protocol CrNativeEventProcessor
- (void)addNativeEventProcessorObserver:
(CrNativeEventProcessorObserver*)observer;
- (void)removeNativeEventProcessorObserver:
(CrNativeEventProcessorObserver*)observer;
@end
Alternately, if usage of NativeEventProcessor is intended to be optional for Chromium-based applications, could we instead make the conformsToProtocol checks in [3] optional?
Thanks,
Marshall
[1] https://cs.chromium.org/chromium/src/content/public/browser/native_event_processor_mac.h
[2] https://cs.chromium.org/chromium/src/base/mac/scoped_sending_event.h
[3] https://cs.chromium.org/chromium/src/content/browser/scheduler/responsiveness/native_event_observer_mac.mm
,
Nov 6
Thanks for reaching out. I think that the best solution would be to avoid instantiating the responsiveness::Watcher. https://cs.chromium.org/chromium/src/content/browser/browser_main_loop.cc?l=1437 What is the typical mechanism by which Chromium exposes run-time/build-time flags to CEF?
,
Nov 6
@erikchen: In that case perhaps we can put responsiveness::Watcher usage behind a BUILDFLAG [1] which can be configured at build time using a GN flag? [1] https://groups.google.com/a/chromium.org/d/msg/chromium-dev/5c4ySpPsV14/JJfAeDdyAgAJ
,
Dec 18
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/faa579def265753007f87c3ea85a907f0ed469b0 commit faa579def265753007f87c3ea85a907f0ed469b0 Author: Thomas Anderson <thomasanderson@chromium.org> Date: Tue Dec 18 19:44:53 2018 Revert "[Reland 1] Use accurate X11 event timestamp computation.""" This reverts commit 2c72cd9089bc3278bf3d2852c971325a9c07f097. Reason for revert: bugs 915277 and 899881 Original change's description: > [Reland 1] Use accurate X11 event timestamp computation."" > > The original CL didn't allow negative time skew from the time server. While this > is likely correct, this caused unit tests to fail as they relied on negative time > skew. This CL allows negative time skew, and also removes another source of > non-determinism from the tests. > > Original change's description: > > Use accurate X11 event timestamp computation. > > > > X events have a timestamp which is only well defined relative to the X11 Server > > time. The previous computation for timestamp for X11 events was making the > > assumption that Server time and Chrome time were the same. This assumption is > > not always true -- this is likely the root cause of "bad" timestamps observed in > > https://bugs.chromium.org/p/chromium/issues/detail?id=650338#c1 > > > > This CL changes event timestamp computation to make a roundtrip to the X11 > > Server to get an accurate base::TimeTicks. This logic was lifted out of the > > responsiveness calculator, which was already doing this computation. The latter > > will subsequently be changed to use the computation in this CL. > > > > Change-Id: I963019cd8bfb8ce14e06b3743a159c9c85f2cb82 > > Bug: 859155 > > Reviewed-on: https://chromium-review.googlesource.com/1249383 > > Reviewed-by: Thomas Anderson <thomasanderson@chromium.org> > > Reviewed-by: Avi Drissman <avi@chromium.org> > > Commit-Queue: Erik Chen <erikchen@chromium.org> > > Cr-Commit-Position: refs/heads/master@{#594844} > > Change-Id: I6595f6aa163a18c9724c50546ac8c84bc5c84a03 > Bug: 859155 > Reviewed-on: https://chromium-review.googlesource.com/1252685 > Reviewed-by: Thomas Anderson <thomasanderson@chromium.org> > Reviewed-by: Avi Drissman <avi@chromium.org> > Commit-Queue: Erik Chen <erikchen@chromium.org> > Cr-Commit-Position: refs/heads/master@{#595454} TBR=avi@chromium.org,erikchen@chromium.org,thomasanderson@chromium.org # Not skipping CQ checks because original CL landed > 1 day ago. Bug: 859155 , 899881 , 915277 Change-Id: Ib85a5f5d7bf9469076a334d77ef92771199d6a2d Reviewed-on: https://chromium-review.googlesource.com/c/1382868 Reviewed-by: Thomas Anderson <thomasanderson@chromium.org> Commit-Queue: Thomas Anderson <thomasanderson@chromium.org> Cr-Commit-Position: refs/heads/master@{#617593} [modify] https://crrev.com/faa579def265753007f87c3ea85a907f0ed469b0/ui/events/event_unittest.cc [modify] https://crrev.com/faa579def265753007f87c3ea85a907f0ed469b0/ui/events/platform/x11/x11_event_source.cc [modify] https://crrev.com/faa579def265753007f87c3ea85a907f0ed469b0/ui/events/platform/x11/x11_event_source.h [modify] https://crrev.com/faa579def265753007f87c3ea85a907f0ed469b0/ui/events/x/events_x.cc [modify] https://crrev.com/faa579def265753007f87c3ea85a907f0ed469b0/ui/events/x/events_x_unittest.cc [modify] https://crrev.com/faa579def265753007f87c3ea85a907f0ed469b0/ui/events/x/events_x_utils.cc [modify] https://crrev.com/faa579def265753007f87c3ea85a907f0ed469b0/ui/events/x/events_x_utils.h
,
Dec 20
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/6ad9f6333b2a7d81bbbf926529e560269e05f439 commit 6ad9f6333b2a7d81bbbf926529e560269e05f439 Author: Tom Anderson <thomasanderson@chromium.org> Date: Thu Dec 20 19:15:13 2018 [Merge to M72] Revert "[Reland 1] Use accurate X11 event timestamp computation.""" > This reverts commit 2c72cd9089bc3278bf3d2852c971325a9c07f097. > > Reason for revert: bugs 915277 and 899881 > > Original change's description: > > [Reland 1] Use accurate X11 event timestamp computation."" > > > > The original CL didn't allow negative time skew from the time server. While this > > is likely correct, this caused unit tests to fail as they relied on negative time > > skew. This CL allows negative time skew, and also removes another source of > > non-determinism from the tests. > > > > Original change's description: > > > Use accurate X11 event timestamp computation. > > > > > > X events have a timestamp which is only well defined relative to the X11 Server > > > time. The previous computation for timestamp for X11 events was making the > > > assumption that Server time and Chrome time were the same. This assumption is > > > not always true -- this is likely the root cause of "bad" timestamps observed in > > > https://bugs.chromium.org/p/chromium/issues/detail?id=650338#c1 > > > > > > This CL changes event timestamp computation to make a roundtrip to the X11 > > > Server to get an accurate base::TimeTicks. This logic was lifted out of the > > > responsiveness calculator, which was already doing this computation. The latter > > > will subsequently be changed to use the computation in this CL. > > > > > > Change-Id: I963019cd8bfb8ce14e06b3743a159c9c85f2cb82 > > > Bug: 859155 > > > Reviewed-on: https://chromium-review.googlesource.com/1249383 > > > Reviewed-by: Thomas Anderson <thomasanderson@chromium.org> > > > Reviewed-by: Avi Drissman <avi@chromium.org> > > > Commit-Queue: Erik Chen <erikchen@chromium.org> > > > Cr-Commit-Position: refs/heads/master@{#594844} > > > > Change-Id: I6595f6aa163a18c9724c50546ac8c84bc5c84a03 > > Bug: 859155 > > Reviewed-on: https://chromium-review.googlesource.com/1252685 > > Reviewed-by: Thomas Anderson <thomasanderson@chromium.org> > > Reviewed-by: Avi Drissman <avi@chromium.org> > > Commit-Queue: Erik Chen <erikchen@chromium.org> > > Cr-Commit-Position: refs/heads/master@{#595454} > > TBR=avi@chromium.org,erikchen@chromium.org,thomasanderson@chromium.org > > # Not skipping CQ checks because original CL landed > 1 day ago. > > Bug: 859155 , 899881 , 915277 > Change-Id: Ib85a5f5d7bf9469076a334d77ef92771199d6a2d > Reviewed-on: https://chromium-review.googlesource.com/c/1382868 > Reviewed-by: Thomas Anderson <thomasanderson@chromium.org> > Commit-Queue: Thomas Anderson <thomasanderson@chromium.org> > Cr-Commit-Position: refs/heads/master@{#617593} TBR=avi@chromium.org,erikchen@chromium.org,thomasanderson@chromium.org NOTRY=true NOPRESUBMIT=true NOTREECHECKS=true Bug: 859155 , 899881 , 915277 Change-Id: I325a7b1587a8dace272d2b2ac8a2dc0ea0082218 Reviewed-on: https://chromium-review.googlesource.com/c/1387545 Reviewed-by: Thomas Anderson <thomasanderson@chromium.org> Cr-Commit-Position: refs/branch-heads/3626@{#487} Cr-Branched-From: d897fb137fbaaa9355c0c93124cc048824eb1e65-refs/heads/master@{#612437} [modify] https://crrev.com/6ad9f6333b2a7d81bbbf926529e560269e05f439/ui/events/event_unittest.cc [modify] https://crrev.com/6ad9f6333b2a7d81bbbf926529e560269e05f439/ui/events/platform/x11/x11_event_source.cc [modify] https://crrev.com/6ad9f6333b2a7d81bbbf926529e560269e05f439/ui/events/platform/x11/x11_event_source.h [modify] https://crrev.com/6ad9f6333b2a7d81bbbf926529e560269e05f439/ui/events/x/events_x.cc [modify] https://crrev.com/6ad9f6333b2a7d81bbbf926529e560269e05f439/ui/events/x/events_x_unittest.cc [modify] https://crrev.com/6ad9f6333b2a7d81bbbf926529e560269e05f439/ui/events/x/events_x_utils.cc [modify] https://crrev.com/6ad9f6333b2a7d81bbbf926529e560269e05f439/ui/events/x/events_x_utils.h
,
Dec 20
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/6ad9f6333b2a7d81bbbf926529e560269e05f439 Commit: 6ad9f6333b2a7d81bbbf926529e560269e05f439 Author: thomasanderson@chromium.org Commiter: thomasanderson@chromium.org Date: 2018-12-20 19:15:13 +0000 UTC [Merge to M72] Revert "[Reland 1] Use accurate X11 event timestamp computation.""" > This reverts commit 2c72cd9089bc3278bf3d2852c971325a9c07f097. > > Reason for revert: bugs 915277 and 899881 > > Original change's description: > > [Reland 1] Use accurate X11 event timestamp computation."" > > > > The original CL didn't allow negative time skew from the time server. While this > > is likely correct, this caused unit tests to fail as they relied on negative time > > skew. This CL allows negative time skew, and also removes another source of > > non-determinism from the tests. > > > > Original change's description: > > > Use accurate X11 event timestamp computation. > > > > > > X events have a timestamp which is only well defined relative to the X11 Server > > > time. The previous computation for timestamp for X11 events was making the > > > assumption that Server time and Chrome time were the same. This assumption is > > > not always true -- this is likely the root cause of "bad" timestamps observed in > > > https://bugs.chromium.org/p/chromium/issues/detail?id=650338#c1 > > > > > > This CL changes event timestamp computation to make a roundtrip to the X11 > > > Server to get an accurate base::TimeTicks. This logic was lifted out of the > > > responsiveness calculator, which was already doing this computation. The latter > > > will subsequently be changed to use the computation in this CL. > > > > > > Change-Id: I963019cd8bfb8ce14e06b3743a159c9c85f2cb82 > > > Bug: 859155 > > > Reviewed-on: https://chromium-review.googlesource.com/1249383 > > > Reviewed-by: Thomas Anderson <thomasanderson@chromium.org> > > > Reviewed-by: Avi Drissman <avi@chromium.org> > > > Commit-Queue: Erik Chen <erikchen@chromium.org> > > > Cr-Commit-Position: refs/heads/master@{#594844} > > > > Change-Id: I6595f6aa163a18c9724c50546ac8c84bc5c84a03 > > Bug: 859155 > > Reviewed-on: https://chromium-review.googlesource.com/1252685 > > Reviewed-by: Thomas Anderson <thomasanderson@chromium.org> > > Reviewed-by: Avi Drissman <avi@chromium.org> > > Commit-Queue: Erik Chen <erikchen@chromium.org> > > Cr-Commit-Position: refs/heads/master@{#595454} > > TBR=avi@chromium.org,erikchen@chromium.org,thomasanderson@chromium.org > > # Not skipping CQ checks because original CL landed > 1 day ago. > > Bug: 859155 , 899881 , 915277 > Change-Id: Ib85a5f5d7bf9469076a334d77ef92771199d6a2d > Reviewed-on: https://chromium-review.googlesource.com/c/1382868 > Reviewed-by: Thomas Anderson <thomasanderson@chromium.org> > Commit-Queue: Thomas Anderson <thomasanderson@chromium.org> > Cr-Commit-Position: refs/heads/master@{#617593} TBR=avi@chromium.org,erikchen@chromium.org,thomasanderson@chromium.org NOTRY=true NOPRESUBMIT=true NOTREECHECKS=true Bug: 859155 , 899881 , 915277 Change-Id: I325a7b1587a8dace272d2b2ac8a2dc0ea0082218 Reviewed-on: https://chromium-review.googlesource.com/c/1387545 Reviewed-by: Thomas Anderson <thomasanderson@chromium.org> Cr-Commit-Position: refs/branch-heads/3626@{#487} Cr-Branched-From: d897fb137fbaaa9355c0c93124cc048824eb1e65-refs/heads/master@{#612437}
,
Jan 15
|
||||||||||
►
Sign in to add a comment |
||||||||||
Comment 1 by bugdroid1@chromium.org
, Jul 20