Measure the heap memory usage of the remaining derived types of Element

[jdm]

#6874 added the basics for measuring the heap memory used by the DOM tree. However, the heap_size_of_eventtarget method in mem.rs is incomplete - it needs a variant for every single derived type of Node (Element, HTMLElement, HTMLTextAreaElement, etc.) What this means in practice is adding #[derive(HeapSizeOf)] to the type that needs measuring, adding a new match arm to heap_size_of_eventtarget, and adding an #[ignore_heap_size_of = "reason"] annotation to any fields that can't easily be measured (ie. types that are not defined in one of Servo's crates).

There's no particular reason why one person would need to do all of the types here, since there's a lot of them, but maybe somebody's keen. Otherwise, please leave a comment here listing the types you're intending to implement heap measurement for.

[jdm]

You can see the effects of your changes by running ./mach run -m 3 [url] and looking at the value of dom-tree in the output. New implementations for types with a lot of members could make the number increase meaningfully for sites containing lots of DOM nodes of that type!

[boghison]

I am keen, but I'll also leave a comment :)

[boghison]

@jdm So, basically, I should fill util/mem.rs with types, right? (Or should I impl them in their respective files?)

[jdm]

For a type T that does not currently implement HeapSizeOf, there are thee solutions:

• if it's defined a crate under servo/servo, add derive(HeapSizeOf) to it (or manually impl HeapSizeOf in the same module if it can't be automatically derived for some reason)
• if it's defined outside of servo/servo and it's highly unlikely to contain any pointers to other heap-allocated memory, add it to the list of known_heap_size macros in mem.rs
• if it's define outside of servo/servo and it's possible the type contains pointers to other heap-allocated memory (like Sender/Receiver/IpcSender/other complex types from the standard library), add a #[ignore_heap_size_of] annotation to the place where it's used

[boghison]

@jdm Do I ignore Root and JS?

[jdm]

JS<T> already has a size of 0. I'm super curious where we end up with Root values, though!

[boghison]

@jdm What about Trusted?

[jdm]

Interesting question. Just add an ignore attribute for now.

[boghison]

@jdm Just wanted to let you know that I only have 75 errors left, so you can expect a PR soon :3

[boghison]

Ok, I am almost done, I just have 2 more, which are very weird:
/home/boghison/Projects/servo/components/script/dom/htmlcollection.rs:31 Live(JS<Node>, Box<CollectionFilter+'static>), even though

#[ignore_heap_size_of = "Cannot calculate Heap size"]
Live(JS<Node>, Box<CollectionFilter+'static>)

and
/home/boghison/Projects/servo/components/script/dom/servohtmlparser.rs:50 pub form_elem: Option<&'a Node>

[jdm]

Yes, the first one is #6870. For the second, we probably don't have an implementation for &T where T: HeapSizeOf.

[jdm]

That being said, why is form_elem in something that needs to have HeapSizeOf derived?

[boghison]

@jdm Oh, ok. TBH I have no idea, I just wrote a script to add this to every struct and enum in script/dom, then ignore the types that need to be ignored, and then I manually fixed all errors. I'll write it for &T then, and maybe try to fix #6870. I also have a question, why did you say one needs a variant for every type derived of Node, couldn't it just be a Element(_) and then ElementCast or am I missing something?

[jdm]

We should not add an impl for &T. We should remove the annotation instead.

[jdm]

As for why we're doing this, the problem is that if we use ElementCast::to_ref and then call heap_size_of on it, all we're doing is calling Element::heap_size_of_children (ie. measuring the members of the Element struct). If we actually have an HTMLIFrameElement, we don't measure of its members because we don't call HTMLIFrameElement::heap_size_of_children.

[boghison]

@jdm Oh, ok. Could you elaborate on what you said first, what do you mean by "remove the annotation"? And am I doing this right, I mean, IMHO, it won't hurt if we add HeapSizeOf to as much as we can, no?

[jdm]

So on one hand, it won't hurt in the common case . On the other hand, in this case an implementation for &T where T: HeapSizeOf will silently do the wrong thing for something like &Element if we ever measure something that stores that. In this case, the struct being modified should never be reachable from a heap-allocated object, so the HeapSizeOf annotation is unnecessary, so we don't need a new impl.

[Ms2ger]

Surely we shouldn't count borrowed references?

[boghison]

Oh, maybe you misunderstood. I mean if I am doing everything right, by appending HeapSizeOf to as much as possible ie not about the Option<&Node>

[jdm]

I think the only downside is the existence of generated code that is never called, which will increase compile times just by existing. That being said, it's not clear to me how much of a burden that will be.