Closed Bug 1455784 Opened 6 years ago Closed 6 years ago

Use a tagged union to shrink StyleSource

Categories

(Core :: CSS Parsing and Computation, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: bholley, Assigned: bholley)

Details

Attachments

(3 files, 1 obsolete file)

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.
MozReview-Commit-ID: Jxp2A7cj6CV
Attachment #8969836 - Flags: feedback?(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
Attachment #8969836 - Flags: feedback?(manishearth) → feedback+
Attached patch Part 1 - ArcUnion. v1 (obsolete) — Splinter Review
MozReview-Commit-ID: Jxp2A7cj6CV
Attachment #8970741 - Flags: review?(manishearth)
MozReview-Commit-ID: AT4sud9goGV
Attachment #8970742 - Flags: review?(manishearth)
MozReview-Commit-ID: Jxp2A7cj6CV
Attachment #8970743 - Flags: review?(manishearth)
Attachment #8970741 - Attachment is obsolete: true
Attachment #8970741 - Flags: review?(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]`
Attachment #8970743 - Flags: review?(manishearth) → review+
(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?
Oh, that's correct. Sorry.
Attachment #8970742 - Flags: review?(manishearth) → review+
https://hg.mozilla.org/mozilla-central/rev/5a517a7c8cf2
https://hg.mozilla.org/mozilla-central/rev/af621dc793aa
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.