Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2623613008:
Add a new top-level inspector timeline event for the PrePaint step (Closed)
|
Created:
3 years, 11 months ago by pdr. Modified:
3 years, 11 months ago CC:
apavlov+blink_chromium.org, blink-reviews, caseq+blink_chromium.org, chromium-reviews, devtools-reviews_chromium.org, kozyatinskiy+blink_chromium.org, lushnikov+blink_chromium.org, pfeldman+blink_chromium.org, pfeldman Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd a new top-level inspector timeline event for the PrePaint step
Slimming paint invalidation moves paint invalidation from the
"Update Layer Tree" step to a new "Pre-paint" step. This patch
adds an accompanying inspector timeline event for this new
lifecycle phase.
In the code we use "pre-paint" to mean "before-paint" but that
may be confusing to end-users who might confuse it with the
pre-paint window, or pre-fetch concepts. "prepare paint" is
used instead.
Before this patch, paint invalidation is listed under the
"Update Layer Tree" timeline. After this patch, paint invalidation
is listed under the "Prepare Paint" timeline if
--enable-slimming-paint-invalidation is used.
BUG=679403
Patch Set 1 #
Messages
Total messages: 22 (5 generated)
Description was changed from ========== Add a new top-level inspector timeline event for the PrePaint step Slimming paint invalidation moves paint invalidation from the "Update Layer Tree" step to a new "Pre-paint" step. This patch adds an accompanying inspector timeline event for this new lifecycle phase. In the code we use "pre-paint" to mean "before-paint" but that may be confusing to end-users who might confuse it with the pre-paint window, or pre-fetch concepts. "prepare paint" is used. BUG=679403 ========== to ========== Add a new top-level inspector timeline event for the PrePaint step Slimming paint invalidation moves paint invalidation from the "Update Layer Tree" step to a new "Pre-paint" step. This patch adds an accompanying inspector timeline event for this new lifecycle phase. In the code we use "pre-paint" to mean "before-paint" but that may be confusing to end-users who might confuse it with the pre-paint window, or pre-fetch concepts. "prepare paint" is used instead. BUG=679403 ==========
pdr@chromium.org changed reviewers: + caseq@chromium.org, chrishtr@chromium.org, wangxianzhu@chromium.org
lgtm
lgtm lgtm. I should have named prePaint as preparePaint which looks better.
The CQ bit was checked by chrishtr@chromium.org
lgtm
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 chrishtr@chromium.org
Actually: why should we communicate "pre-paint" to developers at all? It seems better to fold it into Paint.
On 2017/01/10 17:54:32, chrishtr wrote: > Actually: why should we communicate "pre-paint" to developers at all? It seems > better > to fold it into Paint. Well, Paint events are per-layer and this one is per-page from what I can see... I wonder, though, do we ever happen to spend a noticeable amount of time there? I played with this patch a bit and in all cases where we had some time spent in both update layers and paint, this one still was on the order of a few microseconds.
On 2017/01/10 at 19:10:51, caseq wrote: > On 2017/01/10 17:54:32, chrishtr wrote: > > Actually: why should we communicate "pre-paint" to developers at all? It seems > > better > > to fold it into Paint. > > Well, Paint events are per-layer and this one is per-page from what I can see... I wonder, though, do we ever happen to spend a noticeable amount of time there? I played with this patch a bit and in all cases where we had some time spent in both update layers and paint, this one still was on the order of a few microseconds. Paint is not really per-layer any more. I think we should change it to be per-page.
On 2017/01/10 at 19:10:51, caseq wrote: > On 2017/01/10 17:54:32, chrishtr wrote: > > Actually: why should we communicate "pre-paint" to developers at all? It seems > > better > > to fold it into Paint. > > Well, Paint events are per-layer and this one is per-page from what I can see... I wonder, though, do we ever happen to spend a noticeable amount of time there? I played with this patch a bit and in all cases where we had some time spent in both update layers and paint, this one still was on the order of a few microseconds. I think you're seeing this because it requires --enable-slimming-paint-invalidation. Without that flag, we should spend a bit of time (e.g., ~150ms on https://www.w3.org/TR/html5/single-page.html) in Update Layer Tree, and with the flag you should see roughly the same amount of time in the new "Prepare Paint" step. @caseq, does this explain what you were seeing? I agree with chrishtr about "prepare paint" not being particularly actionable for end-users, but caseq's point about paint being per-GraphicsLayer is a good one. When slimming paint v2 launches, we should just have a single GraphicsLayer, but in this intermediate step of "slimming paint invalidation", the per-GraphicsLayer distinction is still useful. I think it makes sense to go ahead with "prepare paint" and then fold this into "paint" for SPV2. What do you all think?
On 2017/01/10 at 19:15:45, chrishtr wrote: > On 2017/01/10 at 19:10:51, caseq wrote: > > On 2017/01/10 17:54:32, chrishtr wrote: > > > Actually: why should we communicate "pre-paint" to developers at all? It seems > > > better > > > to fold it into Paint. > > > > Well, Paint events are per-layer and this one is per-page from what I can see... I wonder, though, do we ever happen to spend a noticeable amount of time there? I played with this patch a bit and in all cases where we had some time spent in both update layers and paint, this one still was on the order of a few microseconds. > > Paint is not really per-layer any more. I think we should change it to be per-page. Sorry, we were replying at the same time. Isn't paint still per-layer in spv1?
On 2017/01/10 19:23:40, pdr. wrote: > On 2017/01/10 at 19:10:51, caseq wrote: > > On 2017/01/10 17:54:32, chrishtr wrote: > > > Actually: why should we communicate "pre-paint" to developers at all? It > seems > > > better > > > to fold it into Paint. > > > > Well, Paint events are per-layer and this one is per-page from what I can > see... I wonder, though, do we ever happen to spend a noticeable amount of time > there? I played with this patch a bit and in all cases where we had some time > spent in both update layers and paint, this one still was on the order of a few > microseconds. > > I think you're seeing this because it requires > --enable-slimming-paint-invalidation. Without that flag, we should spend a bit > of time (e.g., ~150ms on https://www.w3.org/TR/html5/single-page.html) in Update > Layer Tree, and with the flag you should see roughly the same amount of time in > the new "Prepare Paint" step. @caseq, does this explain what you were seeing? Ouch, indeed -- quite silly of me, considering I've seen the check in the code. Gotta get some coffee! > I agree with chrishtr about "prepare paint" not being particularly actionable > for end-users, but caseq's point about paint being per-GraphicsLayer is a good > one. When slimming paint v2 launches, we should just have a single > GraphicsLayer, but in this intermediate step of "slimming paint invalidation", > the per-GraphicsLayer distinction is still useful. I think it makes sense to go > ahead with "prepare paint" and then fold this into "paint" for SPV2. What do you > all think? Yup, makes sense to me. Also, we're currently producing complete SkPictures for each layer's paint -- I guess with SPV2 we could potentially switch to a somewhat more efficient approach?
On 2017/01/10 at 19:31:21, pdr wrote: > On 2017/01/10 at 19:15:45, chrishtr wrote: > > On 2017/01/10 at 19:10:51, caseq wrote: > > > On 2017/01/10 17:54:32, chrishtr wrote: > > > > Actually: why should we communicate "pre-paint" to developers at all? It seems > > > > better > > > > to fold it into Paint. > > > > > > Well, Paint events are per-layer and this one is per-page from what I can see... I wonder, though, do we ever happen to spend a noticeable amount of time there? I played with this patch a bit and in all cases where we had some time spent in both update layers and paint, this one still was on the order of a few microseconds. > > > > Paint is not really per-layer any more. I think we should change it to be per-page. > > Sorry, we were replying at the same time. Isn't paint still per-layer in spv1? Yeah, you're right that it's per-layer. Ok to leave it for now..
On 2017/01/10 at 19:33:35, caseq wrote: > On 2017/01/10 19:23:40, pdr. wrote: > > On 2017/01/10 at 19:10:51, caseq wrote: > > > On 2017/01/10 17:54:32, chrishtr wrote: > > > > Actually: why should we communicate "pre-paint" to developers at all? It > > seems > > > > better > > > > to fold it into Paint. > > > > > > Well, Paint events are per-layer and this one is per-page from what I can > > see... I wonder, though, do we ever happen to spend a noticeable amount of time > > there? I played with this patch a bit and in all cases where we had some time > > spent in both update layers and paint, this one still was on the order of a few > > microseconds. > > > > I think you're seeing this because it requires > > --enable-slimming-paint-invalidation. Without that flag, we should spend a bit > > of time (e.g., ~150ms on https://www.w3.org/TR/html5/single-page.html) in Update > > Layer Tree, and with the flag you should see roughly the same amount of time in > > the new "Prepare Paint" step. @caseq, does this explain what you were seeing? > > Ouch, indeed -- quite silly of me, considering I've seen the check in the code. Gotta get some coffee! > > > I agree with chrishtr about "prepare paint" not being particularly actionable > > for end-users, but caseq's point about paint being per-GraphicsLayer is a good > > one. When slimming paint v2 launches, we should just have a single > > GraphicsLayer, but in this intermediate step of "slimming paint invalidation", > > the per-GraphicsLayer distinction is still useful. I think it makes sense to go > > ahead with "prepare paint" and then fold this into "paint" for SPV2. What do you > > all think? > > Yup, makes sense to me. Also, we're currently producing complete SkPictures for each layer's paint -- I guess with SPV2 we could potentially switch to a somewhat more efficient approach? Isn't it weird to introduce "prepare paint" and then get rid of it a few months later? I'd prefer to pretend to the developer it's "update layer tree", then make a change later to move it all to "paint"
Description was changed from ========== Add a new top-level inspector timeline event for the PrePaint step Slimming paint invalidation moves paint invalidation from the "Update Layer Tree" step to a new "Pre-paint" step. This patch adds an accompanying inspector timeline event for this new lifecycle phase. In the code we use "pre-paint" to mean "before-paint" but that may be confusing to end-users who might confuse it with the pre-paint window, or pre-fetch concepts. "prepare paint" is used instead. BUG=679403 ========== to ========== Add a new top-level inspector timeline event for the PrePaint step Slimming paint invalidation moves paint invalidation from the "Update Layer Tree" step to a new "Pre-paint" step. This patch adds an accompanying inspector timeline event for this new lifecycle phase. In the code we use "pre-paint" to mean "before-paint" but that may be confusing to end-users who might confuse it with the pre-paint window, or pre-fetch concepts. "prepare paint" is used instead. Before this patch, paint invalidation is listed under the "Update Layer Tree" timeline. After this patch, paint invalidation is listed under the "Prepare Paint" timeline if --enable-slimming-paint-invalidation is used. BUG=679403 ==========
On 2017/01/10 19:45:16, chrishtr wrote: > On 2017/01/10 at 19:31:21, pdr wrote: > > On 2017/01/10 at 19:15:45, chrishtr wrote: > > > On 2017/01/10 at 19:10:51, caseq wrote: > > > > On 2017/01/10 17:54:32, chrishtr wrote: > > > > > Actually: why should we communicate "pre-paint" to developers at all? It > seems > > > > > better > > > > > to fold it into Paint. > > > > > > > > Well, Paint events are per-layer and this one is per-page from what I can > see... I wonder, though, do we ever happen to spend a noticeable amount of time > there? I played with this patch a bit and in all cases where we had some time > spent in both update layers and paint, this one still was on the order of a few > microseconds. > > > > > > Paint is not really per-layer any more. I think we should change it to be > per-page. > > > > Sorry, we were replying at the same time. Isn't paint still per-layer in spv1? > > Yeah, you're right that it's per-layer. Ok to leave it for now.. I think we can treat the per-layer paints as one paint because we paint all the layers at once, then we can combine FrameView::prePaint() and FrameView::paintTree() into one Paint event in timeline.
On 2017/01/10 19:59:13, Xianzhu wrote: > I think we can treat the per-layer paints as one paint because we paint all the > layers at once, then we can combine FrameView::prePaint() and > FrameView::paintTree() into one Paint event in timeline. Timeline currently expects a SkPicture for the painted layer to be emitted from within the Paint event for that layer, this is how we match pictures to layers in the Layers viewer, so this logic would have to be fixed as well.
On 2017/01/10 20:12:50, caseq wrote: > On 2017/01/10 19:59:13, Xianzhu wrote: > > > I think we can treat the per-layer paints as one paint because we paint all > the > > layers at once, then we can combine FrameView::prePaint() and > > FrameView::paintTree() into one Paint event in timeline. > > Timeline currently expects a SkPicture for the painted layer to be emitted from > within the Paint event for that layer, this is how we match pictures to layers > in the Layers viewer, so this logic would have to be fixed as well. I see. We should keep per-layer paints for SPv1. I agree with Chris about pretending "prepare paint" as "update layer tree".
On 2017/01/10 at 20:15:45, wangxianzhu wrote: > On 2017/01/10 20:12:50, caseq wrote: > > On 2017/01/10 19:59:13, Xianzhu wrote: > > > > > I think we can treat the per-layer paints as one paint because we paint all > > the > > > layers at once, then we can combine FrameView::prePaint() and > > > FrameView::paintTree() into one Paint event in timeline. > > > > Timeline currently expects a SkPicture for the painted layer to be emitted from > > within the Paint event for that layer, this is how we match pictures to layers > > in the Layers viewer, so this logic would have to be fixed as well. > > I see. We should keep per-layer paints for SPv1. > > I agree with Chris about pretending "prepare paint" as "update layer tree". This was a good discussion. I also agree. I've uploaded a new patch at https://codereview.chromium.org/2622983002 and will close this review. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
