Remove renderer timestamps from FirstMeaningfulPaintDetector and PaintTiming |
|||||
Issue descriptionIn crbug.com/738235 , we are switching to using more accurate until-GPU-swap timestamps. However, the internal timestamps are being kept around for three reasons: 1) We're collecting the deltas between the renderer and swap timestamps. 2) Not wanting to break the internals of FirstMeaningfulPaintDetector 3) The FirstMeaningfulPaintDetector unit tests rely on the renderer timestamps. This bug is to remove the renderer timestamps and rely strictly on the swap timestamps in PaintTiming and FirstMeaningfulPaintDetector. As part of this bug, remove the TODO( crbug.com/738235 )'s in PaintTiming and FirstMeaningfulPaintDetector for "Consider not reporting any timestamp when failing for reasons other than kDidNotSwapSwapFails." The current method is consistent for all SwapPromise results, and the accuracy has been improved over the blink renderer timestamp. The data shows that these cases are rare, and when they occur it is ideal that the timestamps are consistent with the kDidSwap and kDidNotSwapSwapFails cases.
,
Jul 31 2017
,
Aug 31 2017
,
Jan 4 2018
Max, could you take this cleanup bug?
,
Jan 5 2018
Yep I can take that.
,
Apr 23 2018
shaseley: If I understand correctly, first_*_swap_ and first_*_ are functionally the same, with one value more precise than the other. Now this bug is to remove all of first_*_ and their related logics, is that right? https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/paint/paint_timing.h?rcl=cfb4f38aebe493e438b40761997f95dc6807d6cb&l=157 https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/paint/first_meaningful_paint_detector.h?rcl=682a3df7a2c8046066f265956b9adcd900e9bbfe&l=89
,
Apr 23 2018
shaseley@ isn't around at this point. I believe your understanding is correct.
,
Aug 2
maxlg@ any update?
,
Aug 2
I have attempted to work on this but it was not successful because of a failing test: https://chromium-review.googlesource.com/c/chromium/src/+/1025116 And I haven't prioritized it again yet. Will work on it later.
,
Nov 26
Max, I think you removed these already, yea?
,
Nov 26
nope, I haven't. I will kick off another try soon later.
,
Dec 4
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/35a754d33b59d709e806068dd77347511e18daef commit 35a754d33b59d709e806068dd77347511e18daef Author: Liquan(Max) Gu <maxlg@chromium.org> Date: Tue Dec 04 01:28:13 2018 [FMP] Clean up renderer timestamp from FirstMeaningfulPaintDetector The current implementation has switched to use until-GPU-swap timestamp, but the code base still keep the renderer timestamp. As the renderer timestamp is no longer used. We should remove it. The CL cleans up the renderer timestamp from paint-timing and first-meaningful-paint-detector. But this CL has leftover work, as we still have first_meaningful_paint2_quiet_ and first_meaningful_paint2_quiet_swap. Intuitively we would remove first_meaningful_paint2_quiet_ and keep first_meaningful_paint2_quiet_swap. But first_meaningful_paint2_quiet_ is still playing a role in the algorithm, which therefore makes the clean up a bit tricker. We will focus on this issue in a later patch. Bug: 747484 Change-Id: I8bdb5f4551fe823ad56fd26d735e37e626006c61 Reviewed-on: https://chromium-review.googlesource.com/c/1355258 Reviewed-by: Chris Harrelson <chrishtr@chromium.org> Commit-Queue: Liquan (Max) Gǔ <maxlg@chromium.org> Cr-Commit-Position: refs/heads/master@{#613374} [modify] https://crrev.com/35a754d33b59d709e806068dd77347511e18daef/third_party/blink/renderer/core/paint/first_meaningful_paint_detector.cc [modify] https://crrev.com/35a754d33b59d709e806068dd77347511e18daef/third_party/blink/renderer/core/paint/first_meaningful_paint_detector.h [modify] https://crrev.com/35a754d33b59d709e806068dd77347511e18daef/third_party/blink/renderer/core/paint/first_meaningful_paint_detector_test.cc [modify] https://crrev.com/35a754d33b59d709e806068dd77347511e18daef/third_party/blink/renderer/core/paint/paint_timing.cc [modify] https://crrev.com/35a754d33b59d709e806068dd77347511e18daef/third_party/blink/renderer/core/paint/paint_timing.h
,
Yesterday
(28 hours ago)
I've made an initial cleanup for the timestamps of FMP, but there is still some codes using the render time, which I am not aware whether they can be replaced directly with swap time yet. The concern of it is that swap time is not assigned immediately as render time does. So the change may or may not be one replacing the other. I have to get more familiar with the code before making the change. |
|||||
►
Sign in to add a comment |
|||||
Comment 1 by sheriffbot@chromium.org
, Jul 24 2017