Clear image map entry on node disposal or off from the layout [chromium/src : main]
Seokho Song (Gerrit)
Seokho Song added 1 comment![]( "Open in Gerrit")
void Node::DisposeNode() {Seokho SongPlease run this against speedometer3 - this may affect benchmark performance if done with a pre-finalizer.
In Speedometer3, with the exception of `TodoMVC-jQuery`, the other metrics appear unchanged. The `TodoMVC-jQuery` case is roughly 5ms slower (e.g., 182.594ms -> 187.396ms). Please check [the pinpoint results](https://pinpoint-dot-chromeperf.appspot.com/job/1132f46b090000).
However, test cases that handle edge-cases are significantly slower, and the regressions are surprisingly platform-specific:
* Mac: `fast/overflow/lots-of-sibling-inline-boxes.html` regressed from 2.3s to 13s.
* Linux: `external/wpt/html/select/options-length-too-large.html` regressed from 57s to 72s.
I think this could cause serious performance issues.
To address this, I've modified the approach. I changed it to use a prefinalizer for the `NodeImages` class (which is hold from the rare data) to avoid overhead of creating and invoking prefinalizers for every single node.
Related details
- Florin Malita
- Ian Kilpatrick
- Stephen Chenney
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Florin Malita (Gerrit)
Florin Malita added 4 comments![]( "Open in Gerrit")
At a high level, it seems we are adding a lot of complexity to support this feature. Specifically, the spec part that requires us to track data for normal images maybe should be revisited.
Ian's suggestion to use existing animation mechanisms if possible makes a lot of sense. I'm not familiar with the specifics and can't offer actionable advice on that front though.
}What about invalidated weakptrs, should we clean them up at some point?
BindRepeating(I think `BindRepeating` performs some allocations, so I'm kinda wary of doing this on every paint.
Naively, do we need a callback at all? Can't `PaintImageForCurrentFrameWithInfo` execute the steps directly?
base::WeakPtrFactory<Image> weak_factory_{this};`Image`s are `ThreadSafeRefCounted`, which means we can drop them from any thread (background decoding, worker threads, main thread, etc).
`WeakPtr`s OTOH are [thread-bound](https://source.chromium.org/chromium/chromium/src/+/main:base/memory/weak_ptr.h;drc=d1a2bfde021c2bc6500bd2ef11b4e7e786840149;l=50), and expected to be dereferenced and invalidated from a single/stable thread.
IIUC we can't mix them safely.
Related details
- Ian Kilpatrick
- Seokho Song
- Stephen Chenney
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Seokho Song (Gerrit)
Seokho Song added 5 comments![]( "Open in Gerrit")
At a high level, it seems we are adding a lot of complexity to support this feature. Specifically, the spec part that requires us to track data for normal images maybe should be revisited.
Ian's suggestion to use existing animation mechanisms if possible makes a lot of sense. I'm not familiar with the specifics and can't offer actionable advice on that front though.
Integrating this into an existing module (like `core/animations`, perhaps?) sounds good, but it seems like a very large project since image animations are fully managed by the cc layer. Therefore, I think this should be a long-term goal.
The spec part is quite minimal, as we mainly just need to consider what happens if a running or paused image goes back to layout. These behaviors are adopted from other specs like CSS Animations. However, we need to think about whether we can fully adopt their approach. FYI, Ian also filed an issue with the CSSWG: [https://github.com/w3c/csswg-drafts/issues/13745](https://github.com/w3c/csswg-drafts/issues/13745)
What about invalidated weakptrs, should we clean them up at some point?
I think this has been resolve since we don't use WeakPtr. Marking as done
I think `BindRepeating` performs some allocations, so I'm kinda wary of doing this on every paint.
Naively, do we need a callback at all? Can't `PaintImageForCurrentFrameWithInfo` execute the steps directly?
I initially thought we should only register the node once it is added to `image_animation_map_`. Since using `dom/node` inside `platform/graphics` is not allowed, I used a callback approach. However, I realized this is the same as checking for its existence in the node_images collection. Therefore, I removed the callback and handled it in place. PTAL.
base::WeakPtrFactory<Image> weak_factory_{this};`Image`s are `ThreadSafeRefCounted`, which means we can drop them from any thread (background decoding, worker threads, main thread, etc).
`WeakPtr`s OTOH are [thread-bound](https://source.chromium.org/chromium/chromium/src/+/main:base/memory/weak_ptr.h;drc=d1a2bfde021c2bc6500bd2ef11b4e7e786840149;l=50), and expected to be dereferenced and invalidated from a single/stable thread.
IIUC we can't mix them safely.
I considered using `scoped_refptr<>`, but I was worried that holding a strong reference might keep the image alive longer than necessary (e.g., when the image for a node changes). To avoid that, I tried to find a weaker approach.
I think we can use `ImageResourceContent` instead. Since it is garbage-collected, we can safely reference it using a `WeakMember`. PTAL :)
#include "base/notreached.h"Unnecessary include. I think it has been accidentally added..
Related details
- Florin Malita
- Ian Kilpatrick
- Stephen Chenney
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ian Kilpatrick (Gerrit)
Ian Kilpatrick added 2 comments![]( "Open in Gerrit")
CancelImageAnimations();I suspect feature should be more integrated to the existing web animations infrastructure - rather that re-building peicemeal. Can you investigate doing this after this patch?
Have you spoken to Florian about this yet?
void Node::DisposeNode() {Seokho SongPlease run this against speedometer3 - this may affect benchmark performance if done with a pre-finalizer.
In Speedometer3, with the exception of `TodoMVC-jQuery`, the other metrics appear unchanged. The `TodoMVC-jQuery` case is roughly 5ms slower (e.g., 182.594ms -> 187.396ms). Please check [the pinpoint results](https://pinpoint-dot-chromeperf.appspot.com/job/1132f46b090000).
However, test cases that handle edge-cases are significantly slower, and the regressions are surprisingly platform-specific:
* Mac: `fast/overflow/lots-of-sibling-inline-boxes.html` regressed from 2.3s to 13s.
* Linux: `external/wpt/html/select/options-length-too-large.html` regressed from 57s to 72s.
I think this could cause serious performance issues.
To address this, I've modified the approach. I changed it to use a prefinalizer for the `NodeImages` class (which is hold from the rare data) to avoid overhead of creating and invoking prefinalizers for every single node.
For speedometer you need to run 150 iterations to get statistical results.
Related details
- Florin Malita
- Seokho Song
- Stephen Chenney
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
📍 Job linux-perf/speedometer3 complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/1495f49e890000
Related details
- Florin Malita
- Seokho Song
- Stephen Chenney
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Seokho Song (Gerrit)
Seokho Song added 2 comments![]( "Open in Gerrit")
I suspect feature should be more integrated to the existing web animations infrastructure - rather that re-building peicemeal. Can you investigate doing this after this patch?
Have you spoken to Florian about this yet?
This will be discussed different CL: https://chromium-review.googlesource.com/c/chromium/src/+/7727509/comments/6054b44a_bbb17962
void Node::DisposeNode() {Seokho SongPlease run this against speedometer3 - this may affect benchmark performance if done with a pre-finalizer.
Ian KilpatrickIn Speedometer3, with the exception of `TodoMVC-jQuery`, the other metrics appear unchanged. The `TodoMVC-jQuery` case is roughly 5ms slower (e.g., 182.594ms -> 187.396ms). Please check [the pinpoint results](https://pinpoint-dot-chromeperf.appspot.com/job/1132f46b090000).
However, test cases that handle edge-cases are significantly slower, and the regressions are surprisingly platform-specific:
* Mac: `fast/overflow/lots-of-sibling-inline-boxes.html` regressed from 2.3s to 13s.
* Linux: `external/wpt/html/select/options-length-too-large.html` regressed from 57s to 72s.
I think this could cause serious performance issues.
To address this, I've modified the approach. I changed it to use a prefinalizer for the `NodeImages` class (which is hold from the rare data) to avoid overhead of creating and invoking prefinalizers for every single node.
For speedometer you need to run 150 iterations to get statistical results.
- Florin Malita
- Stephen Chenney