Wire up the JS engine's memory reporting.

[nnethercote]

SpiderMonkey provides an extremely fine-grained breakdown of memory
usage, but for Servo we aggregate the measurements into a small number
of coarse buckets, which seems appropriate for the current level of
detail provided by Servo's memory profiler. Sample output:

|      17.41 MiB -- url(file:///home/njn/moz/servo/../servo-static-suite/wikipedia/Guardians%20of%20the%20Galaxy%20(film)%20-%20Wikipedia,%20the%20free%20encyclopedia.html)
|          7.32 MiB -- js
|             3.07 MiB -- malloc-heap
|             3.00 MiB -- gc-heap
|                2.48 MiB -- used
|                0.34 MiB -- decommitted
|                0.09 MiB -- unused
|                0.09 MiB -- admin
|             1.25 MiB -- non-heap

Most of the changes are plumbing to get the script task communicating
with the memory profiler task.

[highfive]

Warning

• These commits modify unsafe code. Please review it carefully!

[hoppipolla-critic-bot]

Critic review: https://critic.hoppipolla.co.uk/r/5487

This is an external review system which you may optionally use for the code review of your pull request.

In order to help critic track your changes, please do not make in-place history rewrites (e.g. via git rebase -i or git commit --amend) when updating this pull request.

[nnethercote]

@jdm, this review has your name all over it :)

I'm actually going to have to land patches in four repositories before this is done.

• An API change to SpiderMonkey;
• Backport that to mozjs;
• Add appropriate glue to rust-mozjs;
• Use the glue in Servo (i.e. this change).

I'm putting this change up for review first because it's driving the other changes. If any modifications are needed here it'll be best to work them out here first and then back-propagate them as necessary.

I'm a little worried about testing all this. I can do a certain amount of local testing (e.g. I've done Linux and Mac) but I'm worried about the following case.

• I get r+ on all the changes;
• I land the changes in the first three repos;
• I try to land the changes in the fourth repo (Servo) but I get some kind of CI test failure that doesn't occur locally;
• I potentially then have to diagnose and fix that failure by modifying my changes in all four repos.

That would be truly painful, but I can't see a way to CI-test all of my changes in advance. Hopefully it won't come to that.

[michaelwu]

I'd like to see the rust-mozjs changes.

[nnethercote]

I'm actually going to have to land patches in four repositories before this is done.

• An API change to SpiderMonkey;

This is https://bugzilla.mozilla.org/show_bug.cgi?id=1181452.

• Backport that to mozjs;

This is servo/mozjs#49

• Add appropriate glue to rust-mozjs;

This is servo/rust-mozjs#177

• Use the glue in Servo (i.e. this change).

[jdm]

It would be useful to hook up the worker threads to the memory reporting, too: http://mxr.mozilla.org/servo/source/components/script/dom/dedicatedworkerglobalscope.rs and http://mxr.mozilla.org/servo/source/components/script/dom/workerglobalscope.rs are where those are implemented.

-S-awaiting-review +S-needs-code-changes

---

Reviewed 6 of 6 files at r1.
Review status: all files reviewed at latest revision, 5 unresolved discussions, all commit checks successful.

---

^{components/script/script_task.rs, line 286 [r1] (raw file):}
Use no_jsmanaged_fields in trace.rs instead.

---

^{components/script/script_task.rs, line 295 [r1] (raw file):}
This is a bit misleading. A script task can contain multiple pages (eg. via same-origin iframes), and in the future we'll have a model of a script task containing all in-memory pages for a particular eTLD.

---

^{components/script/script_task.rs, line 528 [r1] (raw file):}
If you move this into ScriptTask::start we don't need to store the reporter name as a member.

---

^{components/script/script_task.rs, line 731 [r1] (raw file):}
I'd prefer this in ScriptTask::start.

---

^{components/script_traits/lib.rs, line 149 [r1] (raw file):}
We could extract this from the LoadData that's already passed instead.

---

Comments from the review on Reviewable.io

[nnethercote]

Review status: all files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.

---

^{components/script/script_task.rs, line 295 [r1] (raw file):}
Is it just the comment you dislike, or should I find a different way to distinguish/label a script task? Other memory (e.g. layout) is currently reported on a page basis, and it would be a shame if JS memory had to be done differently.

---

Comments from the review on Reviewable.io

[nnethercote]

If you move this into ScriptTask::start we don't need to store the reporter name as a member.

Hmm, the same simplification could be applied to the existing reporters for the Layout and Paint tasks. I'll add that to my todo list.

[nnethercote]

I've pushed my in-progress changes. I addressed all the comments except the one about hooking up workers; I'll do that tomorrow. And @jdm requested compartment-level reporting which I will consider (I'll need to think about compartments vs. zones...)

[nnethercote]

I've hooked up workers now. I still haven't done anything about compartment-level reporting; my enthusiasm level for doing that is low.

[jdm]

-S-awaiting-review +S-needs-code-changes
I'd rather see this code merge as-is than have it wait for the compartment-level stuff if you're that unenthused.

---

Reviewed 4 of 5 files at r2, 6 of 6 files at r3.
Review status: all files reviewed at latest revision, 3 unresolved discussions, all commit checks successful.

---

^{components/script/dom/bindings/global.rs, line 90 [r3] (raw file):}
I'm pretty sure this will trigger if we spawn a worker inside of another worker.

---

^{components/script/script_task.rs, line 1030 [r3] (raw file):}
I think for it_page in &self.root_page() should work.

---

^{components/script/script_task.rs, line 1031 [r3] (raw file):}
urls.push(it_page.document().url().serialize());

---

Comments from the review on Reviewable.io

[nnethercote]

Review status: all files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.

---

^{components/script/script_task.rs, line 1030 [r3] (raw file):}
I tried that previously, but rustc says: "error: the trait core::iter::Iterator is not implemented for the type &alloc::rc::Rc<page::Page>". I also tried without the & and with a '*' and got much the same error. Page does not implement Iterator, but Rc<Page> does implement IterablePage, which requires the use of iter().

---

Comments from the review on Reviewable.io

[nnethercote]

Ok! All review comments are addressed. But this still needs to wait for the mozjs and rust-mozjs changes to land first.

[bors-servo]

⌛ Testing commit d70fdc5 with merge 800c090...

[bors-servo]

💔 Test failed - mac1

[jdm]

17:35.11 PROCESS_OUTPUT: Thread-TestrunnerManager-2 (pid:27325) Full command: /Users/servo/buildbot/slave/mac1/build/target/debug/servo --cpu --hard-fail -u Servo/wptrunner -z http://localhost:8000/workers/semantics/multiple-workers/002.html
(pid:27325) "thread 'Memory profiler' panicked at 'RegisterReporter: 'worker-reporter-0' name is already in use', /Users/servo/buildbot/slave/mac1/build/components/profile/mem.rs:87"
17:35.11 PROCESS_OUTPUT: Thread-TestrunnerManager-2 (pid:27325) "stack backtrace:"
17:35.11 PROCESS_OUTPUT: Thread-TestrunnerManager-2 (pid:27325) "   1:        0x10f99f9d5 - ALERT: RESULT: {"tests":[{"name":"creating 3 sibling dedicated workers","status":0,"message":null,"stack":null}],"status":0,"message":null,"stack":null}"
17:35.11 PROCESS_OUTPUT: Thread-TestrunnerManager-2 (pid:27325) "sys::backtrace::write::hde88a3e3a343e9f8zms"
17:35.11 PROCESS_OUTPUT: Thread-TestrunnerManager-2 (pid:27325) "   2:        0x10f9a2fc5 - panicking::on_panic::h8c73c8f1e33f6d60hIw"
17:35.11 PROCESS_OUTPUT: Thread-TestrunnerManager-2 (pid:27325) "   3:        0x10f990ab2 - rt::unwind::begin_unwind_inner::h781ff1cd39e7f47etqw"
17:35.11 PROCESS_OUTPUT: Thread-TestrunnerManager-2 (pid:27325) "   4:        0x10d2e1385 - rt::unwind::begin_unwind::h389154536123609620"
17:35.11 PROCESS_OUTPUT: Thread-TestrunnerManager-2 (pid:27325) "   5:        0x10d2d7853 - mem::Profiler::handle_msg::hef9799da9a1e7247Zca"
17:35.11 PROCESS_OUTPUT: Thread-TestrunnerManager-2 (pid:27325) "   6:        0x10d2ced4e - mem::Profiler::start::h457c0bdc01213071Aca"
17:35.12 PROCESS_OUTPUT: Thread-TestrunnerManager-2 (pid:27325) "   7:        0x10d2ceb45 - mem::Profiler::create::closure.5520"
17:35.12 PROCESS_OUTPUT: Thread-TestrunnerManager-2 (pid:27325) "   8:        0x10d2cea1e - task::spawn_named::closure.5514"
17:35.12 PROCESS_OUTPUT: Thread-TestrunnerManager-2 (pid:27325) "   9:        0x10d2ce975 - boxed::F.FnBox<A>::call_box::h3233880008695368661"
17:35.12 PROCESS_OUTPUT: Thread-TestrunnerManager-2 (pid:27325) "  10:        0x10d2cb3b0 - boxed::Box<FnBox<A, Output $u3d$$u20$R$GT$$u2b$$u20$Send$u20$$u2b$$u20$$u27$a$GT$.FnOnce$LT$A$GT$::call_once::h12673241061259117211"
17:35.12 PROCESS_OUTPUT: Thread-TestrunnerManager-2 (pid:27325) "  11:        0x10d2cae22 - thread::Builder::spawn_inner::closure.5367"
17:35.12 PROCESS_OUTPUT: Thread-TestrunnerManager-2 (pid:27325) "  12:        0x10d2cad9e - rt::unwind::try::try_fn::__rust_abi::h12586371467759283055"
17:35.12 PROCESS_OUTPUT: Thread-TestrunnerManager-2 (pid:27325) "  13:        0x10d2cad39 - rt::unwind::try::try_fn::h12586371467759283055"
17:35.12 PROCESS_OUTPUT: Thread-TestrunnerManager-2 (pid:27325) "  14:        0x10f9a49c8 - rust_try_inner"
17:35.13 PROCESS_OUTPUT: Thread-TestrunnerManager-2 (pid:27325) "  15:        0x10f9a49b5 - rust_try"
17:35.13 PROCESS_OUTPUT: Thread-TestrunnerManager-2 (pid:27325) "  16:        0x10f99df75 - rt::unwind::try::inner_try::h9e58480e93edc0cdmmw"
17:35.13 PROCESS_OUTPUT: Thread-TestrunnerManager-2 (pid:27325) "  17:        0x10d2cac98 - rt::unwind::try::h17192558137596237202"
17:35.13 PROCESS_OUTPUT: Thread-TestrunnerManager-2 (pid:27325) "  18:        0x10d2caad1 - thread::Builder::spawn_inner::closure.5317"
17:35.13 PROCESS_OUTPUT: Thread-TestrunnerManager-2 (pid:27325) "  19:        0x10d2cb73d - boxed::F.FnBox<A>::call_box::h14907357354884052942"
17:35.14 PROCESS_OUTPUT: Thread-TestrunnerManager-2 (pid:27325) "  20:        0x10f9a198d - sys::thread::Thread::new::thread_start::hf153915745363fa7wKv"
17:35.14 PROCESS_OUTPUT: Thread-TestrunnerManager-2 (pid:27325) "  21:     0x7fff94cef267 - _pthread_body"
17:35.14 PROCESS_OUTPUT: Thread-TestrunnerManager-2 (pid:27325) "  22:     0x7fff94cef1e4 - _pthread_start"

Whoops, we should probably fix that. Let's use the worker id (http://mxr.mozilla.org/servo/source/components/script/dom/worker.rs#86) and append that to the pipeline id so we get globally-unique names.

[jdm]

Actually, that's probably not good enough, since we've got separate "next_worker_id" values in Window and WorkerGlobalScope, so a name like "worker-reporter-{pipeline}-{worker_id}" yields "worker-reporter-0-0" for a toplevel worker, then "worker-reporter-0-0" for the first subworker too.

[jdm]

@bors-servo: r+

[bors-servo]

📌 Commit 7429b90 has been approved by jdm

[bors-servo]

⌛ Testing commit 7429b90 with merge 2b0bdbe...

[bors-servo]

💔 Test failed - mac1

[nnethercote]

websockets/constructor/004.html failed with "thread 'LayoutTask PipelineId(0)' panicked at 'could not initialize thread_rng: Too many open files (os error 24)', src/libstd/rand/mod.rs:161"

Although my latest revision did add calls to rand::random() this doesn't feel like it's fundamentally my fault :(

[nnethercote]

Looks like taht thread_rng panic has been seen before in #6350.

[mbrubeck]

@bors-servo retry

[bors-servo]

⚡ Previous build results for android, gonk, linux1, linux2, linux3, mac2, mac3 are reusable. Rebuilding only mac1...

[bors-servo]

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