1475191 - measure heap allocations hanging off selector components

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement, P3)

Comment 1

[Cameron McCormack (:heycam)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1a0c219e4001caafd0a1834239852e5afb9677bb

Comment 2

[Cameron McCormack (:heycam)]

Attachment: Bug 1475191 - Measure heap allocations hanging off selector components.

Review commit: https://reviewboard.mozilla.org/r/256486/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/256486/

Comment 3

[Emilio Cobos Álvarez (:emilio)]

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.

Comment 4

[Cameron McCormack (:heycam)]

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 5

[Cameron McCormack (:heycam)]

Comment on attachment 8991567 [details]
Bug 1475191 - Measure heap allocations hanging off selector components.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/256486/diff/1-2/

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/24576552e570
Measure heap allocations hanging off selector components. r=emilio

Comment 7

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/24576552e570