style: Move all origin-specific cascade data to PerOriginCascadeData #18017

heycam · 2017-08-09T07:48:47Z

This is a preliminary refactoring in preparation for only rebuilding cascade data for origins that have a change. https://bugzilla.mozilla.org/show_bug.cgi?id=1382925

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #__ (github issue number if applicable).

There are tests for these changes OR
These changes do not require tests because _____

This change is

highfive · 2017-08-09T07:48:51Z

Heads up! This PR modifies the following files:

@bholley: components/style/invalidation/element/invalidation_map.rs, components/style/invalidation/element/invalidator.rs, components/style/animation.rs, components/style/context.rs, components/style/stylist.rs
@canaltinova: components/style/invalidation/element/invalidation_map.rs, components/style/invalidation/element/invalidator.rs, components/style/animation.rs, components/style/context.rs, components/style/stylist.rs
@emilio: ports/geckolib/glue.rs, components/style/invalidation/element/invalidation_map.rs, components/style/invalidation/element/invalidator.rs, components/style/animation.rs, components/style/context.rs and 1 more

highfive · 2017-08-09T07:48:52Z

Warning

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

heycam · 2017-08-09T07:48:58Z

r? @emilio

emilio

Looks pretty straight-forward. r=me with the nits addressed, thanks!

emilio · 2017-08-09T08:04:58Z

components/style/stylist.rs

@@ -1635,6 +1635,8 @@ impl CascadeData {
    }
 }

+/// Iterator over `PerOriginCascadeData`, from highest level (user) to lowest
+/// (user agent).


Add a comment to note that we rely on that to get animations, and potentially other stuff?

emilio · 2017-08-09T08:07:30Z

components/selectors/bloom.rs

+    }
+}
+
+pub struct BloomStorageU8 {


needs docs.

emilio · 2017-08-09T08:08:54Z

components/selectors/bloom.rs

+
+    #[inline]
+    fn slot_is_empty(&self, index: usize) -> bool {
+        let bit = 1 << (index % 8);


nit: maybe create a method for this? presumably not worth the effort.

Yeah, I almost did, then decided it wasn't worth it.

emilio · 2017-08-09T08:11:14Z

components/selectors/bloom.rs

-    *slot == 0xff
+pub trait BloomStorage : Clone + Default {
+    fn slot_is_empty(&self, index: usize) -> bool;
+    fn adjust_slot(&mut self, index: usize, increment: bool);


Could you mention what are these functions expected to do?

I found adjust to be a little not too descriptive... But I can't think of a better name that isn't super-long, so, maybe something simple like:

Either increment or decrement the slot at `index`.

Would be helpful.

emilio · 2017-08-09T08:12:46Z

components/selectors/bloom.rs

@@ -237,6 +291,10 @@ fn hash2(hash: u32) -> u32 {
 fn create_and_insert_some_stuff() {
    let mut bf = BloomFilter::new();

+    assert!(ARRAY_SIZE % 8 == 0,


I think you can static-assert this with transmute or something like that...

transmute::<[u8; 1 + ARRAY_SIZE % 8], [u8; 1]>(..)

emilio · 2017-08-09T08:16:33Z

components/style/stylist.rs

+    /// Returns the number of entries in invalidation maps.
+    pub fn num_invalidations(&self) -> usize {
+        self.cascade_data.iter_origins()
+            .map(|d| d.invalidation_map.len()).sum()


nit: I'd move sum, and maybe iter_origins, to its own line, but no huge deal.

emilio · 2017-08-09T08:17:21Z

components/style/stylist.rs

-            self.style_attribute_dependency
+            self.cascade_data
+                .iter_origins()
+                .map(|d| d.style_attribute_dependency)


nit: just any(|d| d.style_attribute_dependency).

emilio · 2017-08-09T08:17:27Z

components/style/stylist.rs

+                    d.attribute_dependencies
+                        .might_contain_hash(local_name.get_hash())
+                })
+                .any(|b| b)


emilio · 2017-08-09T08:17:37Z

components/style/stylist.rs

+        self.cascade_data
+            .iter_origins()
+            .map(|d| d.state_dependencies.intersects(state))
+            .any(|b| b)


emilio · 2017-08-09T08:17:44Z

components/style/stylist.rs

+        self.cascade_data
+            .iter_origins()
+            .map(|d| d.mapped_ids.might_contain_hash(id.get_hash()))
+            .any(|b| b)


… hash table.

emilio · 2017-08-09T11:14:29Z

components/selectors/bloom.rs

+    #[inline]
+    fn adjust_slot(&mut self, index: usize, increment: bool) {
+        let slot = &mut self.counters[index];
+        if *slot == 0xff {  // full


Err, this needs to be !=. Travis is red because of this.

That is probably why my try run is all orange. :-)

…ing version.

heycam · 2017-08-10T00:26:58Z

@bors-servo r=emilio

bors-servo · 2017-08-10T00:26:59Z

📌 Commit 6cbe552 has been approved by emilio

bors-servo · 2017-08-10T00:28:09Z

⌛ Testing commit 6cbe552 with merge ca14848...

style: Move all origin-specific cascade data to PerOriginCascadeData  This is a preliminary refactoring in preparation for only rebuilding cascade data for origins that have a change. https://bugzilla.mozilla.org/show_bug.cgi?id=1382925 ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [ ] These changes fix #__ (github issue number if applicable).  - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/18017)

bors-servo · 2017-08-10T01:34:00Z

☀️ 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: emilio
Pushing ca14848 to master...

highfive assigned KiChjang Aug 9, 2017

highfive added the S-awaiting-review There is new code that needs to be reviewed. label Aug 9, 2017

highfive assigned emilio and unassigned KiChjang Aug 9, 2017

heycam force-pushed the split-cascade branch from 67ea659 to 120ae1f Compare August 9, 2017 07:55

emilio approved these changes Aug 9, 2017

View reviewed changes

heycam added 3 commits August 9, 2017 16:24

style: Make CascadeDataIter iterate from highest to lowest level.

6bdb0ab

style: Tweak Stylist API for getting animations to avoid exposing the…

6826822

… hash table.

style: Slight refactoring of TraversalStatistics::finish.

7f47ae0

heycam force-pushed the split-cascade branch from 120ae1f to ed244a0 Compare August 9, 2017 08:41

emilio reviewed Aug 9, 2017

View reviewed changes

heycam added 9 commits August 9, 2017 19:27

selectors: Genericize BloomFilter so we can easily define a non-count…

e294372

…ing version.

selectors: Add a non-counting Bloom filter type.

df2c8b2

selectors: Add a simple Debug impl for CountingBloomFilter.

b23f947

style: Use non-counting Bloom filters in Stylist where appropriate.

781e755

style: Move animations table into PerOriginCascadeData.

77c4a42

style: Move invalidation map into PerOriginCascadeData.

16937ba

style: Move attribute and state dependencies into PerOriginCascadeData.

30de56f

style: Move revalidation selectors into PerOriginCascadeData.

cd5b2c9

style: Move selector/declaration counts into PerOriginCascadeData.

6cbe552

heycam force-pushed the split-cascade branch from ed244a0 to 6cbe552 Compare August 9, 2017 11:28

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels Aug 10, 2017

bors-servo merged commit 6cbe552 into servo:master Aug 10, 2017

highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Aug 10, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

style: Move all origin-specific cascade data to PerOriginCascadeData #18017

style: Move all origin-specific cascade data to PerOriginCascadeData #18017

heycam commented Aug 9, 2017 •

edited by SimonSapin

highfive commented Aug 9, 2017

highfive commented Aug 9, 2017

heycam commented Aug 9, 2017

emilio left a comment

emilio Aug 9, 2017

emilio Aug 9, 2017

emilio Aug 9, 2017

heycam Aug 9, 2017

emilio Aug 9, 2017

emilio Aug 9, 2017

emilio Aug 9, 2017

emilio Aug 9, 2017

emilio Aug 9, 2017

emilio Aug 9, 2017

emilio Aug 9, 2017

emilio Aug 9, 2017

heycam Aug 9, 2017

heycam commented Aug 10, 2017

bors-servo commented Aug 10, 2017

bors-servo commented Aug 10, 2017

bors-servo commented Aug 10, 2017

style: Move all origin-specific cascade data to PerOriginCascadeData #18017

style: Move all origin-specific cascade data to PerOriginCascadeData #18017

Conversation

heycam commented Aug 9, 2017 • edited by SimonSapin

highfive commented Aug 9, 2017

highfive commented Aug 9, 2017

heycam commented Aug 9, 2017

emilio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

heycam commented Aug 10, 2017

bors-servo commented Aug 10, 2017

bors-servo commented Aug 10, 2017

bors-servo commented Aug 10, 2017

heycam commented Aug 9, 2017 •

edited by SimonSapin