Shrink RuleNode by two words

[bholley]

RuleNode is a frequently-allocated data structure. It lives in components/style/rule_tree/mod.rs

Right now it's 96 bytes on 64-bit platforms. We can shrink this to 80 quite easily.

We should do the following:
(1) Add a size_of test for RuleNode in tests/unit/stylo/size_of.rs.
(2) Change StyleSource to have a "None" variant, and eliminate the Option<> around StyleSource.
(3) Reuse a sibling pointer for free_count, as described in the comment.

CC @nox @jdm @emilio @SimonSapin

[highfive]

Hi! If you have any questions regarding this issue, feel free to make a comment here, or ask it in the #servo channel in IRC.

If you intend to work on this issue, then add @highfive: assign me to your comment, and I'll assign this to you. 😄

[highfive]

cc @emilio

[emilio]

(2) Change StyleSource to have a "None" variant, and eliminate the Option<> around StyleSource.

This wouldn't shrink anything, AFAICT, would it? StyleSource contains an Arc, which contains NonZero.

[bholley]

This wouldn't shrink anything, AFAICT, would it? StyleSource contains an Arc,
which contains NonZero.

It would, because the NonZero optimization doesn't extend into nested enums: https://is.gd/lviB8t

I did indeed verify that Option is 24 bytes.

[br-lewis]

I'm interested in working on this.
@highfive: assign me

[highfive]

Hey @zuwow! Thanks for your interest in working on this issue. It's now assigned to you!

[bholley]

Great! Let me know if you have any questions.

[br-lewis]

I'm not sure what to do about the read function on the StyleSource struct.
https://github.com/servo/servo/blob/master/components/style/rule_tree/mod.rs#L96
With the addition of a None variant, what should block be? Or should that be handled at the caller like it is now with the option?

[bholley]

Right - it should just panic, and the callers should check is_none().

[br-lewis]

Makes sense. To be clear then, you're thinking that StyleSource would look like

enum StyleSource  {
    Style(...),
    Declarations(...),
    None
}

and then I'd add an is_none() function to StyleSource . Any trait I should use for that?

Sorry, my rust is a bit uh... rusty.

[jdm]

No need for a trait. You can use a bare impl StyleSource to add the method.

[br-lewis]

I was able to remove the option and refactor everything such that RuleNode is now 88 bytes and passing the unit tests. Though I don't know what to do for ptr_equals in the case where they're both None since there's no reference to check. Have it return true, leave it as is and have it be false, or have it panic?

For free_count though I'm a little confused. First, I'm not sure what your proposed fix from the comment would entail but I probably don't have enough context on this whole module. Second, I don't think free_count is doing anything. None of the usages I could find in the code involved modifying its value besides when it's initialized and reset once a gc happens.

❯ git grep -n free_count
components/style/rule_tree/mod.rs:546:    free_count: AtomicUsize,
components/style/rule_tree/mod.rs:568:            free_count: AtomicUsize::new(0),
components/style/rule_tree/mod.rs:583:            free_count: AtomicUsize::new(0),
components/style/rule_tree/mod.rs:913:        me.free_count.store(0, Ordering::Relaxed);
components/style/rule_tree/mod.rs:920:        if self.get().free_count.load(Ordering::Relaxed) > RULE_TREE_GC_INTERVAL {

When it was first added (commit), it was being modified but that line was removed here. It looks like it might be possible to simply remove it.