[soft navs] Introduce a Pre-Paint-based paint attribution mode [chromium/src : main]
Scott Haseley (Gerrit)
Scott Haseley added 1 comment![]( "Open in Gerrit")
If this is too much, happy to split this up into pre-paint/soft-nav sides. But I think ~50% of this is tests, and half of the remaining is probably comments :)
Related details
- Michal Mocny
- Philip Rogers
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Philip Rogers (Gerrit)
Philip Rogers added 7 comments![]( "Open in Gerrit")
paint tree walk paint. This also might limit which input events we cannit "paint tree walk paint" -> "paint tree walk"?
// document being walked. This will be null for iframes or if theThis will be null for iframes
Can you tell me about this?
There appears to be one SoftNavigationPaintAttributionTracker per local window? That makes me think we should support iframes? Or is it a design choice to not support them at the API level? Do we have issues with the various bits of this patch being set even for objects in iframes? As an example, you can move a node from one document to another with adoptNode.
A bit of info about how this works (this is just background FYI and is not a problem with your patch)... If we have something like the following:
```
<!doctype html>
<span>hello</span>
<iframe srcdoc="<span>world</span>"></iframe>
```
Both spans will paint from the same BeginMainFrame. Similarly, PrePaintTreeWalk will start at the root and will cross frame boundaries. This example has two frames and two LayoutViews.
When we call SetNeedsPaintPropertyUpdate on a LayoutObject, we call SetDescendantNeedsPaintPropertyUpdate on all ancestors but stop at LayoutViews which do not have an ancestor LayoutObject. Then, when we run pre-paint, we first run [this code](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/frame/local_frame_view.cc;drc=46e0a7fb3fd7c4951efd982ae2caf138e20d2a95;l=2610) which propagates the NeedsPaintPropertyUpdate bits from LayoutView in an iframe up to the object in the parent frame.
SoftNavigationContext* UpdateSoftNavigationContextHelper(I like how you've encapsulated so much of the logic in soft_navigation_paint_attribution_tracker so that the PrePaint code is pretty simple. Could we take that even further by inlining these two functions? This function only has some basic logic around GetNode--can that just be moved into UpdateOnPrePaint?
if (auto* node = object.GetNode(); node && object.IsBox()) {Can this be moved in UpdateSoftNavigationContext?
IsImageType(*node->GetLayoutObject()));Anything that can change PrePaint in `InitOrUpdatePaintTrackingForModifiedNodeIfNeeded` or `GetSoftNavigationContextForPropagation` needs to call LayoutObject::MarkSoftNavigationContextChanged.
For example, IsImageType() can change independent of `SoftNavigationHeuristics::ModifiedDOM`, and I think we are missing an invalidation for that. If we insert a node with style="width: 100px; height: 100px;", and then we later change the style to add "background-image: foo.png;", we need to mark the context as having changed (maybe [here](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/layout/layout_object.cc;drc=46e0a7fb3fd7c4951efd982ae2caf138e20d2a95;l=3342))? Can you add a simple test of this?
Similarly, if we change style to display: none (i.e., LayoutObject::WillBeDestroyed), do we need an invalidation? Similarly for removed nodes.
TEST_F(SoftNavigationPaintAttributionTrackerTest, TextAggregation) {Do you think it would be useful to add a dynamic version of this test where the inline is added dynamically, followed by checking the attributable bits? Similarly, for when an ancestor is added that becomes the aggregating parent?
// memory properties, since we're limiting the set of elements we record tomemory properties?
Related details
- Michal Mocny
- Scott Haseley
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Scott Haseley (Gerrit)
Scott Haseley added 7 comments![]( "Open in Gerrit")
Thanks Philip! I'll have to refactor a bit on top of crrev.com/c/6638389 once it lands, but it shouldn't be too bad.
// document being walked. This will be null for iframes or if theThis will be null for iframes
Can you tell me about this?
There appears to be one SoftNavigationPaintAttributionTracker per local window? That makes me think we should support iframes? Or is it a design choice to not support them at the API level? Do we have issues with the various bits of this patch being set even for objects in iframes? As an example, you can move a node from one document to another with adoptNode.
A bit of info about how this works (this is just background FYI and is not a problem with your patch)... If we have something like the following:
```
<!doctype html>
<span>hello</span>
<iframe srcdoc="<span>world</span>"></iframe>
```
Both spans will paint from the same BeginMainFrame. Similarly, PrePaintTreeWalk will start at the root and will cross frame boundaries. This example has two frames and two LayoutViews.When we call SetNeedsPaintPropertyUpdate on a LayoutObject, we call SetDescendantNeedsPaintPropertyUpdate on all ancestors but stop at LayoutViews which do not have an ancestor LayoutObject. Then, when we run pre-paint, we first run [this code](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/frame/local_frame_view.cc;drc=46e0a7fb3fd7c4951efd982ae2caf138e20d2a95;l=2610) which propagates the NeedsPaintPropertyUpdate bits from LayoutView in an iframe up to the object in the parent frame.
@mmo...@chromium.org can probably say more about the design choice, but yes, iframes are ignored; the `SoftNavigationHeuristics` object is only created for main frames: https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/timing/soft_navigation_heuristics.cc;l=209;drc=9ba7fc14c994ebb3a083726360612d50ec1de26f;bpv=1;bpt=1
In iframes:
- The LayoutObject bit for "needing update" will be set to true, and by default it will inherit a null context
- But because we clear the paint attribution tracker, it will just continue to early-out and not really do anything for iframes (same as w/ the feature off)
adoptNode is interesting -- we're not really handling on ToT either. We don't do anything on removal (we wait for GC), so when the node is removed from the DOM, it's still associated with the old tracker (and old window) -- if it's a main frame and attributable to a soft nav. If/when it's appended, it either:
a) From main frame --> iframe, it's won't be tracked in the iframe since there's no `SoftNavigationHeuristics` instance. But it will still be potentially tracked in the old tracker. That won't cause attribution issues (we won't paint it in the old doc), but it does keep it alive.
b) From iframe --> main frame. I think this is really the same as appending any node to the main frame, since it won't have been associated with a tracker previously, so I think this is fine.
We should probably clean up (a), either specifically for adoptNode or in general on removal. I'd like to do that as a follow-up, since this is sprawling already, and it doesn't seem too bad of an issue (unless I've missed something).
---
Re: `LocalFrameView::RunPrePaintLifecyclePhase()` -- I did see that, but chose not to update it since we're not tracking paints in iframes (although we might need to, as I mentioned offline that soft nav tracking doesn't work for Chat). I was going to add a comment to that method that we aren't propagating bits for soft navs, but thought it might be overkill.
SoftNavigationContext* UpdateSoftNavigationContextHelper(I like how you've encapsulated so much of the logic in soft_navigation_paint_attribution_tracker so that the PrePaint code is pretty simple. Could we take that even further by inlining these two functions? This function only has some basic logic around GetNode--can that just be moved into UpdateOnPrePaint?
Yep, done.
if (auto* node = object.GetNode(); node && object.IsBox()) {Can this be moved in UpdateSoftNavigationContext?
Done
IsImageType(*node->GetLayoutObject()));Anything that can change PrePaint in `InitOrUpdatePaintTrackingForModifiedNodeIfNeeded` or `GetSoftNavigationContextForPropagation` needs to call LayoutObject::MarkSoftNavigationContextChanged.
For example, IsImageType() can change independent of `SoftNavigationHeuristics::ModifiedDOM`, and I think we are missing an invalidation for that. If we insert a node with style="width: 100px; height: 100px;", and then we later change the style to add "background-image: foo.png;", we need to mark the context as having changed (maybe [here](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/layout/layout_object.cc;drc=46e0a7fb3fd7c4951efd982ae2caf138e20d2a95;l=3342))? Can you add a simple test of this?
Similarly, if we change style to display: none (i.e., LayoutObject::WillBeDestroyed), do we need an invalidation? Similarly for removed nodes.
ACK. We discussed this offline a bit, and we don't currently handle style changes _at all_ (that's next, likely in M140 or as origin trial progresses). I think the consequence of this now is just decreased accuracy, which we're aware of.
I added a TODO in the header, to the method that should be called to attribute such things. The wiring is here, which I've been exploiting in the unit tests. Any attributed change will call this with `PaintTrackingUpdateMode::kDomModification`, which will handle setting the LayoutObject bit below -- we just don't do that in enough places yet, and as discussed, doing this for style is tricky.
I'll resolve this for now, but re-open / re-comment if there's anything else actionable.
TEST_F(SoftNavigationPaintAttributionTrackerTest, TextAggregation) {Do you think it would be useful to add a dynamic version of this test where the inline is added dynamically, followed by checking the attributable bits? Similarly, for when an ancestor is added that becomes the aggregating parent?
Do you think it would be useful to add a dynamic version of this test where the inline is added dynamically, followed by checking the attributable bits?
Yes, done. That tickled a question of whether we should update the text aggregation node if it has an older context. I think yes, to be consistent.
Similarly, for when an ancestor is added that becomes the aggregating parent?
Do you mean append a child which is an aggregating node, or change ancestry in some way?
// memory properties, since we're limiting the set of elements we record toScott Haseleymemory properties?
As in "memory properties of the implementation". Reworded the comment to hopefully be clearer.
(The gist is, whether or not we store "inline" here doesn't matter for correctness or the interface to paint, but really it ought to be checked since a goal of this is to minimize the memory footprint.)
Related details
- Michal Mocny
- Philip Rogers
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Scott Haseley (Gerrit)
Scott Haseley voted and added 2 comments![]( "Open in Gerrit")
Votes added by Scott Haseley
| Commit-Queue | +1 |
2 comments
Rebased on Michal's latest refactor
paint tree walk paint. This also might limit which input events we cannit "paint tree walk paint" -> "paint tree walk"?
Done
Related details
- Michal Mocny
- Philip Rogers
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Philip Rogers (Gerrit)
Philip Rogers voted and added 4 comments![]( "Open in Gerrit")
Votes added by Philip Rogers
| Code-Review | +1 |
4 comments
LGTM
// document being walked. This will be null for iframes or if theScott HaseleyThis will be null for iframes
Can you tell me about this?
There appears to be one SoftNavigationPaintAttributionTracker per local window? That makes me think we should support iframes? Or is it a design choice to not support them at the API level? Do we have issues with the various bits of this patch being set even for objects in iframes? As an example, you can move a node from one document to another with adoptNode.
A bit of info about how this works (this is just background FYI and is not a problem with your patch)... If we have something like the following:
```
<!doctype html>
<span>hello</span>
<iframe srcdoc="<span>world</span>"></iframe>
```
Both spans will paint from the same BeginMainFrame. Similarly, PrePaintTreeWalk will start at the root and will cross frame boundaries. This example has two frames and two LayoutViews.When we call SetNeedsPaintPropertyUpdate on a LayoutObject, we call SetDescendantNeedsPaintPropertyUpdate on all ancestors but stop at LayoutViews which do not have an ancestor LayoutObject. Then, when we run pre-paint, we first run [this code](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/frame/local_frame_view.cc;drc=46e0a7fb3fd7c4951efd982ae2caf138e20d2a95;l=2610) which propagates the NeedsPaintPropertyUpdate bits from LayoutView in an iframe up to the object in the parent frame.
@mmo...@chromium.org can probably say more about the design choice, but yes, iframes are ignored; the `SoftNavigationHeuristics` object is only created for main frames: https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/timing/soft_navigation_heuristics.cc;l=209;drc=9ba7fc14c994ebb3a083726360612d50ec1de26f;bpv=1;bpt=1
In iframes:
- The LayoutObject bit for "needing update" will be set to true, and by default it will inherit a null context
- But because we clear the paint attribution tracker, it will just continue to early-out and not really do anything for iframes (same as w/ the feature off)adoptNode is interesting -- we're not really handling on ToT either. We don't do anything on removal (we wait for GC), so when the node is removed from the DOM, it's still associated with the old tracker (and old window) -- if it's a main frame and attributable to a soft nav. If/when it's appended, it either:
a) From main frame --> iframe, it's won't be tracked in the iframe since there's no `SoftNavigationHeuristics` instance. But it will still be potentially tracked in the old tracker. That won't cause attribution issues (we won't paint it in the old doc), but it does keep it alive.b) From iframe --> main frame. I think this is really the same as appending any node to the main frame, since it won't have been associated with a tracker previously, so I think this is fine.We should probably clean up (a), either specifically for adoptNode or in general on removal. I'd like to do that as a follow-up, since this is sprawling already, and it doesn't seem too bad of an issue (unless I've missed something).
---
Re: `LocalFrameView::RunPrePaintLifecyclePhase()` -- I did see that, but chose not to update it since we're not tracking paints in iframes (although we might need to, as I mentioned offline that soft nav tracking doesn't work for Chat). I was going to add a comment to that method that we aren't propagating bits for soft navs, but thought it might be overkill.
Acknowledged
// attribution++ needs to be enabled intentionally from the command line (itSupernit: "advanced paint attribution++" is now "PrePaintBasedAttribution"? Or, is that another thing?
TEST_F(SoftNavigationPaintAttributionTrackerTest, TextAggregation) {Scott HaseleyDo you think it would be useful to add a dynamic version of this test where the inline is added dynamically, followed by checking the attributable bits? Similarly, for when an ancestor is added that becomes the aggregating parent?
Do you think it would be useful to add a dynamic version of this test where the inline is added dynamically, followed by checking the attributable bits?
Yes, done. That tickled a question of whether we should update the text aggregation node if it has an older context. I think yes, to be consistent.
Similarly, for when an ancestor is added that becomes the aggregating parent?
Do you mean append a child which is an aggregating node, or change ancestry in some way?
- Michal Mocny
- Scott Haseley
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Scott Haseley (Gerrit)
Scott Haseley added 2 comments![]( "Open in Gerrit")
Thanks Philip! I fixed the comment and this is now rebased again (a couple new conflicts fixed). @mmo...@chromium.org PTAL when you get a chance, thanks!
// attribution++ needs to be enabled intentionally from the command line (itSupernit: "advanced paint attribution++" is now "PrePaintBasedAttribution"? Or, is that another thing?
Good call -- that was what we were calling it before it took shape. Changing to be consistent.
Related details
- Michal Mocny
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michal Mocny (Gerrit)
Michal Mocny added 35 comments![]( "Open in Gerrit")
Apologies. So many of these comments are me just wrapping my head around this-- I will immediately do another pass but wanted to send off first.
1. If the update is not "task attributable", do nothingNit: "the update" --> "a dom update", or something
{"Advanced Paint Attribution (DOM walk)",Nit: These are just names and as long as we understand, I dont think it matters, buuut:
- "Dom walk" is actually something like:
- Lazy to compute
- O(height) "walk" per request
- Uncached.
- "Pre-paint walk" is actually:
- Eager to compute
- O(dom-size) once and O(1) per request, so O(1) amortized
- Cached
So perhaps something like "Lazy-uncached-post-paint-walk" vs "Eager-cached-pre-paint-walk" would give readers more insights.
// If we're locked, mark our descendants as needing this change. This is usedCuriosity about "locked" tree:
- Are we doing this to early-exit to not bother with work that might never be needed, or...
- Is it actually not possible to iterate these descendants?
if (ChildPrePaintBlockedByDisplayLock()) {I *think* you could replace all the lines after 5046 with a call to `LayoutObject::MarkDescendantSoftNavigationContextChanged()`, except that that one checks `ChildPrePaintBlockedByDisplayLock()` *after* setting the bit, and this one checks *before*.
Is this fundamentally necessary to only conditionally set on self if we can unconditionally set on parent?
// When the `SoftNavigationContext` of a node changes on an ancestor, theOn an ancestor -> For the context of this comment, its really just tracking if any context on any part of the dom, rather than a specific nodes' anscestor, right?
Or is the comment right because this only gets set if a previously set context gets changed to a new one?
soft_navigation_text_aggregation_node = nullptr;(thought, for the future)
We have a Context* for soft-navs / interactions.
We probably also want to investigating having a "ContainerRoot" for Container-timing, that could work similar to Context* w.r.t. paint attribution mapping.
...and I wonder if we should consider migrating text_aggregation_node (as well as other container-like content for LCP), to use that mechanism as well.
One difference might be that Context* is exclusive (only one nearest root context per node), while Container-Timing supports overlapping/layering (paints report up through container roots to other container roots).
void PrePaintTreeWalk::InvalidatePaintForHitTesting(Nit: In theory this is a bad name now?
PrePaintTreeWalk::PrePaintTreeWalkContext& context) {(idle thoughts)
"context" is overloaded :P
As we rename SNC to be Interaction-->Paint we might also want to not call it a context. "The Context" is the single value stored by task attribution, inside of which there are multiple "variables". One of those is SNC, but this is just "Interaction State" or "Interaction Paint Detector" or something.
if (auto* node = object.GetNode(); node && object.IsBox()) {Nit: Is it possible to move this to [text_]paint_timing_detector.h to match what happens in `ScopedPaintTimingDetectorBlockPaintHook::EmplaceIfNeeded` or `TextPaintTimingDetector::ShouldWalkObject`
(Or, just add a comment above for why this is restricted as it is)
As discussed, we expect ShouldWalk to depend on this PrePaint walk, so it would be circular to literally call that. But maybe ShouldWalk can be split into a CouldWalk?
if (context.soft_navigation_context_changed) {Can this ever get set outside of the line 298 above? If not, can this merge?
object.GetMutableForPainting().SetShouldInheritSoftNavigationContext(Q: is GetMutableForPainting part of RenderingNG and how we resolved not writing into data structures while reading them in stages of rendering?
context.soft_navigation_context == context_for_node);So, we might have a Node that changes its context value but that value might be the same as we used to have (i.e. previously through inheritance?)
if (already_painted_modified_nodes_.Contains(node)) {This "new mode" won't support repaint, so this old mode doesn't need to either.
I can remove it in a separate patch if that helps. Fine to leave branched here.
// (it has no REF status), pick that first.Just checking if this comment is true?
I thought chrome:flags lists an option for this mode?
Or, by "command line" do you mean any way to locally explicitly opt-in vs finch.
paint_attribution_tracker_->InitializePaintTracking(node, context);Nit: Upon first read I thought `Initialize` meant this happed only once.
Perhaps `AssignNewContainerRoot` or something like that?
return attributable ? context_for_current_url_ : nullptr;If using PrePaint based attribution, I thought we don't need to limit ourselves to just the one url context?
We might still choose not to report side effects, but I think we can start to track them?
// TODO(crbug.com/423670827): `NodePaintState` currently keeps the associatedPreviously we would keep context alive as long scheduling remains, and for one special context we also keep it around forever to track paints.
Now all contexts are alive as long as dom content is alive. Sometimes, this might actually be better-- but yes, it comes with risk
Still, its O(Interactions) which we know from data isn't a huge number (99.9+% are less than 1000)
SoftNavigationContext* GetSoftNavigationContextForPropagation(The comment does a good job explaining, but perhaps rename with `ForPrePaint` specifically?
// all of its ancestors are up-to-date. This is only meant only to be used"only meant only"
// TODO(crbug.com/328783345): This is currently limited to a small set of DOM"this is currently limited" -> "this function currently only gets called by" just to differentiate that nothing after this point makes these decisions.
// updating the mapping during `UpdateOnPrePaint()`.I wonder if this is the right strategy. I wonder if it should just be the most recent context to modify, rather than specifically caring about the age of the context.
Did you think this was more semantically useful for attribution, or was there some reason beyond that (like simplicity of implementation)?
// from the map (note: this allows us to store only nodes where changes occur).Q: do we add nodes that were marked as modified to the set, even when its redundant (i.e. an parent context is the same)? Would that make a different if the shape of dom changed in the future?
// During pre-paint, `UpdateOnPrePaint()` is called for each modified node andNit: "each modified node" --> just checking, we do this walk every prepaint (with caching) or just with each new modification?
(Not sure if the caching implies these are the same thing, but I suspect we might walk when other things change)
// changes).Q: do we have tests for this, in this patch? (tree re-walk after dom structure change)
If you are looking for a good set of tests, the container-timing tests have some really nice coverage for making changes to container-roots and changes to dom structure into/out of roots, etc.
bool IsImageType(const LayoutObject& object) {Similar to the comment about Text, it might be nice if image paint timing detector could supply this primitive.
Maybe something like: `CouldRecordPaint` or `CouldRecordElementPaint`?
SoftNavigationContext* current_context =Nit: `previous_context`
current_context->ContextId() >= modifying_context->ContextId()) {We only call MarkSoftNavigationContextChanged if the context actually changed... but then in `PrePaintTreeWalk::UpdateSoftNavigationContext` if Marked, we also check if it changed...
Is the difference that here we are only certain about the node, and the other is checking also for inheritance?
// Note that this also includes s with background images, which may not"s with" ? Content with?
"node", node->ToString(), "context", modifying_context);There is also `Node::DebugName` and I dont know when to use which.
Coth seem to do a lot of processing to generate the string. I wonder if these should be guarded by tracing_enabled? I think Annie's change might already do some of this and we can make it consistent...
// Note: this overwrites the existing value, resetting both the the textthe the
if (auto* object = node->GetLayoutObject()) {What is an example where this isn't true?
PaintTrackingUpdateMode::kTextAggregation);Could this be made easier if we just pretended that the text_aggregator was being modified when you update a nested node?
---
Ignoring text and thinking forward to the future: another common use case is for `<table>` elements. If a `<td>` is modified, but `<table>` does paint reporting as a single container unit, and then it doesn't matter to us if some other `<td>` is or isn't edited, and it doesn't matter if another context partially updates some of the table...
I think its fine to just consider the whole thing as one unit of modification.
---
(Perhaps we need to still do a dom tree walk up, eagerly, when dom modified for text nodes?)
TRACE_EVENT(TRACE_DISABLED_BY_DEFAULT("loading"),Should this be TRACE_EVENT_INSTANT?
// node or update the cached state if the `parent_context` is newer.Is the whole "if newer" mechanism needed because we cannot guarantee that we "see" dom mods in the order they are applied, for cases where we have a shard pre-paint walk with multiple Context effects?
If so, should we store a modification timestamp (or counter) in the `NodePaintState` to use instead?
(I guess Context_id_ is close enough for now and we can followup to see if there any any cases that suffer)
Related details
- Scott Haseley
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Scott Haseley (Gerrit)
Scott Haseley added 28 comments![]( "Open in Gerrit")
{"Advanced Paint Attribution (DOM walk)",Nit: These are just names and as long as we understand, I dont think it matters, buuut:
- "Dom walk" is actually something like:
- Lazy to compute
- O(height) "walk" per request
- Uncached.
- "Pre-paint walk" is actually:
- Eager to compute
- O(dom-size) once and O(1) per request, so O(1) amortized
- Cached
So perhaps something like "Lazy-uncached-post-paint-walk" vs "Eager-cached-pre-paint-walk" would give readers more insights.
It's not post-paint though (it's actually during paint). Changed to your suggestions, but s/post-paint/paint.
// If we're locked, mark our descendants as needing this change. This is usedCuriosity about "locked" tree:
- Are we doing this to early-exit to not bother with work that might never be needed, or...
- Is it actually not possible to iterate these descendants?
AFAIK the former
if (ChildPrePaintBlockedByDisplayLock()) {I *think* you could replace all the lines after 5046 with a call to `LayoutObject::MarkDescendantSoftNavigationContextChanged()`, except that that one checks `ChildPrePaintBlockedByDisplayLock()` *after* setting the bit, and this one checks *before*.
Is this fundamentally necessary to only conditionally set on self if we can unconditionally set on parent?
This follows exactly what `MarkEffectiveAllowedTouchActionChanged()` and `MarkBlockingWheelEventHandlerChanged()` do, and I don't really want to diverge from that.
I think the idea here is tho not do any unnecessary walking until the element becomes unlocked. Here, we're choosing not to set the descendant bits on ancestors, which will avoid having to walk this subtree (until unlocked).
// When the `SoftNavigationContext` of a node changes on an ancestor, theOn an ancestor -> For the context of this comment, its really just tracking if any context on any part of the dom, rather than a specific nodes' anscestor, right?
Or is the comment right because this only gets set if a previously set context gets changed to a new one?
It's an ancestor. This class (`PrePaintTreeWalkContextBase`) is what get propagated from the root (Document) down, and it forks when we start processing children.
So, initially this is false. When we get to a node that was modified, it gets updated to true, and passed down that way to all descendants (but not unrelated subtrees).
if (auto* node = object.GetNode(); node && object.IsBox()) {Nit: Is it possible to move this to [text_]paint_timing_detector.h to match what happens in `ScopedPaintTimingDetectorBlockPaintHook::EmplaceIfNeeded` or `TextPaintTimingDetector::ShouldWalkObject`
(Or, just add a comment above for why this is restricted as it is)
As discussed, we expect ShouldWalk to depend on this PrePaint walk, so it would be circular to literally call that. But maybe ShouldWalk can be split into a CouldWalk?
Adding a comment and a TODO for now.
if (context.soft_navigation_context_changed) {Can this ever get set outside of the line 298 above? If not, can this merge?
Yes, and that's actually an important detail. This gets passed down to children so that they get updated - but the children might not have the SoftNavigationContextChanged() bit set.
So, that will be true if set in this invocation of UpdateSoftNavigationContext, or by an ancestor. Merging with line 298 would eliminate propagation to descendants.
(Note: when we set the bit on the layout object, it gets propagated UP to the root to say "something below needs to propagate" so we don't early out. This part handles propagating down).
context.soft_navigation_context == context_for_node);So, we might have a Node that changes its context value but that value might be the same as we used to have (i.e. previously through inheritance?)
Yep, that's possible. But we don't know unless we track every node in the paint attribution tracker, so we have to rerun this.
if (already_painted_modified_nodes_.Contains(node)) {This "new mode" won't support repaint, so this old mode doesn't need to either.
I can remove it in a separate patch if that helps. Fine to leave branched here.
SG, let's clean this up as a follow-up.
// (it has no REF status), pick that first.Just checking if this comment is true?
I thought chrome:flags lists an option for this mode?
Or, by "command line" do you mean any way to locally explicitly opt-in vs finch.
Yeah I meant that as a catch all. I'll update to " or about:flags".
paint_attribution_tracker_->InitializePaintTracking(node, context);Nit: Upon first read I thought `Initialize` meant this happed only once.
Perhaps `AssignNewContainerRoot` or something like that?
Hmm, I'd push back on `AssignNewContainerRoot` because it might not be a container root, e.g. changing image src, where image is a leaf (and we might need to also handle text nodes to fix appending text to body).
What about `InitializeOrUpdatePaintTracking()` ? (changed to that, can change again if you disagree).
return attributable ? context_for_current_url_ : nullptr;If using PrePaint based attribution, I thought we don't need to limit ourselves to just the one url context?
We might still choose not to report side effects, but I think we can start to track them?
Yes, but I'd rather not do that in this patch. (apples to apples for tests/etc., then apples to oranges to see the delta, or something like that).
// TODO(crbug.com/423670827): `NodePaintState` currently keeps the associatedPreviously we would keep context alive as long scheduling remains, and for one special context we also keep it around forever to track paints.
Now all contexts are alive as long as dom content is alive. Sometimes, this might actually be better-- but yes, it comes with risk
Still, its O(Interactions) which we know from data isn't a huge number (99.9+% are less than 1000)
Thanks for the numbers! It's probably fine to start with, but going to think through lifetime, of this and more, especially once we better support multiple interactions.
SoftNavigationContext* GetSoftNavigationContextForPropagation(The comment does a good job explaining, but perhaps rename with `ForPrePaint` specifically?
Funny, I had it that way but changed it. Changed back.
// all of its ancestors are up-to-date. This is only meant only to be usedScott Haseley"only meant only"
Done
// TODO(crbug.com/328783345): This is currently limited to a small set of DOM"this is currently limited" -> "this function currently only gets called by" just to differentiate that nothing after this point makes these decisions.
Done
// updating the mapping during `UpdateOnPrePaint()`.I wonder if this is the right strategy. I wonder if it should just be the most recent context to modify, rather than specifically caring about the age of the context.
Did you think this was more semantically useful for attribution, or was there some reason beyond that (like simplicity of implementation)?
I thought we had said "newest context" in offline discussions, so I went with it. We can certainly change this (either now or in a follow-up).
// from the map (note: this allows us to store only nodes where changes occur).Q: do we add nodes that were marked as modified to the set, even when its redundant (i.e. an parent context is the same)? Would that make a different if the shape of dom changed in the future?
Q: do we add nodes that were marked as modified to the set, even when its redundant (i.e. an parent context is the same)?
Yes. We could potentially optimize this by trimming the set, but I didn't bother making it more complicated.
Would that make a different if the shape of dom changed in the future?
Absent structural changes, I think storing any subset of the full tree should "just work". Do you have examples of what changes you mean? One thing I'd like to follow up with is clearing on node removal or layout detach.
// During pre-paint, `UpdateOnPrePaint()` is called for each modified node andNit: "each modified node" --> just checking, we do this walk every prepaint (with caching) or just with each new modification?
(Not sure if the caching implies these are the same thing, but I suspect we might walk when other things change)
It's true we still walk if other things change, but in that case we don't do an update. If nothing has changed, (we didn't set the bit on any layout objects), then the prepaint context's soft_navigation_context_changed path will be false, and we'll either inherit the parent's context (initially null) or call GetSoftNavigationContextForPropagation().
// changes).Q: do we have tests for this, in this patch? (tree re-walk after dom structure change)
If you are looking for a good set of tests, the container-timing tests have some really nice coverage for making changes to container-roots and changes to dom structure into/out of roots, etc.
Yes. Most (maybe all) of our WPTs, as well as a couple unit tests where we append to the DOM.
(There is no layout object until layout runs, so most appends hit this path).
bool IsImageType(const LayoutObject& object) {Similar to the comment about Text, it might be nice if image paint timing detector could supply this primitive.
Maybe something like: `CouldRecordPaint` or `CouldRecordElementPaint`?
Added a TODO.
SoftNavigationContext* current_context =Nit: `previous_context`
I'm reading this as the context we're currently tracking in this class (not the task context). It might become the previous context, depending on what happens next. So, slight preference for keeping it, but if you have a stronger preference to change it, will do.
// Note that this also includes s with background images, which may not"s with" ? Content with?
Fixed.
"node", node->ToString(), "context", modifying_context);There is also `Node::DebugName` and I dont know when to use which.
Coth seem to do a lot of processing to generate the string. I wonder if these should be guarded by tracing_enabled? I think Annie's change might already do some of this and we can make it consistent...
Ah right, I added this before her change.. I found ToString to be helpful when I was debugging, but don't remember if it was more helpful. I'll change this to DebugName to be consistent, but also going to leave this disabled-by-default unless we find it's needed for the trace viewer.
// Note: this overwrites the existing value, resetting both the the textScott Haseleythe the
Whoops.
if (auto* object = node->GetLayoutObject()) {What is an example where this isn't true?
Appending a node. The node's LayoutObject is null until layout runs, which might not be until the next BeginMainFrame.
PaintTrackingUpdateMode::kTextAggregation);Could this be made easier if we just pretended that the text_aggregator was being modified when you update a nested node?
---
Ignoring text and thinking forward to the future: another common use case is for `<table>` elements. If a `<td>` is modified, but `<table>` does paint reporting as a single container unit, and then it doesn't matter to us if some other `<td>` is or isn't edited, and it doesn't matter if another context partially updates some of the table...
I think its fine to just consider the whole thing as one unit of modification.
---
(Perhaps we need to still do a dom tree walk up, eagerly, when dom modified for text nodes?)
Not just up, but down too. Suppose you append/modify "target":
```
<div id=target>
<div>
<div>
<div id=content>Text goes here</div>
</div>
</div>
</div>
```
The text paint timing detector will notify us about "content", so in ModifiedDOM we'd need to walk _down_ and track "content". That's what we're doing here, but leveraging the existing walk, etc.
Alternatively, we could change the aggregator / TPTD and somehow track individual text nodes, but this seemed like a better starting point.
---
We could also keep a separate set of text aggregation nodes, which might make this a little cleaner, and would allow us to store make the set of nodes fully weak (weak key, weak value) -- but there's some complexity in maintaining both / and moving things between, so I started here.
TRACE_EVENT(TRACE_DISABLED_BY_DEFAULT("loading"),Scott HaseleyShould this be TRACE_EVENT_INSTANT?
I liked this as a regular TRACE_EVENT to see the instant ones in InitOrUpdatePaintTrackingForModifiedNodeIfNeeded get nested underneath it.
// node or update the cached state if the `parent_context` is newer.Is the whole "if newer" mechanism needed because we cannot guarantee that we "see" dom mods in the order they are applied, for cases where we have a shard pre-paint walk with multiple Context effects?
If so, should we store a modification timestamp (or counter) in the `NodePaintState` to use instead?
(I guess Context_id_ is close enough for now and we can followup to see if there any any cases that suffer)
Mostly I thought this was the direction you mentioned wanting to go in. We should discuss :). I think it would be fairly straightforward to change it to be "last touched", i.e. I think this more policy than technical limitation.
Related details
- Michal Mocny
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Scott Haseley (Gerrit)
Scott Haseley voted and added 1 comment![]( "Open in Gerrit")
Votes added by Scott Haseley
| Commit-Queue | +1 |
1 comment
1. If the update is not "task attributable", do nothingScott HaseleyNit: "the update" --> "a dom update", or something
Done
Related details
- Michal Mocny
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Johannes Henkel (Gerrit)
Johannes Henkel added 1 comment![]( "Open in Gerrit")
++num_modified_dom_nodes_;
TRACE_EVENT_INSTANT(
"loading", "SoftNavigationContext::AddedModifiedNodeInAnimationFrame",
perfetto::Track::FromPointer(this), "context", this, "nodeId",
node->GetDomNodeId(), "nodeDebugName", node->DebugName(),
"domModificationsThisAnimationFrame",
num_modified_dom_nodes_ - num_modified_dom_nodes_last_animation_frame_);Is this equivalent? (not saying it's nicer - just trying to read a bit)
```
if (paint_attribution_mode_ ==
features::SoftNavigationHeuristicsMode::kPrePaintBasedAttribution ||
modified_nodes_.insert(node).is_new_entry) {
++num_modified_dom_nodes_;
TRACE...
}
```
Related details
- Michal Mocny
- Scott Haseley
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Scott Haseley (Gerrit)
Scott Haseley added 1 comment![]( "Open in Gerrit")
++num_modified_dom_nodes_;
TRACE_EVENT_INSTANT(
"loading", "SoftNavigationContext::AddedModifiedNodeInAnimationFrame",
perfetto::Track::FromPointer(this), "context", this, "nodeId",
node->GetDomNodeId(), "nodeDebugName", node->DebugName(),
"domModificationsThisAnimationFrame",
num_modified_dom_nodes_ - num_modified_dom_nodes_last_animation_frame_);Is this equivalent? (not saying it's nicer - just trying to read a bit)
```
if (paint_attribution_mode_ ==
features::SoftNavigationHeuristicsMode::kPrePaintBasedAttribution ||
modified_nodes_.insert(node).is_new_entry) {++num_modified_dom_nodes_;
TRACE...
}
```
Yep, that works too (or invert it and return early).
FWIW I typically isolate flag-specific code as much as possible, for minimal diffs, clearer separation, and to make cleanup easy.
Related details
- Johannes Henkel
- Michal Mocny
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michal Mocny (Gerrit)
Michal Mocny added 16 comments![]( "Open in Gerrit")
{"Advanced Paint Attribution (DOM walk)",Nit: These are just names and as long as we understand, I dont think it matters, buuut:
- "Dom walk" is actually something like:
- Lazy to compute
- O(height) "walk" per request
- Uncached.
- "Pre-paint walk" is actually:
- Eager to compute
- O(dom-size) once and O(1) per request, so O(1) amortized
- Cached
So perhaps something like "Lazy-uncached-post-paint-walk" vs "Eager-cached-pre-paint-walk" would give readers more insights.
It's not post-paint though (it's actually during paint). Changed to your suggestions, but s/post-paint/paint.
Heh, yeah I meant "after image/text is painted, then recorded, we will walk the tree (but all inside the paint stage)" but didn't want top write that :P
if (ChildPrePaintBlockedByDisplayLock()) {Scott HaseleyI *think* you could replace all the lines after 5046 with a call to `LayoutObject::MarkDescendantSoftNavigationContextChanged()`, except that that one checks `ChildPrePaintBlockedByDisplayLock()` *after* setting the bit, and this one checks *before*.
Is this fundamentally necessary to only conditionally set on self if we can unconditionally set on parent?
This follows exactly what `MarkEffectiveAllowedTouchActionChanged()` and `MarkBlockingWheelEventHandlerChanged()` do, and I don't really want to diverge from that.
I think the idea here is tho not do any unnecessary walking until the element becomes unlocked. Here, we're choosing not to set the descendant bits on ancestors, which will avoid having to walk this subtree (until unlocked).
Acknowledged
// When the `SoftNavigationContext` of a node changes on an ancestor, theScott HaseleyOn an ancestor -> For the context of this comment, its really just tracking if any context on any part of the dom, rather than a specific nodes' anscestor, right?
Or is the comment right because this only gets set if a previously set context gets changed to a new one?
It's an ancestor. This class (`PrePaintTreeWalkContextBase`) is what get propagated from the root (Document) down, and it forks when we start processing children.
So, initially this is false. When we get to a node that was modified, it gets updated to true, and passed down that way to all descendants (but not unrelated subtrees).
Acknowledged
if (context.soft_navigation_context_changed) {Scott HaseleyCan this ever get set outside of the line 298 above? If not, can this merge?
Yes, and that's actually an important detail. This gets passed down to children so that they get updated - but the children might not have the SoftNavigationContextChanged() bit set.
So, that will be true if set in this invocation of UpdateSoftNavigationContext, or by an ancestor. Merging with line 298 would eliminate propagation to descendants.
(Note: when we set the bit on the layout object, it gets propagated UP to the root to say "something below needs to propagate" so we don't early out. This part handles propagating down).
Thank you!
context.soft_navigation_context == context_for_node);Scott HaseleySo, we might have a Node that changes its context value but that value might be the same as we used to have (i.e. previously through inheritance?)
Yep, that's possible. But we don't know unless we track every node in the paint attribution tracker, so we have to rerun this.
Acknowledged
if (already_painted_modified_nodes_.Contains(node)) {Scott HaseleyThis "new mode" won't support repaint, so this old mode doesn't need to either.
I can remove it in a separate patch if that helps. Fine to leave branched here.
SG, let's clean this up as a follow-up.
I'll get started.
paint_attribution_tracker_->InitializePaintTracking(node, context);Nit: Upon first read I thought `Initialize` meant this happed only once.
Perhaps `AssignNewContainerRoot` or something like that?
Hmm, I'd push back on `AssignNewContainerRoot` because it might not be a container root, e.g. changing image src, where image is a leaf (and we might need to also handle text nodes to fix appending text to body).
What about `InitializeOrUpdatePaintTracking()` ? (changed to that, can change again if you disagree).
Meh... If you change src= I would think that "conceptually" the image becomes a new root, too?
Like, although images are expected (guaranteed?) to be leaf nodes, if you *could* add children to them, those paints would still also trickle up and be grouped. Afaik we don't special case leaf nodes, anyway (do we?)
---
How about `EnsureContainerPaintAttributionToContext` which seems to match the intent well?
return attributable ? context_for_current_url_ : nullptr;Scott HaseleyIf using PrePaint based attribution, I thought we don't need to limit ourselves to just the one url context?
We might still choose not to report side effects, but I think we can start to track them?
Yes, but I'd rather not do that in this patch. (apples to apples for tests/etc., then apples to oranges to see the delta, or something like that).
Good idea!
// TODO(crbug.com/423670827): `NodePaintState` currently keeps the associatedScott HaseleyPreviously we would keep context alive as long scheduling remains, and for one special context we also keep it around forever to track paints.
Now all contexts are alive as long as dom content is alive. Sometimes, this might actually be better-- but yes, it comes with risk
Still, its O(Interactions) which we know from data isn't a huge number (99.9+% are less than 1000)
Thanks for the numbers! It's probably fine to start with, but going to think through lifetime, of this and more, especially once we better support multiple interactions.
Acknowledged
// updating the mapping during `UpdateOnPrePaint()`.Scott HaseleyI wonder if this is the right strategy. I wonder if it should just be the most recent context to modify, rather than specifically caring about the age of the context.
Did you think this was more semantically useful for attribution, or was there some reason beyond that (like simplicity of implementation)?
I thought we had said "newest context" in offline discussions, so I went with it. We can certainly change this (either now or in a follow-up).
Ah sorry-- what I was trying to say was 'the latest to update' rather than the newest to get created.
---
I don't think there is any specific evidence which way is best, I'm just basing on intuition that most-recent-change matters most?
For simple things like updating src= or .innerText/.innerHTML it seems to me that the most-recent-to-apply-update should get attribution.
But for things like the whole <body> being marked modified because some attribute changed... and where multiple contexts are collectively making changes... and its not clear which actual paint updates are triggered... now I can see that we might want to use heuristics like age to differentiate.
But I would start without that?
// from the map (note: this allows us to store only nodes where changes occur).Scott HaseleyQ: do we add nodes that were marked as modified to the set, even when its redundant (i.e. an parent context is the same)? Would that make a different if the shape of dom changed in the future?
Q: do we add nodes that were marked as modified to the set, even when its redundant (i.e. an parent context is the same)?
Yes. We could potentially optimize this by trimming the set, but I didn't bother making it more complicated.
Would that make a different if the shape of dom changed in the future?
Absent structural changes, I think storing any subset of the full tree should "just work". Do you have examples of what changes you mean? One thing I'd like to follow up with is clearing on node removal or layout detach.
I like these choices.
---
The example I was thinking of come from the container timing test cases, where you have things like:
```
<div containertiming>
<div id=a>
<div id=b containertiming>
<div>
```
And then if you *moveBefore()* nodes around the dom, such that `id=b` moves out of the `id=a` tree.
Then, suddenly `id=a` doesn't have any descendants w/ CT and `id=b` doesn't have any ancestors with CT, and any cached values that rely on those relationships are re-computed.
SoftNavigationContext* current_context =Nit: `previous_context`
I'm reading this as the context we're currently tracking in this class (not the task context). It might become the previous context, depending on what happens next. So, slight preference for keeping it, but if you have a stronger preference to change it, will do.
Acknowledged
if (auto* object = node->GetLayoutObject()) {Scott HaseleyWhat is an example where this isn't true?
Appending a node. The node's LayoutObject is null until layout runs, which might not be until the next BeginMainFrame.
Acknowledged
PaintTrackingUpdateMode::kTextAggregation);Scott HaseleyCould this be made easier if we just pretended that the text_aggregator was being modified when you update a nested node?
---
Ignoring text and thinking forward to the future: another common use case is for `<table>` elements. If a `<td>` is modified, but `<table>` does paint reporting as a single container unit, and then it doesn't matter to us if some other `<td>` is or isn't edited, and it doesn't matter if another context partially updates some of the table...
I think its fine to just consider the whole thing as one unit of modification.
---
(Perhaps we need to still do a dom tree walk up, eagerly, when dom modified for text nodes?)
Not just up, but down too. Suppose you append/modify "target":
```
<div id=target>
<div>
<div>
<div id=content>Text goes here</div>
</div>
</div>
</div>
```The text paint timing detector will notify us about "content", so in ModifiedDOM we'd need to walk _down_ and track "content". That's what we're doing here, but leveraging the existing walk, etc.
Alternatively, we could change the aggregator / TPTD and somehow track individual text nodes, but this seemed like a better starting point.
---
We could also keep a separate set of text aggregation nodes, which might make this a little cleaner, and would allow us to store make the set of nodes fully weak (weak key, weak value) -- but there's some complexity in maintaining both / and moving things between, so I started here.
Hmm. I would have thought that if target was directly modified that we would only mark it in the modified set.
Later in PrePaint it would "trickle down" to all nodes including id=content. Later when we receive a paint for content, it would already have the right context.
And we wouldn't even need a text_aggregator concept for that part. Is that wrong?
---
OTOH if a Node inside id=content gets modified, we need to somehow remember that its id=content parent is what is really going to get the attribution. Which is why I thought we could just change which Node gets marked...
---
TPTD kinda already tracks individual nodes, then throws that work out. We inherited the aggregation stuff for soft-navs but one of my earlier patches did just report every individual node separately. We want the aggregation for LCP candidate reasons, but we don't need it for paint attribution. It could be an option to re-architect the order (ideally later :P).
TRACE_EVENT(TRACE_DISABLED_BY_DEFAULT("loading"),Scott HaseleyShould this be TRACE_EVENT_INSTANT?
I liked this as a regular TRACE_EVENT to see the instant ones in InitOrUpdatePaintTrackingForModifiedNodeIfNeeded get nested underneath it.
Ack.
// node or update the cached state if the `parent_context` is newer.Scott HaseleyIs the whole "if newer" mechanism needed because we cannot guarantee that we "see" dom mods in the order they are applied, for cases where we have a shard pre-paint walk with multiple Context effects?
If so, should we store a modification timestamp (or counter) in the `NodePaintState` to use instead?
(I guess Context_id_ is close enough for now and we can followup to see if there any any cases that suffer)
Mostly I thought this was the direction you mentioned wanting to go in. We should discuss :). I think it would be fairly straightforward to change it to be "last touched", i.e. I think this more policy than technical limitation.
- Johannes Henkel
- Scott Haseley
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Scott Haseley (Gerrit)
Scott Haseley added 1 comment![]( "Open in Gerrit")
Later in PrePaint it would "trickle down" to all nodes including id=content. Later when we receive a paint for content, it would already have the right context.
And we wouldn't even need a text_aggregator concept for that part. Is that wrong?
This is for memory savings: rather than store a mapping for every node, only store ones that matter. So yes, if we stored every node, then we don't need that concept, but I wanted to keep things sparse.
In this example, we only store entries for target and content, but not the intermediate divs.
Related details
- Johannes Henkel
- Michal Mocny
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michal Mocny (Gerrit)
Michal Mocny added 1 comment![]( "Open in Gerrit")
Of course-- I'm asking: why are we needing to store for id=content at all?
Related details
- Johannes Henkel
- Scott Haseley
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michal Mocny (Gerrit)
Oh, are you not referring to the `modified_nodes_` set but rather the Context* in PrePaintTreeWalkContextBase?
Scott Haseley (Gerrit)
Scott Haseley added 1 comment![]( "Open in Gerrit")
PrePaintTreeWalkContextBase only lives as long as the tree walk happens (it's basically stack state, that changes for each child). We store the SNC there temporarily to build up the `modified_nodes_` set. In `modified_nodes_`, we only store (a) Nodes explicitly tracked by ModifiedDOM, and (b) a subset of Nodes, which we expect to be/might be passed to AddPaintedArea (images and text aggregation nodes), computed during pre-paint.
We store an entry for id=content because when the "Text goes here" is painted (a text node), the TPTD aggregates that into id=content. And when AddPaintedArea is called, it's called with id=content.
Note: the only place we can look up during paint is in `modified_nodes_`.
HTH and that I'm understanding the question. Let's maybe talk through it in the next eng sync?
Related details
- Johannes Henkel
- Michal Mocny
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michal Mocny (Gerrit)
Michal Mocny added 1 comment![]( "Open in Gerrit")
ty ty, and also for the explanation in person.
Do you think it could be useful to separate `modified_nodes_` from `potential_paint_records_` or something, given that this second mechanism might have room for alternatives?
Related details
- Johannes Henkel
- Scott Haseley
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Scott Haseley (Gerrit)
Scott Haseley added 1 comment![]( "Open in Gerrit")
Quoting myself from upthread:
We could also keep a separate set of text aggregation nodes, which might make this a little cleaner, and would allow us to store make the set of nodes fully weak (weak key, weak value) -- but there's some complexity in maintaining both / and moving things between, so I started here.
Doing so complicates other parts of there's no reason a modified node can't also be a potential paint record, so we'd either need to move things between sets (and query both) or add memory overhead by storing nodes in both places. I have a preference not to change this now, but leave open the possibility to change it in the future.
Related details
- Johannes Henkel
- Michal Mocny
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michal Mocny (Gerrit)
Michal Mocny added 1 comment![]( "Open in Gerrit")
Huzzah!
Suggest perhaps renaming `modified_nodes_`, for now? My existing mental model misled me...
Perhaps: `marked_nodes_` ?
---
Also, our ability to map Node->Context now also depends on our ability to predict the types of nodes that paint timing detectors might report. I know we discussed synchronizing this in the future, but one option now is to do a dom tree walk up whenever nodes fail to have a mapping in `marked_nodes_`. Just to confirm there isn't a (grand)parent after all. (this would be a mix of both approaches, together). Might just do this via DCHECK?
Related details
- Johannes Henkel
- Scott Haseley
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michal Mocny (Gerrit)
Michal Mocny added 8 comments![]( "Open in Gerrit")
I (think I) better understand how this all works.
This mostly looks good to me, a few comments are mostly not of substance.
One part I don't love is the shared Internal helper for marking but I think it works. Will wait for next patchset for final +1.
...also, potentially rename all of:
- `InitializeOrUpdatePaintTracking` --> `MarkNodeAsDirectlyModified`
- `InitOrUpdatePaintTrackingForModifiedNodeIfNeeded` --> `MarkNodeForPaintTrackingIfNeeded`
- `PaintTrackingUpdateMode` --> `MarkReason`
EXPECT_TRUE(Tracker()->IsAttributable(GetElement("content"), context));Nice tests!
EXPECT_TRUE(Tracker()->IsAttributable(content_node, context));What is the expectation for `Tracker()->IsAttributable(element, context)`?
The test for `<body>` implies that it would be in the modified set, even though it is never painted.
AppendInlineTextOverwritePreviousContext) {Nit: `AppendInlineTextOverwritePreviousContextOfAggregationNode` since thats the most important detail this is testing, I think.
// TODO(crbug.com/423670827): For now, appending to an attributed node withoutThe bug doesn't mention this detail (or much detail). Not blocking patch but if we have followups that are leftovers let's document.
element->setInnerText(" And this is bold text.");If you created yet another context and updated inner text again (after the first one is set), would it update to a new context? The comments imply that its a bug that we only assign context when going from none -> some on first text?
I know we wont currently observe paint for nodes but I think these tests don't need that to work.
Tracker()->GetSoftNavigationContextForPrePaint(GetDocument().body()),And to confirm I get this now-- this means when we are going top-down during prepaint we won't apply this context to any paintable-nodes, but if this node happens to paint, it will get attributed.
Cool.
Michal Mocny (Gerrit)
Michal Mocny added 3 comments![]( "Open in Gerrit")
I (think I) better understand how this all works.
This mostly looks good to me, a few comments are mostly not of substance.
One part I don't love is the shared Internal helper for marking but I think it works. Will wait for next patchset for final +1.
I wanted to make the last line of this comment more actionable, so here's my attempt:
---
It seems to me that we have two similar but distinct jobs:
- Visit the node during pre-paint and check if any of the following apply:
- Had a context, and context changed
- Is image
- Is text (and find the aggregation node)
If any of those apply, or also when you have a new explicit DomMod, then you will:
- Mark the node with a specific Context and Reason.
bool is_text_aggregation_only_ = false;Do you think this could be `MarkForPaintRecordingOnly()` and also set it for Images?
Or ,even flip the meaning to `MarkExplicitlyModified()` and only set it when its an explicit root?
---
I think the main use case for this flag is for `GetSoftNavigationContextForPrePaint` and so we don't accidentally inherit contexts that are only "marked" for Paint recording purposes.
Today this probably only matters for text because we "mark up" and dont want to "mark down" to siblings... while Images don't mark up and shouldn't have children to walk anyway, so it doesn't matter.
For background-images its trickier, these could be part of a container tree, but they should always have the same context that they inherited from the root, anyway... until we start to modify dom and move things around?
---
Either way, we discussed segmenting modified vs marked nodes, but this would be a way to keep a single set and still have a way to differentiate.
kPrePaintWalk,One more small suggestion that popped into shower thoughts: do you think this could be something like `kInheritFromContainer`
We call this during pre-paint but thats just a mechanism, the Reason for being marked is that it has a parent to inherit from.
Scott Haseley (Gerrit)
Scott Haseley added 3 comments![]( "Open in Gerrit")
Michal MocnyI (think I) better understand how this all works.
This mostly looks good to me, a few comments are mostly not of substance.
One part I don't love is the shared Internal helper for marking but I think it works. Will wait for next patchset for final +1.
I wanted to make the last line of this comment more actionable, so here's my attempt:
---
It seems to me that we have two similar but distinct jobs:
- Visit the node during pre-paint and check if any of the following apply:
- Had a context, and context changed
- Is image
- Is text (and find the aggregation node)
If any of those apply, or also when you have a new explicit DomMod, then you will:
- Mark the node with a specific Context and Reason.
I'm actually breaking that internal helper up in order to implement the "most recent modification" bit (which requires quite a few changes), let's revisit.
bool is_text_aggregation_only_ = false;Do you think this could be `MarkForPaintRecordingOnly()` and also set it for Images?
Or ,even flip the meaning to `MarkExplicitlyModified()` and only set it when its an explicit root?
---
I think the main use case for this flag is for `GetSoftNavigationContextForPrePaint` and so we don't accidentally inherit contexts that are only "marked" for Paint recording purposes.
Today this probably only matters for text because we "mark up" and dont want to "mark down" to siblings... while Images don't mark up and shouldn't have children to walk anyway, so it doesn't matter.
For background-images its trickier, these could be part of a container tree, but they should always have the same context that they inherited from the root, anyway... until we start to modify dom and move things around?
---
Either way, we discussed segmenting modified vs marked nodes, but this would be a way to keep a single set and still have a way to differentiate.
Planning to keep a single set but break up the methods.
kPrePaintWalk,One more small suggestion that popped into shower thoughts: do you think this could be something like `kInheritFromContainer`
We call this during pre-paint but thats just a mechanism, the Reason for being marked is that it has a parent to inherit from.
- Johannes Henkel
- Michal Mocny
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Scott Haseley (Gerrit)
Scott Haseley voted and added 13 comments![]( "Open in Gerrit")
Votes added by Scott Haseley
| Commit-Queue | +1 |
Michal MocnyI (think I) better understand how this all works.
This mostly looks good to me, a few comments are mostly not of substance.
One part I don't love is the shared Internal helper for marking but I think it works. Will wait for next patchset for final +1.
Scott HaseleyI wanted to make the last line of this comment more actionable, so here's my attempt:
---
It seems to me that we have two similar but distinct jobs:
- Visit the node during pre-paint and check if any of the following apply:
- Had a context, and context changed
- Is image
- Is text (and find the aggregation node)
If any of those apply, or also when you have a new explicit DomMod, then you will:
- Mark the node with a specific Context and Reason.
I'm actually breaking that internal helper up in order to implement the "most recent modification" bit (which requires quite a few changes), let's revisit.
This is quite different now, so going to resolve this in favor of new comments.
@mmo...@chromium.org: This is ready for another pass. I've updated things to use most recent modification:
- Store a "modification id" to determine age (see the code comments -- this is incremented when the context calling ModifiedDOM is different from the call, to group mods (which helps prune the set of marked nodes).
- Pass down the Node rather than SNC in the pre-paint tree walk, so we can look up the state (so we don't need to leak more details out of the new class)
- Prune stale/redundant nodes during pre-paint. Rather than overwrite a node if we find a stale mapping, just remove it. This works for redundant nodes we're tracking too, e.g. if we track all nodes in a subtree, we only really need the container root (see comments and new test).
Scott HaseleyDo you think this could be `MarkForPaintRecordingOnly()` and also set it for Images?
Or ,even flip the meaning to `MarkExplicitlyModified()` and only set it when its an explicit root?
---
I think the main use case for this flag is for `GetSoftNavigationContextForPrePaint` and so we don't accidentally inherit contexts that are only "marked" for Paint recording purposes.
Today this probably only matters for text because we "mark up" and dont want to "mark down" to siblings... while Images don't mark up and shouldn't have children to walk anyway, so it doesn't matter.
For background-images its trickier, these could be part of a container tree, but they should always have the same context that they inherited from the root, anyway... until we start to modify dom and move things around?
---
Either way, we discussed segmenting modified vs marked nodes, but this would be a way to keep a single set and still have a way to differentiate.
Planning to keep a single set but break up the methods.
Or ,even flip the meaning to MarkExplicitlyModified() and only set it when its an explicit root?
I ended up independently landing on this while I was changing things around to support most recent modification.
Scott HaseleyOne more small suggestion that popped into shower thoughts: do you think this could be something like `kInheritFromContainer`
We call this during pre-paint but thats just a mechanism, the Reason for being marked is that it has a parent to inherit from.
This enum is going away.
Obsolete.
// updating the mapping during `UpdateOnPrePaint()`.I wonder if this is the right strategy. I wonder if it should just be the most recent context to modify, rather than specifically caring about the age of the context.
Did you think this was more semantically useful for attribution, or was there some reason beyond that (like simplicity of implementation)?
Michal MocnyI thought we had said "newest context" in offline discussions, so I went with it. We can certainly change this (either now or in a follow-up).
Ah sorry-- what I was trying to say was 'the latest to update' rather than the newest to get created.
---
I don't think there is any specific evidence which way is best, I'm just basing on intuition that most-recent-change matters most?
For simple things like updating src= or .innerText/.innerHTML it seems to me that the most-recent-to-apply-update should get attribution.
But for things like the whole <body> being marked modified because some attribute changed... and where multiple contexts are collectively making changes... and its not clear which actual paint updates are triggered... now I can see that we might want to use heuristics like age to differentiate.
But I would start without that?
Changed to most recent update.
// During pre-paint, `UpdateOnPrePaint()` is called for each modified node andNit: "each modified node" --> just checking, we do this walk every prepaint (with caching) or just with each new modification?
(Not sure if the caching implies these are the same thing, but I suspect we might walk when other things change)
It's true we still walk if other things change, but in that case we don't do an update. If nothing has changed, (we didn't set the bit on any layout objects), then the prepaint context's soft_navigation_context_changed path will be false, and we'll either inherit the parent's context (initially null) or call GetSoftNavigationContextForPropagation().
Marked as resolved.
current_context->ContextId() >= modifying_context->ContextId()) {We only call MarkSoftNavigationContextChanged if the context actually changed... but then in `PrePaintTreeWalk::UpdateSoftNavigationContext` if Marked, we also check if it changed...
Is the difference that here we are only certain about the node, and the other is checking also for inheritance?
I don't quite follow the "if Marked" part.
Calling `MarkSoftNavigationContextChanged()` indicates we need to call `UpdateOnPrePaint` in `UpdateSoftNavigationContext` (for the modified node and all descendants).
In the simple case, for the node being marked:
- Call `MarkSoftNavigationContextChanged()` on a node
- Set that node's LayoutObjects's inherit bit to true in `UpdateSoftNavigationContext()` (without doing any other updates in UpdateSoftNavigationContext)
- Pass that context down to children...
But in more complex cases:
Mark Node A with context_1
Mark Node B (Node A's grandparent) with context_2 before painting
Now we have a stale Node A record, and UpdateSoftNavigationContext() will handle purging it.
Hopefully with the refactors it's a tad bit clearer, with this method being split up.
- Renamed to `marked_nodes_`
- Renamed to `MarkNodeAsDirectlyModified()`
- Removed `InitOrUpdatePaintTrackingForModifiedNodeIfNeeded()`, split up between (new) `MarkNodeForPaintTrackingIfNeeded()` and `UpdateOnPrePaint()`.
- Removed `PaintTrackingUpdateMode`
---
Re: DCHECK, sounds good (definitely DCHECK over CHECK, given the performance is a key part of this). Let me follow up though: (a) less chance of this getting reverted, and (b) there is one case we _don't_ currently handle: the whole inline text with effects (filter, opacity) is not handled due to that bug. I was thinking we might be able to walk up to find the box if we detect that happening (although would be nice to fix the bug).
EXPECT_TRUE(Tracker()->IsAttributable(content_node, context));What is the expectation for `Tracker()->IsAttributable(element, context)`?
The test for `<body>` implies that it would be in the modified set, even though it is never painted.
It will be `true` (added an expectation):
- Initially, element is added to `marked_nodes_` in `MarkNodeAsDirectlyModified()` (so true at that point).
- During pre-paint, document, ancestor, and content all propagate null. The "This is some content" text node also has a null inherited context, so updating that is a noop. Processing "element" is what actually propagates the context to its text node child, but since it's an inline element, it's not the text aggregator.
Note: with the recent changes, if we were o then directly modify "content" or "ancestor", we'd remove the mapping since there's a more recent context we can inherit.
Nit: `AppendInlineTextOverwritePreviousContextOfAggregationNode` since thats the most important detail this is testing, I think.
Changed to `AppendInlineTextOverwritePreviousAggregationNodeContext`
// TODO(crbug.com/423670827): For now, appending to an attributed node withoutThe bug doesn't mention this detail (or much detail). Not blocking patch but if we have followups that are leftovers let's document.
Yeah I was using this as a TODO list to make bugs later, which can have that one as a parent. If you want me to make bugs before landing and update, can do that too (didn't want to spend the time until this settled).
If you created yet another context and updated inner text again (after the first one is set), would it update to a new context? The comments imply that its a bug that we only assign context when going from none -> some on first text?
I know we wont currently observe paint for nodes but I think these tests don't need that to work.
The question is: should a DOM modification with no associated context that modifies the descendant of a Node associated with a context (A) count be attributed to A? That's what we do now, but the modification could be unrelated. We discussed this a bit offline --- and it's outside the scope of this CL (given this matches the current behavior) --- but noting it for followup.
Tracker()->GetSoftNavigationContextForPrePaint(GetDocument().body()),And to confirm I get this now-- this means when we are going top-down during prepaint we won't apply this context to any paintable-nodes, but if this node happens to paint, it will get attributed.
Cool.
Yes! And we only consider it paintable because it has a direct text child.
Related details
- Johannes Henkel
- Michal Mocny
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michal Mocny (Gerrit)
Michal Mocny added 16 comments![]( "Open in Gerrit")
I think this LGTM but sending comments first!
Once again, you've produced a patch that is some of the coolest code I've ever read!
2. When a connected DOM node is modified (currently only appended, butNit: if src= lands, this comment becomes stale. This detail also doesn't matter for this patch.
{"Advanced Paint Attribution (Eager Cached Pre-Paint Walk)",Q: will this appear in chrome:flags?
Node* soft_navigation_text_aggregation_node = nullptr;For the future:
Perhaps just "paint_timing_text_aggregation_node", or even "paint_timing_reporting_node" even, in case we ever had other default-paint-aggregation types?
Node* soft_navigation_context_source_node = nullptr;Nit: WDYT about calling "source node" ==> "container root"?
Also, I like that this shook out in this direction, it feels quite clean! If we ever did try to merge "Container Timing API" it feels closer to that use case now
!context.soft_navigation_context_changed &&For the future:
I think I lost an old comment, you may have answered-- `InvalidatePaintForHitTesting` in theory needs renaming?
context.soft_navigation_paint_attribution_tracker->UpdateOnPrePaint(Maybe I'm just becoming more comfortable with it-- but this feels so much cleaner!
context.soft_navigation_context_source_node = object.GetNode();I think that `PrePaintTreeWalk::UpdateSoftNavigationContext` will get called for all PrePaint walks, rather than just those with Soft-Nav-Context, right?
And this `else if` will only get here if `!object.SoftNavigationContextChanged()`, or the `PrePaintTreeWalkContext` doesn't already have a context-changed flag.
---
If that is all right, I think we are assuming that `!ShouldInheritSoftNavigationContext()` implies this is a container root?
If thats true, this is sort of hidden in details given that this like 336 would sort of the the line that kicks everything off?
Consider at least adding a comment, or even swapping the if() order so "new root" case comes first.
---
(I might be misunderstanding the details)
uint64_t ContextId() const { return context_id_; }Still need to expose?
CHECK_NE(paint_attribution_mode_,Note: if we do add a DCHECK we will have to remove this.
bool IsAttributable(Node* node, SoftNavigationContext* context) const {Nit: given the "must be image" wonder if this should be `IsAttributablePaintCandidateNode` (or something shorter)
// recorded in `marked_nodes_`. If the `Node` has a `LayoutObject` -- which
// depends if the layout has run for the node -- bits (1) and (2) are updated
// accordingly. Note: the full tree is re-walked as needed when the DOMHmm, I thought we also read bit 3 as part of pre-paint, for new container roots, so presumably we also write bit 3 sometimes?
current_context->ContextId() >= modifying_context->ContextId()) {Scott HaseleyWe only call MarkSoftNavigationContextChanged if the context actually changed... but then in `PrePaintTreeWalk::UpdateSoftNavigationContext` if Marked, we also check if it changed...
Is the difference that here we are only certain about the node, and the other is checking also for inheritance?
I don't quite follow the "if Marked" part.
Calling `MarkSoftNavigationContextChanged()` indicates we need to call `UpdateOnPrePaint` in `UpdateSoftNavigationContext` (for the modified node and all descendants).
In the simple case, for the node being marked:
- Call `MarkSoftNavigationContextChanged()` on a node
- Set that node's LayoutObjects's inherit bit to true in `UpdateSoftNavigationContext()` (without doing any other updates in UpdateSoftNavigationContext)
- Pass that context down to children...But in more complex cases:
Mark Node A with context_1
Mark Node B (Node A's grandparent) with context_2 before paintingNow we have a stale Node A record, and UpdateSoftNavigationContext() will handle purging it.
Hopefully with the refactors it's a tad bit clearer, with this method being split up.
Acknowledged
PaintTrackingUpdateMode::kTextAggregation);Acknowledged
if (node->IsTextNode() || IsImageType(*node->GetLayoutObject())) {This is perfect for this patch, but here's a mental model I'm building:
- We have explicit-container-roots which are passed down (marked by interactions or developers)
- We have implicit-paint-timing-roots (today: text aggregators, and images implicitly, tomorrow: <tables>, canvas roots, svg roots etc)
- We have nodes-that-might-paint (today: text or images, tomorrow: canvas-context? svg-elements?)
There is some overlap, in that nodes-that-might-paint must also be implicit-paint-timing-roots unless already in another root.
We could replace testing for `nodes-that-might-paint` and mark all `implicit-paint-timing-roots` but this fails for potential-text-aggregators-but-no-actual-text so the extra check helps filter a bunch of unneeded marked nodes.
Related details
- Johannes Henkel
- Scott Haseley
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Scott Haseley (Gerrit)
Scott Haseley added 11 comments![]( "Open in Gerrit")
I think this LGTM but sending comments first!
Once again, you've produced a patch that is some of the coolest code I've ever read!
Aww shucks. Thanks, this one was kind of fun :).
2. When a connected DOM node is modified (currently only appended, butNit: if src= lands, this comment becomes stale. This detail also doesn't matter for this patch.
True, and good point.
{"Advanced Paint Attribution (Eager Cached Pre-Paint Walk)",Q: will this appear in chrome:flags?
Yes, those are the descriptions you'll see in the UI.
Nit: WDYT about calling "source node" ==> "container root"?
Also, I like that this shook out in this direction, it feels quite clean! If we ever did try to merge "Container Timing API" it feels closer to that use case now
SGTM -- was reaching for something like that, but was too tired for naming :).
And same, I was thinking that when making this change, which also made me think it's going in the right direction.
!context.soft_navigation_context_changed &&For the future:
I think I lost an old comment, you may have answered-- `InvalidatePaintForHitTesting` in theory needs renaming?
Just chatted w/ pdr@ and we figured out this isn't actually needed here. It won't hurt, but it's not needed.
This method invalidates paint so that updated hit testing metadata will be sent to the compositor thread. Wheel and touch handlers can change this data, so the invalidation is required to get things in sync. OTOH this isn't needed for soft navs (we'll hook into the next paint when it happens), so we want to copy wheel/touch _except_ for this.
context.soft_navigation_context_source_node = object.GetNode();I think that `PrePaintTreeWalk::UpdateSoftNavigationContext` will get called for all PrePaint walks, rather than just those with Soft-Nav-Context, right?
And this `else if` will only get here if `!object.SoftNavigationContextChanged()`, or the `PrePaintTreeWalkContext` doesn't already have a context-changed flag.
---
If that is all right, I think we are assuming that `!ShouldInheritSoftNavigationContext()` implies this is a container root?
If thats true, this is sort of hidden in details given that this like 336 would sort of the the line that kicks everything off?
Consider at least adding a comment, or even swapping the if() order so "new root" case comes first.
---
(I might be misunderstanding the details)
I think that `PrePaintTreeWalk::UpdateSoftNavigationContext` will get called for all PrePaint walks, rather than just those with Soft-Nav-Context, right?
Correct, for nodes being visited anyway (the walk early-outs if children don't need to be updated for any reason).
And this `else if` will only get here if `!object.SoftNavigationContextChanged()`, or the `PrePaintTreeWalkContext` doesn't already have a context-changed flag.
Correct, this is the case for "neither this node nor any ancestor has changed".
---
If that is all right, I think we are assuming that `!ShouldInheritSoftNavigationContext()` implies this is a container root?
Yes. We inherit the parent's context, unless this is a container root (recursively).
If thats true, this is sort of hidden in details given that this like 336 would sort of the the line that kicks everything off?
It is a bit subtle/buried, but that's also only for re-walks. So while 336 kicks things off for rewalks. it's the first block that sets everything up and caches things for the future.
Consider at least adding a comment, or even swapping the if() order so "new root" case comes first.
I'll keep the order, but added a comment about how it works and what happens on first pass/subsequent passes (I like that first block == first pass, and it also keeps the conditions simpler (i.e. we don't have to add !changed to the inherit condition).
---
(I might be misunderstanding the details)
3/3!
uint64_t ContextId() const { return context_id_; }Scott HaseleyStill need to expose?
I'm using this for caching the last context to call `MarkDirectlyModifiedNode()`, rather than storing a (Weak)Member<SoftNavigationContext>. We could also do that, but meh less GC/weakness.
Note: if we do add a DCHECK we will have to remove this.
ACK. I'll follow up in a subsequent patch.
auto* state = GetNodeState(node);Chefs kiss.
:)
bool IsAttributable(Node* node, SoftNavigationContext* context) const {Nit: given the "must be image" wonder if this should be `IsAttributablePaintCandidateNode` (or something shorter)
It will also return true for directly modified container roots (% pruning), which may or may not be paint candidates. And it's used in tests for those nodes test the implementation. Going to leave for now.
// recorded in `marked_nodes_`. If the `Node` has a `LayoutObject` -- which
// depends if the layout has run for the node -- bits (1) and (2) are updated
// accordingly. Note: the full tree is re-walked as needed when the DOMHmm, I thought we also read bit 3 as part of pre-paint, for new container roots, so presumably we also write bit 3 sometimes?
Bit (3) isn't updated/used until pre-paint, described below (lines 42--48). I think that's clear, but lmk if not. See also the comments added in the pre-paint-walking class.
Related details
- Johannes Henkel
- Michal Mocny
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Johannes Henkel (Gerrit)
Johannes Henkel added 1 comment![]( "Open in Gerrit")
CHECK(IsPrePaintBasedAttributionEnabled() || !paint_attribution_tracker_);I'm surprised it says !paint_attribution_tracker_ here, in combination with the ||. Any chance this check should read more something like
CHECK(mode is enabled && tracker is set) ?
Related details
- Michal Mocny
- Scott Haseley
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Scott Haseley (Gerrit)
Scott Haseley added 1 comment![]( "Open in Gerrit")
CHECK(IsPrePaintBasedAttributionEnabled() || !paint_attribution_tracker_);I'm surprised it says !paint_attribution_tracker_ here, in combination with the ||. Any chance this check should read more something like
CHECK(mode is enabled && tracker is set) ?
This returns null when the mode isn't enabled, and it does get called when the feature is disabled (in which case the pre-paint walk basically propagates null), so that check would fry us.
But it should probably be CHECK_EQ(enabled, !!tracker).
Related details
- Johannes Henkel
- Michal Mocny
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Johannes Henkel (Gerrit)
Johannes Henkel added 1 comment![]( "Open in Gerrit")
CHECK(IsPrePaintBasedAttributionEnabled() || !paint_attribution_tracker_);Scott HaseleyI'm surprised it says !paint_attribution_tracker_ here, in combination with the ||. Any chance this check should read more something like
CHECK(mode is enabled && tracker is set) ?
This returns null when the mode isn't enabled, and it does get called when the feature is disabled (in which case the pre-paint walk basically propagates null), so that check would fry us.
But it should probably be CHECK_EQ(enabled, !!tracker).
Ah, makes sense - sorry I wasn't looking deeply enough. I think I do like the CHECK_EQ you mention better, since it'll also check that when it's enabled what's returned is not null.
Related details
- Michal Mocny
- Scott Haseley
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Scott Haseley (Gerrit)
Scott Haseley added 1 comment![]( "Open in Gerrit")
CHECK(IsPrePaintBasedAttributionEnabled() || !paint_attribution_tracker_);Scott HaseleyI'm surprised it says !paint_attribution_tracker_ here, in combination with the ||. Any chance this check should read more something like
CHECK(mode is enabled && tracker is set) ?
Johannes HenkelThis returns null when the mode isn't enabled, and it does get called when the feature is disabled (in which case the pre-paint walk basically propagates null), so that check would fry us.
But it should probably be CHECK_EQ(enabled, !!tracker).
Ah, makes sense - sorry I wasn't looking deeply enough. I think I do like the CHECK_EQ you mention better, since it'll also check that when it's enabled what's returned is not null.
All good, it should be changed - thanks for getting another set of eyes on it :). I'll fix before landing. I'll leave this open and group with any last nits Michal might have. Hoping to land today.
Related details
- Johannes Henkel
- Michal Mocny
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Johannes Henkel (Gerrit)
Johannes Henkel added 2 comments![]( "Open in Gerrit")
// for the element, but not propagate the change to all ancestors of <body>.descendants?
// generation, there's no need to update anything, unless upgrading to a
// direct modification.Upgrading to a direct modification involves flipping the bit to true, so I wonder whether modifying the NodeState object would be better? Maybe it's not needed for correctness (I don't think you're keeping references to these objects elsewhere) but might be slightly more efficient. I think if not needed for correctness, what you have is more readable so maybe best to leave as is, this comment is more of a "did we think about that" type of comment. :-)
Scott Haseley (Gerrit)
Scott Haseley added 1 comment![]( "Open in Gerrit")
// generation, there's no need to update anything, unless upgrading to a
// direct modification.Upgrading to a direct modification involves flipping the bit to true, so I wonder whether modifying the NodeState object would be better? Maybe it's not needed for correctness (I don't think you're keeping references to these objects elsewhere) but might be slightly more efficient. I think if not needed for correctness, what you have is more readable so maybe best to leave as is, this comment is more of a "did we think about that" type of comment. :-)
The Modification ID and context could also change, so it could be all of the bits. My thought process was that it should be cheap enough to make + insert a new one, and I like having the NodeState be immutable.
Johannes Henkel (Gerrit)
Johannes Henkel voted and added 1 comment![]( "Open in Gerrit")
Votes added by Johannes Henkel
| Code-Review | +1 |
1 comment
Related details
- Michal Mocny
- Scott Haseley
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Scott Haseley (Gerrit)
Scott Haseley voted and added 3 comments![]( "Open in Gerrit")
Votes added by Scott Haseley
| Commit-Queue | +2 |
3 comments
CHECK(IsPrePaintBasedAttributionEnabled() || !paint_attribution_tracker_);Scott HaseleyI'm surprised it says !paint_attribution_tracker_ here, in combination with the ||. Any chance this check should read more something like
CHECK(mode is enabled && tracker is set) ?
Johannes HenkelThis returns null when the mode isn't enabled, and it does get called when the feature is disabled (in which case the pre-paint walk basically propagates null), so that check would fry us.
But it should probably be CHECK_EQ(enabled, !!tracker).
Scott HaseleyAh, makes sense - sorry I wasn't looking deeply enough. I think I do like the CHECK_EQ you mention better, since it'll also check that when it's enabled what's returned is not null.
All good, it should be changed - thanks for getting another set of eyes on it :). I'll fix before landing. I'll leave this open and group with any last nits Michal might have. Hoping to land today.
Done
// for the element, but not propagate the change to all ancestors of <body>.Scott Haseleydescendants?
Yes!
Related details
- Michal Mocny
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
25 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: third_party/blink/renderer/core/timing/soft_navigation_paint_attribution_tracker.h
Insertions: 1, Deletions: 1.
@@ -64,7 +64,7 @@
// <body><b>Some text</b></body>
//
// And if such an element is appended to <body>, we want to track the paints
-// for the element, but not propagate the change to all ancestors of <body>.
+// for the element, but not propagate the change to all descendants of <body>.
// This is handled by only propagating directly modified nodes during the
// pre-paint walk.
class CORE_EXPORT SoftNavigationPaintAttributionTracker
```
```
The name of the file: third_party/blink/renderer/core/timing/soft_navigation_heuristics.h
Insertions: 1, Deletions: 1.
@@ -133,7 +133,7 @@
std::optional<EventScope> MaybeCreateEventScopeForEvent(const Event&);
SoftNavigationPaintAttributionTracker* GetPaintAttributionTracker() {
- CHECK(IsPrePaintBasedAttributionEnabled() || !paint_attribution_tracker_);
+ CHECK_EQ(IsPrePaintBasedAttributionEnabled(), !!paint_attribution_tracker_);
return paint_attribution_tracker_.Get();
}
```
Change information
[soft navs] Introduce a Pre-Paint-based paint attribution mode
Currently, Soft Navigation paint attribution works as follows:
1. If the DOM update is not "task attributable", do nothing
2. When a connected DOM node is modified (currently only appended, but
that will change soon), we add it to a set of nodes to do paint
attribution for
3. When painting contentful elements:
a) (enabled-by-default): Check if the node is in the set for some
context, and attribute the paint if so
b) (experimental): Walk up the DOM tree to see if any ancestor is
in the set, and attribute the paint if so
3(a) is the default, but it is not viable because it's inaccurate (we'd
need to consider disconnected nodes as well, which is too inefficient)
3(b) is more accurate, but there are efficiency concerns due to the
tree walk during paint. This also might limit which input events we can
do attribution for, which limits future metrics.
This CL introduces a new experimental option, which caches the state
needed for determining attribution in a pre-paint tree walk:
- When the SoftNavigationContext for a node changes, mark a bit on
LayoutObject indicating that this node has change, and update
ancestors indicating a descendant has changed. True by default in the
case the LayoutObject doesn't change (often true for soft navs, due
to append()).
- In pre-paint, if a node is being marked as changed, the context for
the node is determined to either be the parent (if inheriting) or the
node is a new "container root", with a different context. (tracked by
SoftNavigationPaintAttributionTracker)
- If inheriting, a ShouldInheritSoftNavigationContext bit gets set,
which is used for subsequent walks
- If not inheriting, the node being visited is a new container root,
associated with a different context, and that root is passed down
to children.
Initially, everything is wired up to inherit a null context.
SoftNavigationPaintAttributionTracker maintains a map of (weak) Node to
NodeState, which is the SNC and a couple other bits. To improve memory
efficiency, it only tracks the Nodes it needs to do attribution, which
are:
- Nodes that were directly modified (e.g. appended, which could be a
whole subtree)
- Text aggregation nodes (closest non-anonymous box ancestor of a text
node), which is what the paint functions see
- Images (svg, video, images, and background images)
See comments in SoftNavigationPaintAttributionTracker for more details.
- M chrome/browser/about_flags.cc
- M third_party/blink/common/features.cc
- M third_party/blink/public/common/features.h
- M third_party/blink/renderer/core/display_lock/display_lock_context.cc
- M third_party/blink/renderer/core/display_lock/display_lock_context.h
- M third_party/blink/renderer/core/display_lock/display_lock_context_test.cc
- M third_party/blink/renderer/core/layout/layout_object.cc
- M third_party/blink/renderer/core/layout/layout_object.h
- M third_party/blink/renderer/core/paint/pre_paint_tree_walk.cc
- M third_party/blink/renderer/core/paint/pre_paint_tree_walk.h
- M third_party/blink/renderer/core/paint/pre_paint_tree_walk_test.cc
- M third_party/blink/renderer/core/timing/build.gni
- M third_party/blink/renderer/core/timing/soft_navigation_context.cc
- M third_party/blink/renderer/core/timing/soft_navigation_context.h
- M third_party/blink/renderer/core/timing/soft_navigation_heuristics.cc
- M third_party/blink/renderer/core/timing/soft_navigation_heuristics.h
- A third_party/blink/renderer/core/timing/soft_navigation_paint_attribution_tracker.cc
- A third_party/blink/renderer/core/timing/soft_navigation_paint_attribution_tracker.h
- A third_party/blink/renderer/core/timing/soft_navigation_paint_attribution_tracker_test.cc
- M third_party/blink/renderer/platform/runtime_enabled_features.json5
- M third_party/blink/web_tests/VirtualTestSuites
- A third_party/blink/web_tests/virtual/soft-navigation-heuristics-pre-paint-attribution/README.md
Code-Review: +1 by Philip Rogers, +1 by Johannes Henkel
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |