Start reporting memory usage for Window and all nodes in all DOM tree…

[jdm]

…s for frame treese in script tasks.

This underreports by a significant amount, since only Document, Window and CharacterData (ie. text) nodes are fully represented. That being said, every HTML element in the tree is measured, but only counted as a Node. It's easy to improve this, it just requires adding the appropriate HeapSizeOf derives and increasing the granularity of measure_memory_for_eventtarget. google.com shows a dom-tree value of 0.24 MB for me at the moment.

r? @nnethercote

[bors-servo]

☔ The latest upstream changes (presumably #6586) made this pull request unmergeable. Please resolve the merge conflicts.

[nnethercote]

Cool beans, dude. I have a bunch of suggestions but they're all minor.

I shudder to think how much larger an equivalent patch for Firefox would be. This HeapSizeOf deriving business is sweet.

You've used FIXME throughout. When I did that in the past I was asked to change it to FIXME(njn). You've also used TODO in a few places. I don't know if there's a preferred Servo style, so I'll leave it up to you to decide what to do.

---

Review status: 0 of 33 files reviewed at latest revision, 15 unresolved discussions, some commit checks failed.

---

^{components/script/dom/bindings/cell.rs, line 20 [r1] (raw file):}
Any reason you didn't do #[derive(Clone, HeapSizeOf)], as is done elsewhere?

---

^{components/script/dom/bindings/js.rs, line 48 [r1] (raw file):}
Nit: why two blank lines?

---

^{components/script/dom/bindings/js.rs, line 245 [r1] (raw file):}
I won't pretend to understand this one, and will take it on faith that you know what you are doing.

---

^{components/script/dom/bindings/utils.rs, line 431 [r1] (raw file):}
"defined and measured in rust-mozjs"?

---

^{components/script/dom/browsercontext.rs, line 37 [r1] (raw file):}
Ditto?

---

^{components/script/dom/browsercontext.rs, line 94 [r1] (raw file):}
Because heap_size_of_children for JS<T> always returns 0, we won't measure the Document's children. Is this intentional?

---

^{components/script/dom/window.rs, line 171 [r1] (raw file):}
"defined and measured"?

---

^{components/script/memory.rs, line 5 [r1] (raw file):}
Other similar modules are called mem.rs, not memory.rs. I'll leave it up to you whether you want to follow suit or blaze your own trail.

---

^{components/script/memory.rs, line 15 [r1] (raw file):}
Given that this is very similar to HeapSizeOf, I think the name should reflect that similarity. It should definitely have Heap in it to make it clear that it's on the heap. (I have a pet peeve against functions with generic names like "measure memory" because usually you can be more specific.)

How about HeapSizeOfSelf?

A comment about the purpose of this trait would also be useful. AFAICT it's for measuring heap-allocated things that aren't Box<T>, is that right? (So HeapSizeOfNonBox might also work for the name?)

---

^{components/script/memory.rs, line 23 [r1] (raw file):}
Similarly, something like heap_size_of_event_target would be better. Having said that, is there a reason EventTarget can't implement the HeapSizeOf trait?

---

^{components/script/script_task.rs, line 1118 [r1] (raw file):}
I'd be interested to see example output showing the new measurements.

---

^{components/script/timers.rs, line 117 [r1] (raw file):}
"defined and measured"?

---

^{components/style/values.rs, line 109 [r1] (raw file):}
Is cssparser::Color a scalar? If so, it seems like the kind of thing that could be added to the known_heap_size!(0, ...) macro invocations in components/util/mem.rs. (Looking further down, I see you've actually done this. So this annotation should be removable.)

---

^{components/util/mem.rs, line 144 [r1] (raw file):}
This will be an underestimate because it's ignoring the empty entries. Though it's hard to do better without pushing HeapSizeOf into the language so that built-in types can use it. (Unless you want to emulate the monstrosity that is LinkedList2!) Can you extend the comment to mention the underestimate?

---

^{components/util/mem.rs, line 145 [r1] (raw file):}
I found buckets a confusing name for this variable. Maybe just size would be better?

---

Comments from the review on Reviewable.io

[jdm]

Thanks for the review!

---

Review status: 0 of 33 files reviewed at latest revision, 15 unresolved discussions, some commit checks failed.

---

^{components/script/dom/browsercontext.rs, line 94 [r1] (raw file):}
Hmm, I didn't think about this. JS<T> is like Rc<T>, in that there's not any clear owner much of the time. Maybe we should introduce a ProbableOwner<T> that wraps a JS<T> and forces the containing value to be measured, but that could certainly lead into overcounting if not done carefully. I'm inclined to say that for cases like this we'll want an explicit heap_usage_of_session_history that iterates over all reachable documents and computes their memory usage, rather than trying to get it for free by changing the way we process JS<T> in general.

---

^{components/script/memory.rs, line 15 [r1] (raw file):}
Yeah, this is used to measure objects that were allocated inside of a Box<T> but later had those containers forgotten so we could stash the pointer elsewhere (like in a reserved slot on a reflector). Now that you mention it I do see the similarity here.

---

^{components/script/memory.rs, line 23 [r1] (raw file):}
EventTarget does implement HeapSizeOf, but this allows us to measure any DOM interface that derives from EventTarget too. heap_size_of will give us the heap allocation for the self pointer, which is fine, but we need to ensure that we end up calling the correct implementation of heap_size_of_children for the actual derived type that we end up measuring, hence all this downcasting that's necessary.

---

^{components/util/mem.rs, line 144 [r1] (raw file):}
capacity is the total of empty and non-empty entries. And I looked into duplicating the internals like we do with linked lists, but they're actually terrifying and incomprehensible.

---

Comments from the review on Reviewable.io

[nnethercote]

capacity is the total of empty and non-empty entries.

Ok, that's probably a pretty good estimate, then.

[jdm]

I keep going back and forth on this ProbableOwner<T> thing, which would allow us to automatically measure stuff like the list of HTMLCollection objects in Document, the HTML parser, the various JS<T> pointers that hang off of Window...

[nnethercote]

I suggest sticking with 0 for JS<T> for now, and give the ProbableOwner some more thinking time.

[jdm]

Updated! All comments should have been addressed; I care less about associating a name with TODO/FIXME than I do filing issues and attaching those instead.

[jdm]

Sample output for google.com:

|   22.48 MiB -- explicit
|      12.85 MiB -- jemalloc-heap-unclassified
|       5.87 MiB -- url(https://www.google.ca/?gfe_rd=cr&ei=A_m-VZz_A-KM8QefxabQDA)
|          5.62 MiB -- js
|             2.31 MiB -- malloc-heap
|             2.00 MiB -- gc-heap
|                1.43 MiB -- used
|                0.43 MiB -- decommitted
|                0.09 MiB -- unused
|                0.05 MiB -- admin
|             1.31 MiB -- non-heap
|          0.25 MiB -- dom-tree
|       3.73 MiB -- compositor-task
|          1.86 MiB -- surface-map
|          1.86 MiB -- layer-tree
|       0.03 MiB -- url(http://google.com/)
|          0.03 MiB -- layout-task
|             0.03 MiB -- display-list
|             0.00 MiB -- local-context
|          0.00 MiB -- layout-worker-0-local-context
|          0.00 MiB -- layout-worker-1-local-context
|          0.00 MiB -- layout-worker-2-local-context
|          0.00 MiB -- layout-worker-3-local-context
|          0.00 MiB -- layout-worker-4-local-context
|          0.00 MiB -- layout-worker-5-local-context
|
|   15.12 MiB -- jemalloc-heap-active
|   13.13 MiB -- jemalloc-heap-allocated
|  140.00 MiB -- jemalloc-heap-mapped
|  132.80 MiB -- resident
| 3045.86 MiB -- vsize

[jdm]

Sample output for the html5 spec index page:

|   29.79 MiB -- explicit
|      18.74 MiB -- jemalloc-heap-unclassified
|       6.37 MiB -- url(https://html.spec.whatwg.org/multipage/)
|          4.19 MiB -- js
|             2.00 MiB -- gc-heap
|                0.95 MiB -- used
|                0.89 MiB -- decommitted
|                0.10 MiB -- unused
|                0.05 MiB -- admin
|             1.13 MiB -- malloc-heap
|             1.06 MiB -- non-heap
|          2.08 MiB -- dom-tree
|          0.10 MiB -- layout-task
|             0.10 MiB -- display-list
|             0.00 MiB -- local-context
|          0.00 MiB -- layout-worker-0-local-context
|          0.00 MiB -- layout-worker-1-local-context
|          0.00 MiB -- layout-worker-2-local-context
|          0.00 MiB -- layout-worker-3-local-context
|          0.00 MiB -- layout-worker-4-local-context
|          0.00 MiB -- layout-worker-5-local-context
|       4.69 MiB -- compositor-task
|          2.34 MiB -- surface-map
|          2.34 MiB -- layer-tree
|
|   23.61 MiB -- jemalloc-heap-active
|   20.91 MiB -- jemalloc-heap-allocated
|  136.00 MiB -- jemalloc-heap-mapped
|  138.90 MiB -- resident
| 3058.39 MiB -- vsize

[nnethercote]

Reviewed 33 of 33 files at r1, 24 of 24 files at r2.
Review status: all files reviewed at latest revision, all discussions resolved, all commit checks successful.

---

^{components/util/mem.rs, line 144 [r1] (raw file):}
It's probably still an underestimate because each entry will need space to store the hash value. And then maybe space for padding. Still, hard to do better here.

---

Comments from the review on Reviewable.io

[nnethercote]

Looks great! r=me, or r+, or whatever I'm supposed to type, not that I have the appropriate privileges to talk to Bors, or Homu, or whatever it's called. Whatever!

[jdm]

@bors-servo: r=njn

[bors-servo]

📌 Commit 1b1304a has been approved by njn

[jdm]

@bors-servo: r-
I'll just rebase over #6932 instead.

[bors-servo]

☔ The latest upstream changes (presumably #6784) made this pull request unmergeable. Please resolve the merge conflicts.

[jdm]

@bors-servo: r=njn

[bors-servo]

📌 Commit 8620fe5 has been approved by njn

[bors-servo]

⌛ Testing commit 8620fe5 with merge 84e25be...

[bors-servo]

☀️ Test successful - android, gonk, linux1, linux2, linux3, mac1, mac2, mac3