Add measurement of the flow tree's size. #5152

nnethercote · 2015-03-05T07:45:58Z

This is a DRAFT patch. The way the measurement is incorporated needs to
be fixed, but I'm putting it up now in order to get feedback because
this code exemplifies the likely structure of all future fine-grained
memory reporting in Servo, which is heavily influenced by the
corresponding code in Firefox.

Comments marked with "njn" in particular are places where things are
uncertain and/or incomplete.

Note also that we also will need a DMD-like tool that can verify the
correctness of these measurements (which is almost impossible,
otherwise) and also find places where additional measurements should be
added. This requires alloc/dealloc wrapping, which I've been
experimenting with and soliciting feedback for, without much success
yet.

BTW, it's not clear to me at the moment if the flow tree is a persistent
data structure or one that's rebuilt from scratch on each reflow. At
this point I rather hope it's the former.


        Add measurement of the flow tree's size.

This is a DRAFT patch. The way the measurement is incorporated needs to be fixed, but I'm putting it up now in order to get feedback because this code exemplifies the likely structure of all future fine-grained memory reporting in Servo, which is heavily influenced by the corresponding code in Firefox. Comments marked with "njn" in particular are places where things are uncertain and/or incomplete. Note also that we also will need a DMD-like tool that can verify the correctness of these measurements (which is almost impossible, otherwise) and also find places where additional measurements should be added. This requires alloc/dealloc wrapping, which I've been experimenting with and soliciting feedback for, without much success yet. BTW, it's not clear to me at the moment if the flow tree is a persistent data structure or one that's rebuilt from scratch on each reflow. At this point I rather hope it's the former.

highfive · 2015-03-05T07:46:01Z

Warning

These commits modify unsafe code. Please review it carefully!

hoppipolla-critic-bot · 2015-03-05T07:46:03Z

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

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 · 2015-03-05T07:47:01Z

Some people who might be interested in this: @pcwalton, @Manishearth, @larsbergstrom, @metajack, @jdm.

nnethercote · 2015-03-05T07:50:03Z

BTW https://developer.mozilla.org/en-US/docs/Mozilla/Performance/Memory_reporting#Making_Measurements has some documentation about how the memory reporting works in Firefox, including a lot of motivation explaining why the design is as it is, all of which is highly relevant to all this.

jdm · 2015-03-05T20:51:38Z

This looks like what I expected. Deriving it automatically could be tricker than expected due to size_of_vec_excluding_self.

pcwalton · 2015-03-05T21:14:45Z

BTW, it's not clear to me at the moment if the flow tree is a persistent
data structure or one that's rebuilt from scratch on each reflow. At
this point I rather hope it's the former.

Yes, it's the former.

nnethercote · 2015-03-05T22:07:19Z

Deriving it automatically could be tricker than expected due to size_of_vec_excluding_self.

Oh, I just realized that size_of_vec_excluding_self isn't necessary, because it's possible to implement the SizeOf trait for any type, including primitive types such as integers. (Even if the Rust book says "it is considered poor style"!)


        Remove `size_of_vec_including_this`.

We can get rid of it by implementing the `SizeOf` trait for all measured vector element types. This will make deriving the `SizeOf` trait easier. This patch should be squashed with the preceding patch before landing.


        Remove `size_of_including_this`.

By implementing `SizeOf` for Box<T>, we no longer need `size_of_including_this`. This has the benefit of making it trivial to switch between a measuring implementation and a computing implementation.

nnethercote · 2015-03-06T01:02:24Z

At this point anyone who is interested in these patches should really read the "files changed" tab rather than the "commits" tab.

Manishearth · 2015-03-06T12:21:33Z

components/util/memory.rs

+    if ptr == ::std::rt::heap::EMPTY as *const c_void {
+        0
+    } else {
+        unsafe { je_malloc_usable_size(ptr) as usize }


Why aren't we just using intrinsics::size_of() here? Trait objects?

There's a big comment explaining this 18 lines below. (GitHub is annoying in that it emails code comments immediately, so if subsequent reading gives you new insight you can't go back and change or remove a comment. Perhaps this was one of those cases?)

Yeah, sort of, was on a phone and reading rather slowly :)


        Rename LayoutTask::handle_script_request.

Because it handles requests from tasks other than the Script Task.


        Add infrastructure for memory reporting from threads.

This change adds the ability for threads to register memory reporters with the memory profiler. The memory profiler passes collection request to the reporters and they give memory reports back, which the memory profiler prints.

nnethercote · 2015-03-10T06:01:39Z

The newest changeset adds infrastructure for communication between the memory profiler thread and other threads for the purposes of memory reporting. Unfortunately, my decision to measure the flow tree first was perhaps not the best; I've been told that it is problematic to measure the flow tree at arbitrary times in the layout task, because the script task could modify it.

One possibility is to have the script task manage and trigger the measurement of the flow tree, like it does with reflows.

Another possibility is to pick a different subsystem (and data structure(s)) to use for the first memory reporter. Any suggestions?

metajack · 2015-03-10T18:54:30Z

Routing it through the script task seems reasonable to me. @jdm may have further comments.

As for other subsystems, figuring out the DOM tree sizes would be another good place to look. Especially as we are using eagerly created wrappers and have been told that could be a source of memory waste.

@pcwalton has also been concerned with the size of the display list.

larsbergstrom · 2015-03-10T19:00:24Z

@nnethercote When we were chatting about measuring the flow tree in Portland, I believe we (verbally) sketched out a design where there are profiling heartbeat messages transmitted between our tasks, for exactly this reason (inconsistent view of the system state). I think we talked about it originating from script, since script "drives" changes to the DOM and we could wait for the completion of the reflow to report in order to ensure the memory reported from script at heartbeat N corresponds to same flow tree for that heartbeat instead of just whatever Servo happened to have in memory at the timestamp corresponding to heartbeat N.

I don't think we came up with a great solution for the problem of async resource loading (which could cause other hiccups in the reporting) other than possibly forcing sequential loading, as we do for many of our testing harnesses.

nnethercote · 2015-03-11T04:40:46Z

This PR has been superseded by #5193, which instead measures the display list. I will get back to measuring the flow tree eventually, but this PR has become messy so I will open a new PR for the flow tree when the time comes.

jdm added the S-awaiting-review label Mar 5, 2015

nnethercote added 2 commits Mar 5, 2015

Remove `size_of_vec_including_this`.

1b42d63

We can get rid of it by implementing the `SizeOf` trait for all measured vector element types. This will make deriving the `SizeOf` trait easier. This patch should be squashed with the preceding patch before landing.

Remove `size_of_including_this`.

90055da

By implementing `SizeOf` for Box<T>, we no longer need `size_of_including_this`. This has the benefit of making it trivial to switch between a measuring implementation and a computing implementation.

Manishearth reviewed Mar 6, 2015
View changes

nnethercote added 2 commits Mar 10, 2015

Rename LayoutTask::handle_script_request.

0152ad1

Because it handles requests from tasks other than the Script Task.

Add infrastructure for memory reporting from threads.

52b685b

This change adds the ability for threads to register memory reporters with the memory profiler. The memory profiler passes collection request to the reporters and they give memory reports back, which the memory profiler prints.

nnethercote closed this Mar 11, 2015

nnethercote mentioned this pull request Mar 16, 2015

Fix memory leak in flow tree by adding weak refs. #5228

Merged

servo / servo

Add measurement of the flow tree's size. #5152

Add measurement of the flow tree's size. #5152

nnethercote commented Mar 5, 2015

highfive commented Mar 5, 2015

hoppipolla-critic-bot commented Mar 5, 2015

nnethercote commented Mar 5, 2015

nnethercote commented Mar 5, 2015

jdm commented Mar 5, 2015

pcwalton commented Mar 5, 2015

nnethercote commented Mar 5, 2015

nnethercote commented Mar 6, 2015

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

nnethercote commented Mar 10, 2015

metajack commented Mar 10, 2015

larsbergstrom commented Mar 10, 2015

nnethercote commented Mar 11, 2015

servo / servo

Join GitHub today

Add measurement of the flow tree's size. #5152

Add measurement of the flow tree's size. #5152

Conversation

nnethercote commented Mar 5, 2015

highfive commented Mar 5, 2015

hoppipolla-critic-bot commented Mar 5, 2015

nnethercote commented Mar 5, 2015

nnethercote commented Mar 5, 2015

jdm commented Mar 5, 2015

pcwalton commented Mar 5, 2015

nnethercote commented Mar 5, 2015

nnethercote commented Mar 6, 2015

This comment has been minimized.

Manishearth Mar 6, 2015

This comment has been minimized.

nnethercote Mar 6, 2015

This comment has been minimized.

Manishearth Mar 7, 2015

nnethercote commented Mar 10, 2015

metajack commented Mar 10, 2015

larsbergstrom commented Mar 10, 2015

nnethercote commented Mar 11, 2015