Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1075523002:
cc: Add UMA stats for record and raster time. (Closed)
|
Created:
5 years, 8 months ago by jbroman Modified:
5 years, 8 months ago CC:
asvitkine+watch_chromium.org, cc-bugs_chromium.org, chrishtr, chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptioncc: Add UMA stats for record and raster time.
These should provide a basic smoke test for real-world regressions from
Slimming Paint phase 1.
BUG=471873
Committed: https://crrev.com/187bc57e8b032fea86a401a7fd7c19378f79da5b
Cr-Commit-Position: refs/heads/master@{#325360}
Patch Set 1 #
Total comments: 6
Patch Set 2 : use area actually recorded #
Total comments: 3
Patch Set 3 : move timers into recording source implementations #Patch Set 4 : fix histograms.xml #
Total comments: 6
Patch Set 5 : merge with master #Patch Set 6 : retry bad upload #Patch Set 7 : merge with master #Patch Set 8 : explicitly handle and test edge cases #Patch Set 9 : change constructor/destructor visibility #
Total comments: 6
Patch Set 10 : make sampled time at least 1us, per chrishtr #Patch Set 11 : rename histograms #
Total comments: 8
Patch Set 12 : merge with master #Patch Set 13 : rename histograms #Patch Set 14 : merge with master #Patch Set 15 : intersect with recorded_viewport_ #
Total comments: 10
Patch Set 16 : isherman comments, esp. renaming of histograms #Patch Set 17 : git cl format #
Messages
Total messages: 48 (11 generated)
enne@chromium.org changed reviewers: + enne@chromium.org
https://codereview.chromium.org/1075523002/diff/1/cc/layers/picture_layer.cc File cc/layers/picture_layer.cc (right): https://codereview.chromium.org/1075523002/diff/1/cc/layers/picture_layer.cc#... cc/layers/picture_layer.cc:177: visible_layer_rect.size().GetArea() / elapsed.InMillisecondsF()); visible layer rect is an inaccurate numerator. You're really looking for recording area, yeah? Maybe you should sum up the total pictures created and their areas? https://codereview.chromium.org/1075523002/diff/1/cc/resources/tile_manager.cc File cc/resources/tile_manager.cc (right): https://codereview.chromium.org/1075523002/diff/1/cc/resources/tile_manager.c... cc/resources/tile_manager.cc:121: UMA_HISTOGRAM_COUNTS("Renderer4.RasterTaskUs", elapsed.InMicroseconds()); This one seems like a pretty good regression metric, although it is definitely prone to influence by outside changes, e.g. image decoding improvements. I think if we ever start rastering from the display list directly, it might be worth having a "null canvas" microbenchmark, where we don't do any of the work, but just iterate through it as if we were doing the work.
https://codereview.chromium.org/1075523002/diff/1/cc/layers/picture_layer.cc File cc/layers/picture_layer.cc (right): https://codereview.chromium.org/1075523002/diff/1/cc/layers/picture_layer.cc#... cc/layers/picture_layer.cc:177: visible_layer_rect.size().GetArea() / elapsed.InMillisecondsF()); On 2015/04/08 20:18:45, enne wrote: > visible layer rect is an inaccurate numerator. You're really looking for > recording area, yeah? > > Maybe you should sum up the total pictures created and their areas? I'm hoping to avoid using an area that has different meaning before and after slimming paint v2. I agree that this metric isn't great. I'm not 100% sure I understand what you're suggesting here. Are the bounds of the pictures really guaranteed to meaningfully represent the invalidated area we asked to record? (For example, if I record a small rect somewhere, but it happens to be be filled with a much larger rect, won't this radically overestimate the recorded area?) The visible layer rect is what RasterizeAndRecordBenchmark uses, though I realize in that case the recording is always the full visible layer size. Is that why it's inapplicable here? If we can't find a good option, we might also just consider dropping this one and only tracking the per-layer record time. https://codereview.chromium.org/1075523002/diff/1/cc/resources/tile_manager.cc File cc/resources/tile_manager.cc (right): https://codereview.chromium.org/1075523002/diff/1/cc/resources/tile_manager.c... cc/resources/tile_manager.cc:121: UMA_HISTOGRAM_COUNTS("Renderer4.RasterTaskUs", elapsed.InMicroseconds()); On 2015/04/08 20:18:46, enne wrote: > This one seems like a pretty good regression metric, although it is definitely > prone to influence by outside changes, e.g. image decoding improvements. Agreed. I think the best we're hoping for is to have a signal if we suddenly make things dramatically worse in the real world. > I think if we ever start rastering from the display list directly, it might be > worth having a "null canvas" microbenchmark, where we don't do any of the work, > but just iterate through it as if we were doing the work. Might be an interesting microbenchmark, sure, but it seems orthogonal to this bug, no?
https://codereview.chromium.org/1075523002/diff/1/cc/layers/picture_layer.cc File cc/layers/picture_layer.cc (right): https://codereview.chromium.org/1075523002/diff/1/cc/layers/picture_layer.cc#... cc/layers/picture_layer.cc:177: visible_layer_rect.size().GetArea() / elapsed.InMillisecondsF()); On 2015/04/08 20:37:42, jbroman wrote: > I'm not 100% sure I understand what you're suggesting here. Are the bounds of > the pictures really guaranteed to meaningfully represent the invalidated area we > asked to record? (For example, if I record a small rect somewhere, but it > happens to be be filled with a much larger rect, won't this radically > overestimate the recorded area?) No, the bounds are not the invalidated area. The bounds are the recorded area. What I want to know is "if I say record WxH bounds in Blink, how fast is that?". I think that's the unit that we can meaningfully compare? > The visible layer rect is what RasterizeAndRecordBenchmark uses, though I > realize in that case the recording is always the full visible layer size. Is > that why it's inapplicable here? That's exactly it. > If we can't find a good option, we might also just consider dropping this one > and only tracking the per-layer record time. Record time (in my experience) is about number of calls to paint and about the size of the paint. Per-layer time will give inflated numbers for multiple record calls. https://codereview.chromium.org/1075523002/diff/1/cc/resources/tile_manager.cc File cc/resources/tile_manager.cc (right): https://codereview.chromium.org/1075523002/diff/1/cc/resources/tile_manager.c... cc/resources/tile_manager.cc:121: UMA_HISTOGRAM_COUNTS("Renderer4.RasterTaskUs", elapsed.InMicroseconds()); On 2015/04/08 20:37:42, jbroman wrote: > Might be an interesting microbenchmark, sure, but it seems orthogonal to this > bug, no? Yeah. Just trying to think out loud about more targeted metrics that might better track the real performance issues with less noise.
Is this generally what you mean? (Modulo unit tests, which I haven't yet written.) https://codereview.chromium.org/1075523002/diff/20001/cc/resources/recording_... File cc/resources/recording_source.h (right): https://codereview.chromium.org/1075523002/diff/20001/cc/resources/recording_... cc/resources/recording_source.h:41: int* recorded_area) = 0; Unsure about parameter order. Style guide tells me all out and in-out parameters should be at the end with Chromium style, but existing ones are at the beginning, so I'm not sure.
https://codereview.chromium.org/1075523002/diff/20001/cc/layers/picture_layer.cc File cc/layers/picture_layer.cc (right): https://codereview.chromium.org/1075523002/diff/20001/cc/layers/picture_layer... cc/layers/picture_layer.cc:163: &recorded_area); Yeah, I think this is the right metric. Can you put all of this internal to PicturePile so recorded area doesn't have to get returned? Yeah, yeah, you have to duplicate the UMA stat call, but I think it's ok to have bolted on stats, rather than plumbing their dependencies through like they're first class citizens.
Note also that there already are large differences arising from the fact that the recording area is much larger for slimming paint. e.g. here's what happens when I hover a link on Wikipedia (grabbing a random entry): (no slimming paint, recorded area is ~247009) Renderer4.PicturePileUpdateUs=945 Renderer4.PicturePileUpdatePixelsPerMs=261385 Renderer4.RasterTaskUs=418 Renderer4.RasterTaskPixelsPerMs=156784 (slimming paint, recorded area is ~3053985) Renderer4.DisplayListRecordingSourceUpdateUs=3504 Renderer4.DisplayListRecordingSourceUpdatePixelsPerMs=871571 Renderer4.RasterTaskUs=434 Renderer4.RasterTaskPixelsPerMs=151004 The downside of this is that it's hard to tell how bad it has to be for us to raise the alarm. Still, probably better than nothing. https://codereview.chromium.org/1075523002/diff/20001/cc/layers/picture_layer.cc File cc/layers/picture_layer.cc (right): https://codereview.chromium.org/1075523002/diff/20001/cc/layers/picture_layer... cc/layers/picture_layer.cc:163: &recorded_area); On 2015/04/09 17:37:34, enne wrote: > Yeah, I think this is the right metric. Can you put all of this internal to > PicturePile so recorded area doesn't have to get returned? > > Yeah, yeah, you have to duplicate the UMA stat call, but I think it's ok to have > bolted on stats, rather than plumbing their dependencies through like they're > first class citizens. Okay, split (with different stat names). This triggered Fowler's rule of three, so I refactored into a macro (but the chromium-style plugin made me make the destructor non-inline because it has a templated member . . . oh well). PTAL.
(whoops, fixing name/description mismatches in histograms.xml)
That is a pretty big difference. It's not clear to me that those numbers are at all comparable. T_T Are they more similar to the numbers that we see from cluster telemetry if PicturePileUpdatePixelsPerMs is total recorded area vs newly recorded area?
On 2015/04/09 21:11:43, enne wrote: > That is a pretty big difference. It's not clear to me that those numbers are at > all comparable. T_T > > Are they more similar to the numbers that we see from cluster telemetry if > PicturePileUpdatePixelsPerMs is total recorded area vs newly recorded area? I can check, but I'd expect that to make the recorded area for the purposes of measurement equal between SP and non-SP, and thus the "pixels per ms" to have the same ~3x ratio (well, the reciprocal) that the record times do. Or does cluster telemetry do something else magical I'm not understanding?
+chrishtr, since he reported this bug and may have opinions about what kind of metrics he wants
chrishtr@chromium.org changed reviewers: + chrishtr@chromium.org
https://codereview.chromium.org/1075523002/diff/60001/cc/base/histogram_macros.h File cc/base/histogram_macros.h (right): https://codereview.chromium.org/1075523002/diff/60001/cc/base/histogram_macro... cc/base/histogram_macros.h:16: // pixels per millisecond. Why the difference in time units? https://codereview.chromium.org/1075523002/diff/60001/cc/base/histogram_macro... cc/base/histogram_macros.h:45: UMA_HISTOGRAM_COUNTS(area_histogram, area / elapsed.InMillisecondsF()); \ Protect against divide by zero? https://codereview.chromium.org/1075523002/diff/60001/cc/resources/display_li... File cc/resources/display_list_recording_source.cc (right): https://codereview.chromium.org/1075523002/diff/60001/cc/resources/display_li... cc/resources/display_list_recording_source.cc:130: timer.SetArea(recorded_viewport_.size().GetArea()); What if you just set the area to the invalidation rects, not the recorded_viewport_?
PTAL. Handling division-by-zero edge cases demands unit test, which demands refactoring. Apologies for the inflated code size. https://codereview.chromium.org/1075523002/diff/60001/cc/base/histogram_macros.h File cc/base/histogram_macros.h (right): https://codereview.chromium.org/1075523002/diff/60001/cc/base/histogram_macro... cc/base/histogram_macros.h:16: // pixels per millisecond. On 2015/04/10 17:41:39, chrishtr wrote: > Why the difference in time units? Time would have been in milliseconds, but UMA records integral values only (so we'd mostly record values in the range 0-2 which is rather coarse). For pixels/time, no such issue applies (and in fact 0-1M pixels/ms is about the range I'd expect to see in practice, using my Z620 as a rough upper bound; 1M is the default max value of UMA_HISTOGRAM_COUNTS). No strong objections to changing to pixels/microsecond, though, if you want that. https://codereview.chromium.org/1075523002/diff/60001/cc/base/histogram_macro... cc/base/histogram_macros.h:45: UMA_HISTOGRAM_COUNTS(area_histogram, area / elapsed.InMillisecondsF()); \ On 2015/04/10 17:41:39, chrishtr wrote: > Protect against divide by zero? Euh, the division is defined, but I forgot that the cast back to int32_t is undefined. Since that's evidence that this is complicated enough to demand unit test, I've refactored a base class with unit tests out. saturated_cast + IsNaN should be enough to handle this. https://codereview.chromium.org/1075523002/diff/60001/cc/resources/display_li... File cc/resources/display_list_recording_source.cc (right): https://codereview.chromium.org/1075523002/diff/60001/cc/resources/display_li... cc/resources/display_list_recording_source.cc:130: timer.SetArea(recorded_viewport_.size().GetArea()); On 2015/04/10 17:41:39, chrishtr wrote: > What if you just set the area to the invalidation rects, not the > recorded_viewport_? Changed as discussed in meeting on Friday.
https://codereview.chromium.org/1075523002/diff/160001/cc/base/histograms_uni... File cc/base/histograms_unittest.cc (right): https://codereview.chromium.org/1075523002/diff/160001/cc/base/histograms_uni... cc/base/histograms_unittest.cc:51: std::numeric_limits<Sample>::max()); Shouldn't this case just bail and not record a histogram entry? https://codereview.chromium.org/1075523002/diff/160001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1075523002/diff/160001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:31147: +<histogram name="Renderer4.DisplayListRecordingSourcePixelsPerMs"> Instead of PixelsPerMs, RecordedAreaPerMs to be less misleading?
https://codereview.chromium.org/1075523002/diff/160001/cc/base/histograms_uni... File cc/base/histograms_unittest.cc (right): https://codereview.chromium.org/1075523002/diff/160001/cc/base/histograms_uni... cc/base/histograms_unittest.cc:51: std::numeric_limits<Sample>::max()); On 2015/04/13 19:54:09, chrishtr wrote: > Shouldn't this case just bail and not record a histogram entry? Should it? It seems to me that if we do often manage to record in under one microsecond (which is what maps to 0), we ought to report that. I don't feel super strongly (especially since I suspect this is uncommon), but this doesn't seem irrational to me. It's easy enough to change, though.
https://codereview.chromium.org/1075523002/diff/160001/cc/base/histograms_uni... File cc/base/histograms_unittest.cc (right): https://codereview.chromium.org/1075523002/diff/160001/cc/base/histograms_uni... cc/base/histograms_unittest.cc:51: std::numeric_limits<Sample>::max()); On 2015/04/13 at 19:58:04, jbroman wrote: > On 2015/04/13 19:54:09, chrishtr wrote: > > Shouldn't this case just bail and not record a histogram entry? > > Should it? It seems to me that if we do often manage to record in under one microsecond (which is what maps to 0), we ought to report that. I don't feel super strongly (especially since I suspect this is uncommon), but this doesn't seem irrational to me. > > It's easy enough to change, though. I think it would be bad to average with infinity (if that is what would happen) and allow it to affect the stats histograms. Alternatively, you could just round up all times to be at least one microsecond, which on reflection seems fine because the measurement timing probably has that much noise anyway. This would avoid the whole issue of dividing by zero.
https://codereview.chromium.org/1075523002/diff/160001/cc/base/histograms_uni... File cc/base/histograms_unittest.cc (right): https://codereview.chromium.org/1075523002/diff/160001/cc/base/histograms_uni... cc/base/histograms_unittest.cc:51: std::numeric_limits<Sample>::max()); On 2015/04/13 20:06:19, chrishtr wrote: > On 2015/04/13 at 19:58:04, jbroman wrote: > > On 2015/04/13 19:54:09, chrishtr wrote: > > > Shouldn't this case just bail and not record a histogram entry? > > > > Should it? It seems to me that if we do often manage to record in under one > microsecond (which is what maps to 0), we ought to report that. I don't feel > super strongly (especially since I suspect this is uncommon), but this doesn't > seem irrational to me. > > > > It's easy enough to change, though. > > I think it would be bad to average with infinity (if that is what would happen) > and allow it to affect the stats histograms. Alternatively, you could just round > up all times to be at least one microsecond, which on reflection seems fine > because the measurement timing probably has that much noise anyway. This would > avoid the whole issue of dividing by zero. Well, not infinity, just a big number. And of course the histogram bucket data is still there. Changed, with tests correspondingly adjusted to show the results. https://codereview.chromium.org/1075523002/diff/160001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1075523002/diff/160001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:31147: +<histogram name="Renderer4.DisplayListRecordingSourcePixelsPerMs"> On 2015/04/13 19:54:09, chrishtr wrote: > Instead of PixelsPerMs, RecordedAreaPerMs to be less misleading? Done. Let me know if the adjusted names are more to your liking.
lgtm
https://codereview.chromium.org/1075523002/diff/200001/cc/resources/display_l... File cc/resources/display_list_recording_source.cc (right): https://codereview.chromium.org/1075523002/diff/200001/cc/resources/display_l... cc/resources/display_list_recording_source.cc:85: for (Region::Iterator it(*invalidation); it.has_rect(); it.next()) This is a petty nit, but it's weird to have this inside the timer. I think you could get away with the timer having area as a ctor arg and no Set/Add area calls. https://codereview.chromium.org/1075523002/diff/200001/cc/resources/picture_p... File cc/resources/picture_pile.cc (right): https://codereview.chromium.org/1075523002/diff/200001/cc/resources/picture_p... cc/resources/picture_pile.cc:201: for (Region::Iterator it(*invalidation); it.has_rect(); it.next()) You need to intersect these invalidations with the interest rect. Both recording sources would handle a billion pixel long page that fully invalidated every frame at the same speed as an interest rect-sized page that fully invalidated every frame. https://codereview.chromium.org/1075523002/diff/200001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1075523002/diff/200001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:31150: + Area of recorded content, in pixels, divided by display list update time, in "Area of recorded content" is not quite precise. Area of recorded content would be what the previous patch did, in adding up new cc::Picture sizes that frame. It's the area of invalidated content that's inside the interest rect of the recording source. Maybe "area of invalidated content" is enough?
https://codereview.chromium.org/1075523002/diff/200001/cc/resources/display_l... File cc/resources/display_list_recording_source.cc (right): https://codereview.chromium.org/1075523002/diff/200001/cc/resources/display_l... cc/resources/display_list_recording_source.cc:85: for (Region::Iterator it(*invalidation); it.has_rect(); it.next()) On 2015/04/13 21:52:26, enne wrote: > This is a petty nit, but it's weird to have this inside the timer. I think you > could get away with the timer having area as a ctor arg and no Set/Add area > calls. Prevents the work before the invalidation is available (e.g. in PicturePile::ApplyInvalidationAndResize) from being counted in the time. (Less of a big deal now that the invalidation rects, which are available earlier, are being used.) Happy to make this change if that tradeoff sgty. https://codereview.chromium.org/1075523002/diff/200001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1075523002/diff/200001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:31150: + Area of recorded content, in pixels, divided by display list update time, in On 2015/04/13 21:52:27, enne wrote: > "Area of recorded content" is not quite precise. Area of recorded content would > be what the previous patch did, in adding up new cc::Picture sizes that frame. > It's the area of invalidated content that's inside the interest rect of the > recording source. Maybe "area of invalidated content" is enough? I'm not quite sure what name you are suggesting. I've made a change, but I'm not convinced it's properly addressing your comment.
https://codereview.chromium.org/1075523002/diff/200001/cc/resources/picture_p... File cc/resources/picture_pile.cc (right): https://codereview.chromium.org/1075523002/diff/200001/cc/resources/picture_p... cc/resources/picture_pile.cc:201: for (Region::Iterator it(*invalidation); it.has_rect(); it.next()) On 2015/04/13 21:52:27, enne wrote: > You need to intersect these invalidations with the interest rect. Both > recording sources would handle a billion pixel long page that fully invalidated > every frame at the same speed as an interest rect-sized page that fully > invalidated every frame. Which rect is the right one to intersect with? I see interest_rect in PicturePile, but not in DisplayListRecordingSource, and it's not clear to me what the distinction between the interest rect and the recorded viewport (apparently the intsersection of the interest rect with the layer bounds?) is. Can you clarify this for me, please? Thanks for your patience. :)
lgtm, thanks for putting up with all the back and forth. :) https://codereview.chromium.org/1075523002/diff/200001/cc/resources/display_l... File cc/resources/display_list_recording_source.cc (right): https://codereview.chromium.org/1075523002/diff/200001/cc/resources/display_l... cc/resources/display_list_recording_source.cc:85: for (Region::Iterator it(*invalidation); it.has_rect(); it.next()) On 2015/04/14 14:44:36, jbroman wrote: > On 2015/04/13 21:52:26, enne wrote: > > This is a petty nit, but it's weird to have this inside the timer. I think > you > > could get away with the timer having area as a ctor arg and no Set/Add area > > calls. > > Prevents the work before the invalidation is available (e.g. in > PicturePile::ApplyInvalidationAndResize) from being counted in the time. (Less > of a big deal now that the invalidation rects, which are available earlier, are > being used.) > > Happy to make this change if that tradeoff sgty. Hmm, ok, this is fine. I am not sure whether it makes sense to include the synthetic invalidations here, but I think that's ok. I suspect the display lists are going to need the same synthetic invalidations for resizing anyway, so maybe it'll balanceo ut.
https://codereview.chromium.org/1075523002/diff/200001/cc/resources/picture_p... File cc/resources/picture_pile.cc (right): https://codereview.chromium.org/1075523002/diff/200001/cc/resources/picture_p... cc/resources/picture_pile.cc:201: for (Region::Iterator it(*invalidation); it.has_rect(); it.next()) On 2015/04/14 16:02:17, jbroman wrote: > On 2015/04/13 21:52:27, enne wrote: > > You need to intersect these invalidations with the interest rect. Both > > recording sources would handle a billion pixel long page that fully > invalidated > > every frame at the same speed as an interest rect-sized page that fully > > invalidated every frame. > > Which rect is the right one to intersect with? I see interest_rect in > PicturePile, but not in DisplayListRecordingSource, and it's not clear to me > what the distinction between the interest rect and the recorded viewport > (apparently the intsersection of the interest rect with the layer bounds?) is. > > Can you clarify this for me, please? Thanks for your patience. :) Oops, sorry forgot to address this comment. You should intersect with the recorded_viewport_, which is the interest rect intersected the layer bounds.
On 2015/04/14 16:55:19, enne wrote: > https://codereview.chromium.org/1075523002/diff/200001/cc/resources/picture_p... > File cc/resources/picture_pile.cc (right): > > https://codereview.chromium.org/1075523002/diff/200001/cc/resources/picture_p... > cc/resources/picture_pile.cc:201: for (Region::Iterator it(*invalidation); > it.has_rect(); it.next()) > On 2015/04/14 16:02:17, jbroman wrote: > > On 2015/04/13 21:52:27, enne wrote: > > > You need to intersect these invalidations with the interest rect. Both > > > recording sources would handle a billion pixel long page that fully > > invalidated > > > every frame at the same speed as an interest rect-sized page that fully > > > invalidated every frame. > > > > Which rect is the right one to intersect with? I see interest_rect in > > PicturePile, but not in DisplayListRecordingSource, and it's not clear to me > > what the distinction between the interest rect and the recorded viewport > > (apparently the intsersection of the interest rect with the layer bounds?) is. > > > > Can you clarify this for me, please? Thanks for your patience. :) > > Oops, sorry forgot to address this comment. You should intersect with the > recorded_viewport_, which is the interest rect intersected the layer bounds. For the record, here are representative samples for the "hover 'Main page' link in Wikipedia" sanity-check on ToT today, with this latest measurement: No Flags: Record (invalidated area=798) Renderer4.PicturePileRecordUs=952 Renderer4.PicturePileInvalidatedAreaRecordedPerMs=838 Raster (area=65536) Renderer4.RasterTaskUs=410 Renderer4.RasterTaskPixelsPerMs=159843 --enable-slimming-paint: Record (invalidated area=798) Renderer4.DisplayListRecordingSourceRecordUs=2488 Renderer4.DisplayListRecordingSourceInvalidatedAreaRecordedPerMs=320 Raster (area=65536) Renderer4.RasterTaskUs=420 Renderer4.RasterTaskPixelsPerMs=156038 Invalidated area between the two is the same, but the SP record time is longer, due in part to the larger expanded area it winds up using, leading to the 2.6x slowdown.
On 2015/04/14 16:55:19, enne wrote: > https://codereview.chromium.org/1075523002/diff/200001/cc/resources/picture_p... > File cc/resources/picture_pile.cc (right): > > https://codereview.chromium.org/1075523002/diff/200001/cc/resources/picture_p... > cc/resources/picture_pile.cc:201: for (Region::Iterator it(*invalidation); > it.has_rect(); it.next()) > On 2015/04/14 16:02:17, jbroman wrote: > > On 2015/04/13 21:52:27, enne wrote: > > > You need to intersect these invalidations with the interest rect. Both > > > recording sources would handle a billion pixel long page that fully > > invalidated > > > every frame at the same speed as an interest rect-sized page that fully > > > invalidated every frame. > > > > Which rect is the right one to intersect with? I see interest_rect in > > PicturePile, but not in DisplayListRecordingSource, and it's not clear to me > > what the distinction between the interest rect and the recorded viewport > > (apparently the intsersection of the interest rect with the layer bounds?) is. > > > > Can you clarify this for me, please? Thanks for your patience. :) > > Oops, sorry forgot to address this comment. You should intersect with the > recorded_viewport_, which is the interest rect intersected the layer bounds. Please do a quick check to make sure I did this correctly. Thanks.
https://codereview.chromium.org/1075523002/diff/280001/cc/resources/display_l... File cc/resources/display_list_recording_source.cc (right): https://codereview.chromium.org/1075523002/diff/280001/cc/resources/display_l... cc/resources/display_list_recording_source.cc:90: if (!updated && !invalidation->Intersects(recorded_viewport_)) Possibly this check could use recorded_invalidation.IsEmpty() instead; up to you.
https://codereview.chromium.org/1075523002/diff/280001/cc/resources/display_l... File cc/resources/display_list_recording_source.cc (right): https://codereview.chromium.org/1075523002/diff/280001/cc/resources/display_l... cc/resources/display_list_recording_source.cc:90: if (!updated && !invalidation->Intersects(recorded_viewport_)) On 2015/04/14 at 18:18:12, jbroman wrote: > Possibly this check could use recorded_invalidation.IsEmpty() instead; up to you. This is fine, as-is.
The CQ bit was checked by jbroman@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from chrishtr@chromium.org, enne@chromium.org Link to the patchset: https://codereview.chromium.org/1075523002/#ps280001 (title: "intersect with recorded_viewport_")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1075523002/280001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
jbroman@chromium.org changed reviewers: + isherman@chromium.org
+isherman for tools/metrics
https://codereview.chromium.org/1075523002/diff/280001/cc/resources/tile_mana... File cc/resources/tile_manager.cc (right): https://codereview.chromium.org/1075523002/diff/280001/cc/resources/tile_mana... cc/resources/tile_manager.cc:15: #include "base/metrics/histogram_macros.h" nit: Not needed? https://codereview.chromium.org/1075523002/diff/280001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1075523002/diff/280001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:31196: + name="Renderer4.DisplayListRecordingSourceInvalidatedAreaRecordedPerMs"> nit: Please add a units attribute. https://codereview.chromium.org/1075523002/diff/280001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:31196: + name="Renderer4.DisplayListRecordingSourceInvalidatedAreaRecordedPerMs"> Optional nit: Maybe add a dot ('.') to separate "DisplayListRecordingSource" from "InvalidatedAreaRecordedPerMs"? (And, if so, please apply a similar treatment to the other histogram names as well.) https://codereview.chromium.org/1075523002/diff/280001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:31200: + in milliseconds. Please document when this is recorded.
https://codereview.chromium.org/1075523002/diff/280001/cc/resources/tile_mana... File cc/resources/tile_manager.cc (right): https://codereview.chromium.org/1075523002/diff/280001/cc/resources/tile_mana... cc/resources/tile_manager.cc:15: #include "base/metrics/histogram_macros.h" On 2015/04/14 21:31:50, Ilya Sherman wrote: > nit: Not needed? Whoops, thought I'd removed those. Thanks; done. https://codereview.chromium.org/1075523002/diff/280001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1075523002/diff/280001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:31196: + name="Renderer4.DisplayListRecordingSourceInvalidatedAreaRecordedPerMs"> On 2015/04/14 21:31:50, Ilya Sherman wrote: > nit: Please add a units attribute. Done. https://codereview.chromium.org/1075523002/diff/280001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:31196: + name="Renderer4.DisplayListRecordingSourceInvalidatedAreaRecordedPerMs"> On 2015/04/14 21:31:50, Ilya Sherman wrote: > Optional nit: Maybe add a dot ('.') to separate "DisplayListRecordingSource" > from "InvalidatedAreaRecordedPerMs"? (And, if so, please apply a similar > treatment to the other histogram names as well.) Added a dot, made the names more structured, and moved to Compositing since code isn't necessarily restricted to the renderer. WDYT? https://codereview.chromium.org/1075523002/diff/280001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:31200: + in milliseconds. On 2015/04/14 21:31:50, Ilya Sherman wrote: > Please document when this is recorded. Done.
LGTM
The CQ bit was checked by jbroman@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from chrishtr@chromium.org, enne@chromium.org Link to the patchset: https://codereview.chromium.org/1075523002/#ps300001 (title: "isherman comments, esp. renaming of histograms")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1075523002/300001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by jbroman@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from chrishtr@chromium.org, isherman@chromium.org, enne@chromium.org Link to the patchset: https://codereview.chromium.org/1075523002/#ps320001 (title: "git cl format")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1075523002/320001
Message was sent while issue was closed.
Committed patchset #17 (id:320001)
Message was sent while issue was closed.
Patchset 17 (id:??) landed as https://crrev.com/187bc57e8b032fea86a401a7fd7c19378f79da5b Cr-Commit-Position: refs/heads/master@{#325360} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
