1455784 - Use a tagged union to shrink StyleSource

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement)

Description

[Bobby Holley (On Leave Through ~December)]

right now StyleSource is a three-variant enum, with one of them being "None" to avoid consuming a third word. We could turn this all into a one-word Option<ArcUnion<Rule, Declaration>> with the appropriate machinery.

I hacked together a quick implementation of ArcUnion. Haven't finished hooking it up to StyleSource, but looking for quick feedback on the approach and ergonomics.

Comment 1

[Bobby Holley (On Leave Through ~December)]

Attachment: ArcUnion prototype. v1

MozReview-Commit-ID: Jxp2A7cj6CV

Comment 2

[Manish Goregaokar [:manishearth]]

Comment on attachment 8969836 [details] [diff] [review]
ArcUnion prototype. v1

Review of attachment 8969836 [details] [diff] [review]:
-----------------------------------------------------------------

this looks pretty good.

I'd love to eventually publish ServoArc and/or get the Rust stdlib to expose enough so that it can be implemented compatibly with sync::Arc

::: servo/components/servo_arc/lib.rs
@@ +976,5 @@
> +    /// Similar to deref, but uses the lifetime |a| rather than the lifetime of
> +    /// self, which is incompatible with the signature of the Deref trait.
> +    #[inline]
> +    pub fn get(&self) -> &'a T {
> +        &*self.0

nit: this can just be self.0, immutable references are Copy

Comment 3

[Bobby Holley (On Leave Through ~December)]

Attachment: Part 1 - ArcUnion. v1

MozReview-Commit-ID: Jxp2A7cj6CV

Comment 4

[Bobby Holley (On Leave Through ~December)]

Attachment: Part 2 - Update StyleSource to use ArcUnion. v1

MozReview-Commit-ID: AT4sud9goGV

Comment 5

[Bobby Holley (On Leave Through ~December)]

Attachment: Part 1 - ArcUnion. v2

MozReview-Commit-ID: Jxp2A7cj6CV

Comment 6

[Bobby Holley (On Leave Through ~December)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6ea482e400c7c9aa9f1da32f6cb271984486c2bc

Comment 7

[Manish Goregaokar [:manishearth]]

Comment on attachment 8970743 [details] [diff] [review]
Part 1 - ArcUnion. v2

Review of attachment 8970743 [details] [diff] [review]:
-----------------------------------------------------------------

::: servo/components/servo_arc/lib.rs
@@ +1021,5 @@
> +    First(ArcBorrow<'a, A>),
> +    Second(ArcBorrow<'a, B>),
> +}
> +
> +impl<A: 'static, B: 'static> ArcUnion<A, B> {

might wish to mark some of these methods `#[inline]`

Comment 8

[Bobby Holley (On Leave Through ~December)]

(In reply to Manish Goregaokar [:manishearth] from comment #7)
> might wish to mark some of these methods `#[inline]`

I thought that was unnecessary given that they're generic?

Comment 9

[Manish Goregaokar [:manishearth]]

Oh, that's correct. Sorry.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5a517a7c8cf2
ArcUnion. r=Manishearth
https://hg.mozilla.org/integration/autoland/rev/af621dc793aa
Update StyleSource to use ArcUnion. r=Manishearth

Comment 11

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/5a517a7c8cf2
https://hg.mozilla.org/mozilla-central/rev/af621dc793aa