fix use of intersect/intersects in DisplayListRasterCacheItem #37238
Conversation
flar
added
perf: speed
|
I'm expecting that the picture_use metrics will increase in some of the benchmarks due to this change. I'm not expecting any golden changes and ideally the rasterization metrics will improve due to increased caching - if they regress then we need to adjust the caching metrics. But, this change at least makes our caching logic more rational so it shouldn't be viewed as the "cause" of any regressions - merely uncovering some cases where we might need to adjust the metrics... |
No other alerts on the change as far as I can see though. |
|
I thumbs-upped the comments that I reviewed for "expectation". Hopefully there was no regression on rendering performance as a result (and one would hope that there was an improvement, otherwise we might want to revisit our caching metrics - though if these increases were in the "worst" category then it may have happened during too few frames to impact the benchmark-long timing numbers). |
|
Hmmm, new_gallery_transition_ios increased the picture count in the average which means it should likely be associated with a benchmark render improvement otherwise those additional cache uses are just a memory-intensive wash... |
|
new_gallery_crane was a 99% change so that probably wouldn't affect the frame timings by much. |
3 participants
This problem was discovered when some of the benchmarks changed under the LayerStateStack mechanism, it looks like the code in DLRCI::PrerollFinalize was mutating the preroll context cull_rect when it was just wanting to check for intersection due to the use of the wrong
SkRectmethod (intersectinstead ofintersects). This meant that we would be less likely to cache other DLLayers inside the same sub-tree. Only a layer that saved and restored the cull_rect such as the clip layers or the transform layer would protect against this issue.This problem will be fixed when the LayerStateStack code eventually goes in as a side effect of relegating the culling to the LSS object, but this PR will introduce the fix earlier so that any benchmarks can be rebased to the new behavior. Then when LSS is finally landed, it should have no incidental impacts on the benchmarks and we can get a clearer picture of how the new mechanism itself affects performance.