Autogenerate compute_damage #20001

Manishearth · 2018-02-09T00:43:41Z

fixes #10622

This change is

highfive · 2018-02-09T00:43:45Z

Heads up! This PR modifies the following files:

@bholley: components/style/properties/longhand/inherited_text.mako.rs, components/style/lib.rs, components/style/properties/longhand/list.mako.rs, components/style/properties/longhand/padding.mako.rs, components/style/properties/data.py and 14 more
@canaltinova: components/style/properties/longhand/inherited_text.mako.rs, components/style/lib.rs, components/style/properties/longhand/list.mako.rs, components/style/properties/longhand/padding.mako.rs, components/style/properties/data.py and 14 more
@emilio: components/style/properties/longhand/inherited_text.mako.rs, components/style/lib.rs, components/style/properties/longhand/list.mako.rs, components/style/properties/longhand/padding.mako.rs, components/style/properties/data.py and 14 more

highfive · 2018-02-09T00:43:46Z

Warning

These commits modify style code, but no tests are modified. Please consider adding a test!

Manishearth · 2018-02-09T00:43:50Z

r? @mbrubeck

Manishearth · 2018-02-09T00:44:27Z

There were a bunch of properties that in the old code went unmentioned and are now "repaint". Should they all fall under the repaint category?

animation_delay
animation_direction
animation_duration
animation_fill_mode
animation_iteration_count
animation_name
animation_play_state
animation_timing_function
backface_visibility
border_image_outset
border_image_repeat
border_image_slice
border_image_source
border_image_width
box_sizing
list_style_image
list_style_position
list_style_type
max_height
max_width
min_height
min_width
outline_color
outline_offset
outline_style
outline_width
overflow_x
overflow_y
rotate
scale
transition_delay
transition_duration
transition_property
transition_timing_function
translate
vertical_align

mbrubeck · 2018-02-09T01:18:32Z

components/style/servo/restyle_damage.rs

+            [ServoRestyleDamage::REPAINT, ServoRestyleDamage::REPOSITION,
+            ServoRestyleDamage::STORE_OVERFLOW, ServoRestyleDamage::BUBBLE_ISIZES,
+            ServoRestyleDamage::REFLOW_OUT_OF_FLOW, ServoRestyleDamage::REFLOW]) ||
+    restyle_damage_reflow!(old, new, damage,


On second thought, this should be named reflow_out_of_flow instead of just reflow, because it doesn't include REFLOW damage.

reflow_and_bubble could be renamed to just reflow.

mbrubeck · 2018-02-09T01:23:34Z

backface_visibility
border_image_outset
border_image_repeat
border_image_slice
border_image_source
border_image_width
outline_color
outline_offset
outline_style
outline_width

I believe these can stay "repaint".

list_style_image
list_style_position
list_style_type

We access these during flow construction so they should be rebuild_and_reflow.

box_sizing
max_height
max_width
min_height
min_width
vertical_align
overflow_x
overflow_y

These should be reflow_and_bubble.

translate
rotate
scale

I believe these can be reflow_out_of_flow. These are all derived from the transform property, right?

transition_delay
transition_duration
transition_property
transition_timing_function
animation_delay
animation_direction
animation_duration
animation_fill_mode
animation_iteration_count
animation_name
animation_play_state
animation_timing_function

I'm not sure about whether we need to set damage for these, or if the animation code will take care of triggering appropriate damage and relayouts when the actual style changes get applied...

Manishearth · 2018-02-09T01:30:29Z

Done.

Manishearth · 2018-02-09T01:30:51Z

@bors-servo try

Wonder if this causes new passes.

bors-servo · 2018-02-09T01:30:56Z

⌛ Trying commit de08bf8 with merge 5f994e1...


        Auto merge of #20001 - Manishearth:compute-damage, r=<try>

Autogenerate compute_damage fixes #10622  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/20001)

bors-servo · 2018-02-09T01:45:30Z

💔 Test failed - mac-dev-unit

mbrubeck · 2018-02-09T01:47:20Z

Tests for these are tricky. Setting "too much" damage doesn't cause any bugs, just suboptimal performance. Setting "too little" damage causes bugs only in incremental layout, which don't affect reftests unless they are very specifically written to trigger them.

emilio

So there's something here that concerns me, which is that you really really want to not make each property change to imply a particular restyle damage, at least eventually.

See https://searchfox.org/mozilla-central/rev/a5abf843f8fac8530aa5de6fb40e16547bb4a47a/layout/style/nsStyleContext.cpp#349, for example, and there are more specific examples in the calcdifference methods of style structs.

This may be fine for now I guess. May I suggest servo_restyle_damage to make clear that this has no effect whatsoever in Gecko?

emilio · 2018-02-09T02:00:41Z

Also, I've been thinking on splitting Gecko's model to be different. In particular, the style system will only know whether inherited / reset / variables changed. Then each particular frame / flow type will diff and apply the damage properly.

This is handy because changing properties, for example, in column code, may not / will not really affect non-multicol flows, and thus any flow type but multicol may as well skip them.

Also, there's another optimization that you really really want but you don't have right now, which is skipping the deep comparison iff the structs are pointer-equal (which is somewhat common).

mbrubeck · 2018-02-09T16:03:19Z

This may be fine for now I guess.

Yeah, let's land this for now. It's an improvement for correctness and has the same performance as our old servo code, even if future optimizations might need a different approach.

May I suggest servo_restyle_damage to make clear that this has no effect whatsoever in Gecko?

That sounds good. r=mbrubeck with that change and test-tidy fixes.


        Autogenerate compute_damage (fixes #10622)


        Set damage for min/max width/height to reflow_and_bubble


        Make individual transform properties have restyle_damage = reflow_out…

…_of_flow


        Use restyle_damage=rebuild_and_reflow for list types


        Set restyle_damage = reflow for some more properties

Manishearth · 2018-02-09T17:02:00Z

@bors-servo r=mbrubeck try-

bors-servo · 2018-02-09T17:02:01Z

📌 Commit daea6d0 has been approved by mbrubeck

bors-servo · 2018-02-09T17:02:06Z

⌛ Testing commit daea6d0 with merge e19bab8...


        Auto merge of #20001 - Manishearth:compute-damage, r=mbrubeck

Autogenerate compute_damage fixes #10622  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/20001)

bors-servo · 2018-02-09T17:51:42Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev
Approved by: mbrubeck
Pushing e19bab8 to master...

highfive assigned glennw Feb 9, 2018

highfive added the S-awaiting-review label Feb 9, 2018

highfive assigned mbrubeck and unassigned glennw Feb 9, 2018

mbrubeck reviewed Feb 9, 2018

View changes

Manishearth force-pushed the Manishearth:compute-damage branch from 2bb251e to de08bf8 Feb 9, 2018

highfive added the S-tests-failed label Feb 9, 2018

Manishearth force-pushed the Manishearth:compute-damage branch from de08bf8 to d3d6cd3 Feb 9, 2018

highfive removed the S-tests-failed label Feb 9, 2018

emilio reviewed Feb 9, 2018

View changes

Manishearth force-pushed the Manishearth:compute-damage branch 3 times, most recently from 064739c to 4250233 Feb 9, 2018

Manishearth added 4 commits Feb 8, 2018

Autogenerate compute_damage (fixes #10622)

8c2b7b4

Set damage for min/max width/height to reflow_and_bubble

6ba23fe

Make individual transform properties have restyle_damage = reflow_out…

7e928b0

…_of_flow

Use restyle_damage=rebuild_and_reflow for list types

11fb2e6

Set restyle_damage = reflow for some more properties

Loading status checks…

daea6d0

Manishearth force-pushed the Manishearth:compute-damage branch from 4250233 to daea6d0 Feb 9, 2018

highfive added S-awaiting-merge and removed S-awaiting-review labels Feb 9, 2018

highfive removed the S-awaiting-merge label Feb 9, 2018

Manishearth deleted the Manishearth:compute-damage branch May 7, 2019

servo / servo

Autogenerate compute_damage #20001

Autogenerate compute_damage #20001

Manishearth commented Feb 9, 2018 •

edited by SimonSapin

highfive commented Feb 9, 2018

highfive commented Feb 9, 2018

Manishearth commented Feb 9, 2018

Manishearth commented Feb 9, 2018

This comment has been minimized.

mbrubeck commented Feb 9, 2018

Manishearth commented Feb 9, 2018

Manishearth commented Feb 9, 2018

bors-servo commented Feb 9, 2018

bors-servo commented Feb 9, 2018

mbrubeck commented Feb 9, 2018 •

edited

emilio left a comment

emilio commented Feb 9, 2018

mbrubeck commented Feb 9, 2018

Manishearth commented Feb 9, 2018

bors-servo commented Feb 9, 2018

bors-servo commented Feb 9, 2018

bors-servo commented Feb 9, 2018

servo / servo

Join GitHub today

Autogenerate compute_damage #20001

Autogenerate compute_damage #20001

Conversation

Manishearth commented Feb 9, 2018 • edited by SimonSapin

highfive commented Feb 9, 2018

highfive commented Feb 9, 2018

Manishearth commented Feb 9, 2018

Manishearth commented Feb 9, 2018

This comment has been minimized.

mbrubeck Feb 9, 2018

mbrubeck commented Feb 9, 2018

Manishearth commented Feb 9, 2018

Manishearth commented Feb 9, 2018

bors-servo commented Feb 9, 2018

bors-servo commented Feb 9, 2018

mbrubeck commented Feb 9, 2018 • edited

emilio left a comment

emilio commented Feb 9, 2018

mbrubeck commented Feb 9, 2018

Manishearth commented Feb 9, 2018

bors-servo commented Feb 9, 2018

bors-servo commented Feb 9, 2018

bors-servo commented Feb 9, 2018

Manishearth commented Feb 9, 2018 •

edited by SimonSapin

mbrubeck commented Feb 9, 2018 •

edited