measure heap allocations hanging off selector components

RESOLVED FIXED in Firefox 63

Status

()

enhancement
P3
normal
RESOLVED FIXED
10 months ago
10 months ago

People

(Reporter: heycam, Assigned: heycam)

Tracking

unspecified
mozilla63
Points:
---

Firefox Tracking Flags

(firefox63 fixed)

Details

Attachments

(1 attachment)

Comment hidden (empty)

Comment 3

10 months ago
mozreview-review
Comment on attachment 8991567 [details]
Bug 1475191 - Measure heap allocations hanging off selector components.

https://reviewboard.mozilla.org/r/256486/#review263366

r=me with those, thanks!

::: servo/components/malloc_size_of/lib.rs:699
(Diff revision 1)
>  }
>  
> +impl<Impl: selectors::parser::SelectorImpl> MallocSizeOf
> +    for selectors::parser::Selector<Impl>
> +where
> +    <Impl as selectors::parser::SelectorImpl>::NonTSPseudoClass: MallocSizeOf,

I think you can simplify this as:

```
where
    Impl::NonTSPseudoClass: MallocSizeOf,
    // ...
```

::: servo/components/malloc_size_of/lib.rs:705
(Diff revision 1)
> +    <Impl as selectors::parser::SelectorImpl>::PseudoElement: MallocSizeOf,
> +{
> +    fn size_of(&self, ops: &mut MallocSizeOfOps) -> usize {
> +        let mut n = 0;
> +
> +        // It's safe to measure this ThinArc directly because it's the

s/safe/ok/, right?

::: servo/components/malloc_size_of/lib.rs:720
(Diff revision 1)
> +}
> +
> +impl<Impl: selectors::parser::SelectorImpl> MallocSizeOf
> +    for selectors::parser::Component<Impl>
> +where
> +    <Impl as selectors::parser::SelectorImpl>::NonTSPseudoClass: MallocSizeOf,

ditto

::: servo/components/malloc_size_of/lib.rs:743
(Diff revision 1)
> +                selector.size_of(ops)
> +            }
> +            Component::PseudoElement(ref pseudo) => {
> +                (*pseudo).size_of(ops)
> +            }
> +            _ => 0,

hmm, probably we should list all the components explicitly, so that adding new components don't slip through? Though probably not really important.
Attachment #8991567 - Flags: review?(emilio) → review+
(Assignee)

Comment 4

10 months ago
mozreview-review-reply
Comment on attachment 8991567 [details]
Bug 1475191 - Measure heap allocations hanging off selector components.

https://reviewboard.mozilla.org/r/256486/#review263366

> s/safe/ok/, right?

Sure.  (I was just moving the comment from style_Rules.rs.)

> hmm, probably we should list all the components explicitly, so that adding new components don't slip through? Though probably not really important.

Yeah, I was wondering whether to do that.  Since you mention it I will. :-)

(It makes me a bit sad that we can't have the MallocSizeOf implementation in the selectors crate itself.  Then we could derive it...)
Comment hidden (mozreview-request)

Comment 6

10 months ago
Pushed by cam@mcc.id.au:
https://hg.mozilla.org/integration/autoland/rev/24576552e570
Measure heap allocations hanging off selector components. r=emilio

Comment 7

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/24576552e570
Status: ASSIGNED → RESOLVED
Last Resolved: 10 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.