1380327 - gecko_properties.rs is not generated deterministically

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P1)

Description

[Mike Hommey [:glandium]]

Running two clobber builds in a row, with no changes, using sccache, some crates are getting cache misses.

One of the identified causes of this is the following diff between the objdirs:

--- a/toolkit/library/x86_64-unknown-linux-gnu/release/build/style-844a1337c4fb98af/out/gecko_properties.rs
+++ b/toolkit/library/x86_64-unknown-linux-gnu/release/build/style-844a1337c4fb98af/out/gecko_properties.rs
@@ -4339,7 +4339,7 @@ impl GeckoBorder {
         BorderImageSlice {
             offsets:
                 NumberOrPercentageRect::from_gecko_rect(&self.gecko.mBorderImageSlice)
-                    .expect("mBorderImageSlice[<built-in method index of str object at 0x7fd8924bc8a0>] could not convert to NumberOrP
ercentageRect"),
+                    .expect("mBorderImageSlice[<built-in method index of str object at 0x7f7bfa9cf8a0>] could not convert to NumberOrP
ercentageRect"),
             fill: self.gecko.mBorderImageFill as u32 == NS_STYLE_BORDER_IMAGE_SLICE_FILL
         }
     }
@@ -7829,7 +7829,7 @@ impl GeckoOutline {
 
         // NB: This is needed to correctly handling the initial value of
         // outline-width when outline-style changes, see the
-        // update_border_<_home_glandium_gecko_push_servo_components_style_properties_gecko_mako_rs.Side object at 0x7fd88ef54650> com
ment for more details.
+        // update_border_<_home_glandium_gecko_push_servo_components_style_properties_gecko_mako_rs.Side object at 0x7f7bf7431690> com
ment for more details.
         self.gecko.mActualOutlineWidth = self.gecko.mOutlineWidth;
     }

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1380327: Fix causes of non-determinism in gecko_properties.rs.

We should sort out all the different ways we have to get sides and all those
nasty loops, eventually...

Review commit: https://reviewboard.mozilla.org/r/156632/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/156632/

Comment 2

[Manish Goregaokar [:manishearth]]

Comment on attachment 8885865 [details]
Bug 1380327: Fix causes of non-determinism in gecko_properties.rs.

https://reviewboard.mozilla.org/r/156632/#review161754

Comment 3

[Emilio Cobos Álvarez (:emilio)]

https://github.com/servo/servo/pull/17691

Comment 4

[Xidorn Quan [:xidorn] UTC+10]

Probably we cam add a check to build script to fail if there is any "<.+ object at 0x[0-9a-f]+>" inside the generated file, so that we don't regress this.

Comment 5

[Mike Hommey [:glandium]]

I can confirm this change allows, on my machine, to have 100% cache hit rate with sccache.

Comment 6

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #4)
> Probably we cam add a check to build script to fail if there is any "<.+
> object at 0x[0-9a-f]+>" inside the generated file, so that we don't regress
> this.

Submitted Servo PR servo/servo#17702 to avoid regressing this.

Comment 7

[Mike Hommey [:glandium]]

FWIW
https://treeherder.mozilla.org/perf.html#/graphs?series=%5Btry,7c6ec9c4d898dda7736ddd7f0a8ddb9bef63b3cd,1,2%5D&highlightedRevisions=96ef8343d6de

There are 3 retries for that push:
- The first one had a cache miss on style for obvious reasons, since the code was new
- The second one had 100% cache hit, and is the one that is lower than any other cross build on try
- The third one had a cache miss on webrender, which is another problem.

Also, this landed on autoland: https://hg.mozilla.org/integration/autoland/rev/8b8c58abdd34

Comment 8

[Mike Hommey [:glandium]]

This is starting to take effect:
https://treeherder.mozilla.org/perf.html#/graphs?timerange=86400&series=%5Bautoland,4044b74c437dfc672f4615a746ea01f6e4c0312d,1,2%5D&series=%5Bautoland,7c6ec9c4d898dda7736ddd7f0a8ddb9bef63b3cd,1,2%5D&series=%5Bautoland,be465112346255e89dd26003061b01f27cc7fd39,1,2%5D&series=%5Bautoland,d416d6e2ae32b6d43bb1355394d31bfe4fbeee27,1,2%5D&series=%5Bautoland,be7331d4f74b6970f67fe9da80f4d4d90ef60b73,1,2%5D&series=%5Bautoland,18851c0bb8875e85890ba4b1aed2439f8c682743,1,2%5D

Comment 9

[Chris Peterson [:cpeterson]]

Thanks for identifying this problem, glandium, and for fixing the generated code, Emilio!

Looking at build times over the last 14 days, this fix restores incremental automation builds to their pre-Stylo build times:

https://treeherder.mozilla.org/perf.html#/graphs?series=%5Bautoland,4044b74c437dfc672f4615a746ea01f6e4c0312d,1,2%5D&series=%5Bautoland,be465112346255e89dd26003061b01f27cc7fd39,1,2%5D&series=%5Bautoland,7c6ec9c4d898dda7736ddd7f0a8ddb9bef63b3cd,1,2%5D&series=%5Bautoland,d416d6e2ae32b6d43bb1355394d31bfe4fbeee27,1,2%5D&series=%5Bautoland,18851c0bb8875e85890ba4b1aed2439f8c682743,1,2%5D&series=%5Bautoland,be7331d4f74b6970f67fe9da80f4d4d90ef60b73,1,2%5D

Comment 10

[Chris Peterson [:cpeterson]]

Emilio fixed this in comment 3.