Pass new method in CollectServoSizes to get size of ObjectPrivateVisitor #20430

aeweston98 · 2018-03-26T01:53:27Z

These changes are the servo changes to allow the measurement of DOM objects with the ObjectPrivateVisitor API. Two new functions have been added MemoryReporter and get_size, which are passed as function pointers to two updated mozjs functions. Currently these changes rely on a branch version of mozjs, which has an open pull request.

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #7084 (github issue number if applicable).

There are tests for these changes OR
These changes do not require tests because _____

This change is

highfive · 2018-03-26T01:53:33Z

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @nox (or someone else) soon.

highfive · 2018-03-26T01:53:34Z

Heads up! This PR modifies the following files:

@asajeffrey: components/script/lib.rs, components/script/Cargo.toml, components/script/script_thread.rs, components/script/script_runtime.rs
@fitzgen: components/script/lib.rs, components/script/Cargo.toml, components/script/script_thread.rs, components/script/script_runtime.rs
@KiChjang: components/script/lib.rs, components/script/Cargo.toml, components/script/script_thread.rs, components/script/script_runtime.rs

highfive · 2018-03-26T01:53:35Z

Warning

These commits modify unsafe code. Please review it carefully!
These commits modify script code, but no tests are modified. Please consider adding a test!

jdm

Please squash these commits as well.

jdm · 2018-03-26T02:51:19Z

components/script/lib.rs

@@ -199,6 +202,21 @@ pub fn init_service_workers(sw_senders: SWManagerSenders) {
    ServiceWorkerManager::spawn_manager(sw_senders);
 }

+#[no_mangle]
+#[allow(unsafe_code)]
+pub unsafe extern "C" fn MemoryReporter(obj: *mut JSObject) -> bool {


Let's call this is_dom_object.

jdm · 2018-03-26T02:51:19Z

components/script/lib.rs

+    if obj.is_null() {
+        return false;
+    }
+    if is_platform_object(obj) || is_dom_proxy(obj) {


This can be simplified to:

pub unsafe extern "C" fn MemoryReporter(obj: *mut JSObject) -> bool { !obj.is_null() && (is_platform_object(obj) || is_dom_proxy(obj)) }

jdm · 2018-03-26T02:51:19Z

components/script/lib.rs

@@ -207,6 +225,8 @@ pub fn init() {
        // Create the global vtables used by the (generated) DOM
        // bindings to implement JS proxies.
        RegisterBindings::RegisterProxyHandlers();
+
+        js::glue::InitializeMemoryReporter(serde::export::Some(MemoryReporter));


There should be no need for serde::export:: here.

jdm · 2018-03-26T02:51:19Z

components/script/script_runtime.rs

+#[no_mangle]
+#[allow(unsafe_code)]
+pub unsafe extern "C" fn get_size(obj: *mut JSObject) -> usize {
+        let dom_class_opt = get_dom_class(obj);


This function looks like it's over-indented by 4 spaces.

jdm · 2018-03-26T02:51:19Z

components/script/script_runtime.rs

+pub unsafe extern "C" fn get_size(obj: *mut JSObject) -> usize {
+        let dom_class_opt = get_dom_class(obj);
+
+        match dom_class_opt {


We should be able to do match get_dom_class(obj) {.

jdm · 2018-03-26T02:51:19Z

components/script/script_runtime.rs

+
+        match dom_class_opt {
+            Ok(v) => {
+                let pass_ptr = private_from_object(obj) as *const c_void;


Let's call this dom_object.

jdm · 2018-03-26T02:51:19Z

components/script/script_runtime.rs

+        match dom_class_opt {
+            Ok(v) => {
+                let pass_ptr = private_from_object(obj) as *const c_void;
+                if !(pass_ptr.is_null()) {


Let's write this as:

if dom_object.is_null() { return 0; } let mut ops = ...;

jdm · 2018-03-26T02:51:19Z

components/script/script_runtime.rs

+                let pass_ptr = private_from_object(obj) as *const c_void;
+                if !(pass_ptr.is_null()) {
+                    let mut ops = MallocSizeOfOps::new(::servo_allocator::usable_size, None, None);
+                    let _result = (v.malloc_size_of)(&mut ops, pass_ptr);


We can return this directly instead of storing it in a local variable.

jdm · 2018-03-26T02:51:19Z

components/script/script_runtime.rs

 #[allow(unsafe_code)]
 pub fn get_reports(cx: *mut JSContext, path_seg: String) -> Vec<Report> {
    let mut reports = vec![];

    unsafe {
        let rt = JS_GetRuntime(cx);
        let mut stats = ::std::mem::zeroed();
-        if CollectServoSizes(rt, &mut stats) {
+        if CollectServoSizes(rt, &mut stats, serde::export::Some(get_size)) {


We should be able to remove the serde::export::.

jdm · 2018-03-26T02:51:19Z

components/script/script_thread.rs

-        // malloc_enclosing_size_of function.
-        let mut ops = MallocSizeOfOps::new(::servo_allocator::usable_size, None, None);
-
-        for (_, document) in self.documents.borrow().iter() {


Let's keep the code that modifies path_seg.

jdm · 2018-03-26T02:51:42Z

I'm excited to see these changes almost ready!

aeweston98 · 2018-03-27T01:55:04Z

@jdm I rebased from the latest changes in master, updated according to your comments and squashed the commits. I saw one flag in the change view of cargo.toml which says no newline at end of file, is this something I need to change?

jdm · 2018-03-27T02:05:41Z

No, that should be fine.

jdm · 2018-03-27T02:13:06Z

components/script/script_runtime.rs

@@ -312,14 +315,33 @@ pub unsafe fn new_rt_and_cx() -> Runtime {
    Runtime(runtime)
 }

+#[no_mangle]
+#[allow(unsafe_code)]
+pub unsafe extern "C" fn get_size(obj: *mut JSObject) -> usize {


nit: no need for pub.

jdm · 2018-03-27T02:13:06Z

components/script/script_thread.rs

@@ -1581,30 +1579,11 @@ impl ScriptThread {

    fn collect_reports(&self, reports_chan: ReportsChan) {
        let mut path_seg = String::from("url(");
-        let mut dom_tree_size = 0;
        let mut reports = vec![];


This is unused now.

I am interpreting this to mean that since we don't add any dom-tree entries to the reports vector anymore we can use the return of get_reports directly. ie. reports_chan.send(get_reports(self.get_cx(), path_seg)); Is that what you meant?

Oh, my mistake. I was skimming the changes and didn't look closely enough. You can ignore this comment.

jdm · 2018-03-27T02:13:06Z

components/script/script_thread.rs

-            dom_tree_size += malloc_size_of_including_self(&mut ops, document.window());
-
-            if reports.len() > 0 {
-                path_seg.push_str(", ");


We're missing the joining , now. Let's do the following instead of this loop:

let documents = self.documents.borrow(); let urls = itertools::join(documents.values().map(|d| d.url().as_str()), ", ") let path_seg = format!("url({})", urls);

(you'll need to add an extern crate itertools to script/lib.rs and add itertools to script/Cargo.toml, too)

This change results in a compilation error no method named values found for type std::cell::Ref<'_, script_thread::Documents>. I looked at the impl for Documents, but was not able to fix the problem. I will continue working on it tomorrow.

Oh, I made the wrong assumption about the type of self.documents. You can use documents.iter().map(|(_, d)| d.url().as_str()) instead.

When I put this in as is, it results in a compilation error on d.url() borrowed value does not live long enough (see attached screenshot for details). I think this is caused by as_str() returning a &str. I replaced as_str() with to_string() and it compiled and I tested it and the colon separated list of urls shows up. Please let me know if that change is ok.

Looks good to me!

jdm · 2018-03-27T02:13:06Z

components/script/lib.rs

@@ -200,6 +202,12 @@ pub fn init_service_workers(sw_senders: SWManagerSenders) {
    ServiceWorkerManager::spawn_manager(sw_senders);
 }

+#[no_mangle]
+#[allow(unsafe_code)]
+pub unsafe extern "C" fn is_dom_object(obj: *mut JSObject) -> bool {


nit: no need for pub.

jdm · 2018-03-28T00:04:05Z

@bors-servo r+

bors-servo · 2018-03-28T00:04:06Z

📌 Commit 60dfd66 has been approved by jdm

bors-servo · 2018-03-28T00:04:11Z

⌛ Testing commit 60dfd66 with merge 4efeda6...


        Auto merge of #20430 - aeweston98:aew-issue7084, r=jdm

Pass new method in CollectServoSizes to get size of ObjectPrivateVisitor  These changes are the servo changes to allow the measurement of DOM objects with the ObjectPrivateVisitor API. Two new functions have been added MemoryReporter and get_size, which are passed as function pointers to two updated mozjs functions. Currently these changes rely on a branch version of mozjs, which has an [open pull request](servo/rust-mozjs#409). ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #7084 (github issue number if applicable).  - [ ] There are tests for these changes OR - [x] These changes do not require tests because _____    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/20430)

bors-servo · 2018-03-28T00:24:52Z

💔 Test failed - linux-dev


        Pass new method in CollectServoSizes for accurate DOM heap use reporting

aeweston98 · 2018-03-28T00:38:49Z

@jdm It was the lack of EOF in Cargo.toml which caused test-tidy to fail, fixed now. Also updated to latest changes in master.

jdm · 2018-03-28T01:21:52Z

@bors-servo r+

bors-servo · 2018-03-28T01:21:53Z

📌 Commit 7f7fc91 has been approved by jdm

bors-servo · 2018-03-28T01:21:59Z

⌛ Testing commit 7f7fc91 with merge 339146a...


        Auto merge of #20430 - aeweston98:aew-issue7084, r=jdm

Pass new method in CollectServoSizes to get size of ObjectPrivateVisitor  These changes are the servo changes to allow the measurement of DOM objects with the ObjectPrivateVisitor API. Two new functions have been added MemoryReporter and get_size, which are passed as function pointers to two updated mozjs functions. Currently these changes rely on a branch version of mozjs, which has an [open pull request](servo/rust-mozjs#409). ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #7084 (github issue number if applicable).  - [ ] There are tests for these changes OR - [x] These changes do not require tests because _____    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/20430)

bors-servo · 2018-03-28T02:44:28Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev
Approved by: jdm
Pushing 339146a to master...

highfive assigned nox Mar 26, 2018

highfive added the S-awaiting-review label Mar 26, 2018

aeweston98 mentioned this pull request Mar 26, 2018

Measure the heap usage of every DOM object in existence #7084

Closed

jdm requested changes Mar 26, 2018

View changes

jdm added S-needs-code-changes S-needs-squash and removed S-awaiting-review labels Mar 26, 2018

highfive added S-awaiting-review and removed S-needs-code-changes labels Mar 26, 2018

jdm added S-needs-rebase and removed S-awaiting-review labels Mar 26, 2018

jdm assigned jdm and unassigned nox Mar 26, 2018

jdm added the This Week In Servo (TWiS) label Mar 26, 2018

aeweston98 force-pushed the aeweston98:aew-issue7084 branch from c55908c to 4d64a7d Mar 27, 2018

highfive added the S-awaiting-review label Mar 27, 2018

jdm requested changes Mar 27, 2018

View changes

jdm added S-needs-code-changes and removed S-awaiting-review S-needs-rebase S-needs-squash labels Mar 27, 2018

highfive added S-awaiting-review and removed S-needs-code-changes labels Mar 27, 2018

aeweston98 force-pushed the aeweston98:aew-issue7084 branch from d153a04 to 60dfd66 Mar 27, 2018

highfive added S-awaiting-merge and removed S-awaiting-review labels Mar 28, 2018

highfive added S-tests-failed and removed S-awaiting-merge labels Mar 28, 2018

aeweston98 force-pushed the aeweston98:aew-issue7084 branch from 60dfd66 to 093ee24 Mar 28, 2018

highfive added S-awaiting-review and removed S-tests-failed labels Mar 28, 2018

Pass new method in CollectServoSizes for accurate DOM heap use reporting

Loading status checks…

7f7fc91

aeweston98 force-pushed the aeweston98:aew-issue7084 branch from 093ee24 to 7f7fc91 Mar 28, 2018

highfive added S-awaiting-merge and removed S-awaiting-review labels Mar 28, 2018

bors-servo mentioned this pull request Mar 28, 2018

Use higher level handles from JS bindings #20246

Merged

4 of 5 tasks complete

bors-servo merged commit 7f7fc91 into servo:master Mar 28, 2018
3 checks passed

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

highfive removed the S-awaiting-merge label Mar 28, 2018

This was referenced Mar 28, 2018

Mozmap #20318

Closed

(Do not merge) Introduce pins #20063

Closed

servo / servo

Pass new method in CollectServoSizes to get size of ObjectPrivateVisitor #20430

Pass new method in CollectServoSizes to get size of ObjectPrivateVisitor #20430

aeweston98 commented Mar 26, 2018 •

edited

highfive commented Mar 26, 2018

highfive commented Mar 26, 2018

highfive commented Mar 26, 2018

jdm left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

jdm commented Mar 26, 2018

aeweston98 commented Mar 27, 2018

jdm commented Mar 27, 2018

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

jdm commented Mar 28, 2018

bors-servo commented Mar 28, 2018

bors-servo commented Mar 28, 2018

bors-servo commented Mar 28, 2018

aeweston98 commented Mar 28, 2018

jdm commented Mar 28, 2018

bors-servo commented Mar 28, 2018

bors-servo commented Mar 28, 2018

bors-servo commented Mar 28, 2018

servo / servo

Join GitHub today

Pass new method in CollectServoSizes to get size of ObjectPrivateVisitor #20430

Pass new method in CollectServoSizes to get size of ObjectPrivateVisitor #20430

Conversation

aeweston98 commented Mar 26, 2018 • edited

highfive commented Mar 26, 2018

highfive commented Mar 26, 2018

highfive commented Mar 26, 2018

jdm left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

jdm commented Mar 26, 2018

aeweston98 commented Mar 27, 2018

jdm commented Mar 27, 2018

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

aeweston98 Mar 27, 2018 • edited

This comment has been minimized.

This comment has been minimized.

jdm commented Mar 28, 2018

bors-servo commented Mar 28, 2018

bors-servo commented Mar 28, 2018

bors-servo commented Mar 28, 2018

aeweston98 commented Mar 28, 2018

jdm commented Mar 28, 2018

bors-servo commented Mar 28, 2018

bors-servo commented Mar 28, 2018

bors-servo commented Mar 28, 2018

aeweston98 commented Mar 26, 2018 •

edited

aeweston98 Mar 27, 2018 •

edited