layout: Add a first pass at incremental box tree construction #37751
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
🔨 Triggering try run (#15928552071) for Linux (WPT) |
d03ecc1 to
226770f
Compare
|
🔨 Triggering try run (#15928578749) for Linux (WPT) |
Loirooriol
approved these changes
Jun 27, 2025
|
|
226770f to
d48db54
Compare
|
🔨 Triggering try run (#15929400204) for Linux (WPT) |
|
Test results for linux-wpt from try job (#15929400204): Flaky unexpected result (13)
Stable unexpected results that are known to be intermittent (18)
Stable unexpected results (5)
|
|
|
components/layout/flow/root.rs
Outdated
| // | ||
| // TODO: This isn't going to be good enough for incremental fragment tree | ||
| // reconstruction, as fragment tree damage might extend further up the tree. | 8000||
| parent_node.clear_restyle_damage(); |
There was a problem hiding this comment.
The damage of the dirty node itself has not been cleaned
components/layout/dom_traversal.rs
Outdated
| }, | ||
| } | ||
|
|
||
| element.clear_restyle_damage(); |
There was a problem hiding this comment.
Because the check whether a old block level box can be reused is delayed until the BlockLevelJob finished, clear the restyle damage at here maybe too early if a element need to be traversed one more times, such as the block level child element of a table. I have already tried it at my local and that the failed test case pass. Can we clear the restyle damage within the LayoutBoxBase new function? Or add logic at the BlockLevelJob:new to correct the damage for children element
There was a problem hiding this comment.
In my original PR: 37130, the "box repair" and "box rebuild" is two separate process, so the invariance can always be kept: If the parent node 's box is rebuilt, then all its descendent are also rebuilt.
components/layout/flow/construct.rs
Outdated
|
|
||
| // If this `BlockLevelBox` is undamaged and it has been laid out before, reuse | ||
| // the old one, while being sure to clear the layout cache. | ||
| if !self.damage.has_box_damage() { |
There was a problem hiding this comment.
Only reuse the whole box for the node which has no box damage. However, for those node with only RECOLLECT_CHILDREN_BOX, nothing is reused. With current increment layout, it is enough. However, is it meaningful to preserve the LayoutBoxBase in order to preserve some layout cache for the incremental layout , such as cached_inline_content_size.
| }, | ||
| None => None, | ||
| } { | ||
| block_level_box.borrow().invalidate_cached_fragment(); |
There was a problem hiding this comment.
With current incremental layout, only clear cached fragment maybe not enough. When the incremental box tree update is extended to reuse flex/taffy level box, some incorrect layout maybe appear. So clear the all cached layout result for this node's subtree should be necessary.
|
If you need it, you can use the following changes to fix the failing test case. diff from your diffdiff --git a/components/layout/flow/construct.rs b/components/layout/flow/construct.rs
index f6ed4908b7..65883b5e3e 100644
--- a/components/layout/flow/construct.rs
+++ b/components/layout/flow/construct.rs
@@ -50,6 +50,22 @@ impl BlockFormattingContext {
))
}
+ pub(crate) fn construct_without_recollect_children(
+ context: &LayoutContext,
+ info: &NodeAndStyleInfo<'_>,
+ contents: NonReplacedContents,
+ propagated_data: PropagatedBoxTreeData,
+ is_list_item: bool,
+ ) -> Self {
+ Self::from_block_container(BlockContainer::construct_without_recollect_children(
+ context,
+ info,
+ contents,
+ propagated_data,
+ is_list_item,
+ ))
+ }
+
pub(crate) fn from_block_container(contents: BlockContainer) -> Self {
let contains_floats = contents.contains_floats();
Self {
@@ -158,6 +174,11 @@ pub(crate) struct BlockContainerBuilder<'dom, 'style> {
/// ancestors that have been processed. This `Vec` allows passing those into new
/// [`InlineFormattingContext`]s that we create.
display_contents_shared_styles: Vec<SharedInlineStyles>,
+
+ /// A flag indicates whether this builder can collect box tree children which can be
+ /// reused due to that their originating nodes have no box damage. This is a workround
+ /// for constructing [`BlockFormattingContext`] in [`Table`].
+ allow_recollect_children: bool,
}
impl BlockContainer {
@@ -168,25 +189,19 @@ impl BlockContainer {
propagated_data: PropagatedBoxTreeData,
is_list_item: bool,
) -> BlockContainer {
- let mut builder = BlockContainerBuilder::new(context, info, propagated_data);
- if is_list_item {
- if let Some((marker_info, marker_contents)) = crate::lists::make_marker(context, info) {
- match marker_info.style.clone_list_style_position() {
- ListStylePosition::Inside => {
- builder.handle_list_item_marker_inside(&marker_info, info, marker_contents)
- },
- ListStylePosition::Outside => builder.handle_list_item_marker_outside(
- &marker_info,
- info,
- marker_contents,
- info.style.clone(),
- ),
- }
- }
- }
+ let builder = BlockContainerBuilder::new(context, info, propagated_data, true);
+ builder.build(contents, is_list_item)
+ }
- contents.traverse(context, info, &mut builder);
- builder.finish()
+ pub fn construct_without_recollect_children(
+ context: &LayoutContext,
+ info: &NodeAndStyleInfo<'_>,
+ contents: NonReplacedContents,
+ propagated_data: PropagatedBoxTreeData,
+ is_list_item: bool,
+ ) -> BlockContainer {
+ let builder = BlockContainerBuilder::new(context, info, propagated_data, false);
+ builder.build(contents, is_list_item)
}
}
@@ -195,6 +210,7 @@ impl<'dom, 'style> BlockContainerBuilder<'dom, 'style> {
context: &'style LayoutContext,
info: &'style NodeAndStyleInfo<'dom>,
propagated_data: PropagatedBoxTreeData,
+ allow_recollect_children: bool,
) -> Self {
BlockContainerBuilder {
context,
@@ -206,9 +222,35 @@ impl<'dom, 'style> BlockContainerBuilder<'dom, 'style> {
anonymous_table_content: Vec::new(),
inline_formatting_context_builder: None,
display_contents_shared_styles: Vec::new(),
+ allow_recollect_children,
}
}
+ fn build(mut self, contents: NonReplacedContents, is_list_item: bool) -> BlockContainer {
+ if is_list_item {
+ if let Some((marker_info, marker_contents)) =
+ crate::lists::make_marker(self.context, self.info)
+ {
+ match marker_info.style.clone_list_style_position() {
+ ListStylePosition::Inside => self.handle_list_item_marker_inside(
+ &marker_info,
+ self.info,
+ marker_contents,
+ ),
+ ListStylePosition::Outside => self.handle_list_item_marker_outside(
+ &marker_info,
+ self.info,
+ marker_contents,
+ self.info.style.clone(),
+ ),
+ }
+ }
+ }
+
+ contents.traverse(self.context, self.info, &mut self);
+ self.finish()
+ }
+
fn currently_processing_inline_box(&self) -> bool {
self.inline_formatting_context_builder
.as_ref()
@@ -306,6 +348,7 @@ impl<'dom, 'style> BlockContainerBuilder<'dom, 'style> {
BoxSlot::dummy(),
self.propagated_data,
BlockLevelCreator::AnonymousTable { table_block },
+ self.allow_recollect_children,
));
}
@@ -455,6 +498,7 @@ impl<'dom> BlockContainerBuilder<'dom, '_> {
contents,
list_item_style,
},
+ self.allow_recollect_children,
));
}
@@ -583,6 +627,7 @@ impl<'dom> BlockContainerBuilder<'dom, '_> {
box_slot,
propagated_data,
kind,
+ self.allow_recollect_children,
));
// Any block also counts as the first line for the purposes of text indent. Even if
@@ -620,6 +665,7 @@ impl<'dom> BlockContainerBuilder<'dom, '_> {
box_slot,
self.propagated_data,
kind,
+ self.allow_recollect_children,
));
}
@@ -653,6 +699,7 @@ impl<'dom> BlockContainerBuilder<'dom, '_> {
box_slot,
self.propagated_data,
kind,
+ self.allow_recollect_children,
));
}
@@ -680,6 +727,7 @@ impl<'dom> BlockContainerBuilder<'dom, '_> {
BlockContainer::InlineFormattingContext(inline_formatting_context),
),
),
+ self.allow_recollect_children,
));
self.have_already_seen_first_line_for_text_indent = true;
@@ -692,8 +740,14 @@ impl<'dom> BlockLevelJob<'dom> {
box_slot: BoxSlot<'dom>,
propagated_data: PropagatedBoxTreeData,
kind: BlockLevelCreator,
+ container_allow_recollect_children: bool,
) -> Self {
- let damage = info.restyle_damage();
+ let damage = if container_allow_recollect_children {
+ info.restyle_damage()
+ } else {
+ LayoutDamage::REBUILD_BOX
+ };
+
Self {
info,
box_slot,
diff --git a/components/layout/table/construct.rs b/components/layout/table/construct.rs
index 191faaa77c..bcc9ae68bf 100644
--- a/components/layout/table/construct.rs
+++ b/components/layout/table/construct.rs
@@ -857,13 +857,15 @@ impl<'dom> TraversalHandler<'dom> for TableBuilderTraversal<'_, 'dom> {
DisplayLayoutInternal::TableCaption => {
let contents = match contents.try_into() {
Ok(non_replaced_contents) => {
- IndependentNonReplacedContents::Flow(BlockFormattingContext::construct(
- self.context,
- info,
- non_replaced_contents,
- self.current_propagated_data,
- false, /* is_list_item */
- ))
+ IndependentNonReplacedContents::Flow(
+ BlockFormattingContext::construct_without_recollect_children(
+ self.context,
+ info,
+ non_replaced_contents,
+ self.current_propagated_data,
+ false, /* is_list_item */
+ ),
+ )
},
Err(_replaced) => {
unreachable!("Replaced should not have a LayoutInternal display type.");
@@ -949,7 +951,8 @@ impl<'style, 'builder, 'dom, 'a> TableRowBuilder<'style, 'builder, 'dom, 'a> {
.pseudo(context, PseudoElement::ServoAnonymousTableCell)
.expect("Should never fail to create anonymous table cell info");
let propagated_data = self.propagated_data.disallowing_percentage_table_columns();
- let mut builder = BlockContainerBuilder::new(context, &anonymous_info, propagated_data);
+ let mut builder =
+ BlockContainerBuilder::new(context, &anonymous_info, propagated_data, false);
for cell_content in self.current_anonymous_cell_content.drain(..) {
match cell_content {
@@ -1018,7 +1021,7 @@ impl<'dom> TraversalHandler<'dom> for TableRowBuilder<'_, '_, 'dom, '_> {
self.propagated_data.disallowing_percentage_table_columns();
let contents = match contents.try_into() {
Ok(non_replaced_contents) => {
- BlockFormattingContext::construct(
+ BlockFormattingContext::construct_without_recollect_children(
self.table_traversal.context,
info,
non_replaced_contents,This PR is looks good to me, thank you for split this change from my original PR. After this PR merged, i will immediately commit the support for reusing flex/taffy level box. |
|
🔨 Triggering try run (#15996293014) for Linux (WPT) |
|
Test results for linux-wpt from try job (#15996293014): Flaky unexpected result (16)
Stable unexpected results that are known to be intermittent (27)
Stable unexpected results (5)
|
|
|
d48db54 to
d467934
Compare
|
🔨 Triggering try run (#15997757388) for Linux (WPT) |
|
Test results for linux-wpt from try job (#15997757388): Flaky unexpected result (13)
Stable unexpected results that are known to be intermittent (17)
|
|
✨ Try run (#15997757388) succeeded. |
|
🔨 Triggering try run (#15998842719) for Linux (WPT) |
|
Test results for linux-wpt from try job (#15998842719): Flaky unexpected result (14)
Stable unexpected results that are known to be intermittent (24)
Stable unexpected results (21)
|
|
|
| doc.note_node_with_dirty_descendants(self.upcast()); | ||
| restyle | ||
| .damage | ||
| .insert(LayoutDamage::recollect_box_tree_children()); |
There was a problem hiding this comment.
it should be insert(LayoutDamage::recollect_box_tree_children() | RestyleDamage::RELAYOUT) or the RestyleDamage::RELAYOUT bit is added into the return value of LayoutDamage::recollect_box_tree_children()
There was a problem hiding this comment.
By the way, since it seems that we ready to support the NodeDamage::ContentOrHeritage, is it necessary to preserve the cached layout result in LayoutBoxBase for these boxes which have RecollectBoxTreeChildren damage but not REBUILD_BOX damage.
285fd36 to
9958e7d
Compare
|
🔨 Triggering try run (#16004243835) for Linux (WPT) |
9958e7d to
bdf2032
Compare
|
Test results for linux-wpt from try job (#16004243835): Flaky unexpected result (22)
Stable unexpected results that are known to be intermittent (17)
|
|
✨ Try run (#16004243835) succeeded. |
This change: - Adds a new type of LayoutDamage that signifies that a box needs its children recollected, because one or more of them need to be rebuilt. - During restyle damage propagation, propagate this new damage upward in the tree. Then box tree construction should be able to preserve any still-valid box tree nodes from box slots. - During BlockLevelBox job finalization, if a box slot is valid and there is not LayoutDamage to the element, use the old box slot, ensuring that its fragment cache is invalidated. Signed-off-by: Martin Robinson <mrobinson@igalia.com> Co-authored-by: Oriol Brufau <obrufau@igalia.com> Co-authored-by: coding-joedow <ibluegalaxy_taoj@163.com>
bdf2032 to
3bb0170
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This change:
children recollected, because one or more of them need to be rebuilt.
the tree. Then box tree construction should be able to preserve any
still-valid box tree nodes from box slots.
there is not LayoutDamage to the element, use the old box slot,
ensuring that its fragment cache is invalidated.
Testing: This should not change observable behavior and thus is covered
by existing WPT tests.