Closed Bug 1387958 Opened 7 years ago Closed 7 years ago

stylo: Measure the entire stylist during memory reporting

Categories

(Core :: CSS Parsing and Computation, enhancement, P4)

enhancement

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: bholley, Assigned: n.nethercote)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

I discussed this a bit in bug 1367854 comment 1.

All of the per-window style data hangs off the ServoStyleSet in mRawSet, which basically just wraps a stylist. The stylist holds onto the parsed stylesheets, fast-access hashtables, and various other things.

Right now we're only measuring stylesheets, via ServoStyleSheet, which is a C++ wrapper around the rust stylesheet. Those give us a second graph edge to the servo-side parsed stylesheet representation. So we can either continue traversing that, and not traverse it from the stylist, or vice versa. I don't have an a priori opinion on which one is better.

Anyway, the main problem with the status quo is that we're not measuring the other stuff in stylist, especially the hashtables (which are large). Doing memory reporting for the ServoStyleSet/stylist would fix that.
Nick, does this make sense?
Flags: needinfo?(n.nethercote)
> Right now we're only measuring stylesheets, via ServoStyleSheet, which is a
> C++ wrapper around the rust stylesheet. Those give us a second graph edge to
> the servo-side parsed stylesheet representation. So we can either continue
> traversing that, and not traverse it from the stylist, or vice versa. I
> don't have an a priori opinion on which one is better.

If we switched to measuring them through the Stylist I guess that would allow us to remove Servo_StyleSheet_SizeOfIncludingThis(), which would be nice. On the other hand, going via
the C++ wrappers is currently working, so I might just leave it alone for now.

Will the C++ wrappers go away at any point, e.g. once Gecko's style system is removed?
 
> Anyway, the main problem with the status quo is that we're not measuring the
> other stuff in stylist, especially the hashtables (which are large). Doing
> memory reporting for the ServoStyleSet/stylist would fix that.

I've started working on this, it looks fairly straightforward.
Assignee: nobody → n.nethercote
Flags: needinfo?(n.nethercote)
(In reply to Nicholas Nethercote [:njn] from comment #2)
> > Right now we're only measuring stylesheets, via ServoStyleSheet, which is a
> > C++ wrapper around the rust stylesheet. Those give us a second graph edge to
> > the servo-side parsed stylesheet representation. So we can either continue
> > traversing that, and not traverse it from the stylist, or vice versa. I
> > don't have an a priori opinion on which one is better.
> 
> If we switched to measuring them through the Stylist I guess that would
> allow us to remove Servo_StyleSheet_SizeOfIncludingThis(), which would be
> nice. On the other hand, going via
> the C++ wrappers is currently working, so I might just leave it alone for
> now.
> 
> Will the C++ wrappers go away at any point, e.g. once Gecko's style system
> is removed?

Unlikely, there's a lot of C++ that likes to manipulate stylesheets.

>  
> > Anyway, the main problem with the status quo is that we're not measuring the
> > other stuff in stylist, especially the hashtables (which are large). Doing
> > memory reporting for the ServoStyleSet/stylist would fix that.
> 
> I've started working on this, it looks fairly straightforward.

Great, thanks!
Priority: -- → P4
> I've started working on this, it looks fairly straightforward.

I spoke too soon.

Stylist contains HashMaps. It's not possible to measure the memory usage of a Rust HashMap, because Rust doesn't provide an API that exposes the pointer for the underlying memory block. (In comparision, it does expose that pointer for Vec.) We can estimate the memory usage, but (a) that's potentially inaccurate, and (b) doesn't integrate with DMD. (This has come up before in https://github.com/servo/servo/issues/6908.)

But we've been talking about forking HashMap anyway, in order to extend it with fallible growth methods. So let's do that, and add the memory reporting functions as well. However, HashMap uses numerous unstable features that are only available if you are compiling with Nightly Rust. The features include the following:

- alloc::heap::{Heap, Alloc, Layout}
- std::mem::needs_drop
- std::ptr::{Unique, Shared}

Manishearth, any suggestions on how to proceed here?
Flags: needinfo?(manishearth)
(In reply to Nicholas Nethercote [:njn] from comment #4)
> > I've started working on this, it looks fairly straightforward.
> 
> I spoke too soon.
> 
> Stylist contains HashMaps. It's not possible to measure the memory usage of
> a Rust HashMap, because Rust doesn't provide an API that exposes the pointer
> for the underlying memory block. (In comparision, it does expose that
> pointer for Vec.) We can estimate the memory usage, but (a) that's
> potentially inaccurate, and (b) doesn't integrate with DMD. (This has come
> up before in https://github.com/servo/servo/issues/6908.)
> 
> But we've been talking about forking HashMap anyway, in order to extend it
> with fallible growth methods. So let's do that, and add the memory reporting
> functions as well. However, HashMap uses numerous unstable features that are
> only available if you are compiling with Nightly Rust. The features include
> the following:
> 
> - alloc::heap::{Heap, Alloc, Layout}

http://searchfox.org/mozilla-central/rev/bd39b6170f04afeefc751a23bb04e18bbd10352b/servo/components/servo_arc/lib.rs#557

Though if the point is to support fallible allocation, we may need our own custom alloc hook anyway.

> - std::mem::needs_drop
> - std::ptr::{Unique, Shared}

http://searchfox.org/mozilla-central/rev/bd39b6170f04afeefc751a23bb04e18bbd10352b/servo/components/servo_arc/lib.rs#76
 
> 
> Manishearth, any suggestions on how to proceed here?
Also, if this were just about memory reports, it seems like forking hashmap would be overkill. We could probably get away with some scheme where we grab an arbitrary element from the hashmap, and pass that to a fancy jemalloc API to tell us what allocated block it corresponds to.

However, if we're pretty sure we're going to need fallible allocation anyway, then we may have no choice?
the needs_drop can be removed; it's an optimization.

I would suggest using ordermap or some other HashMap crate from crates.io, the stdlib one is pretty complicated and maintaining a fork will be nontrivial IMO.
Flags: needinfo?(manishearth)
FWIW, here are the 8(!) alternatives I've considered here.

1. expose HashMap::as_ptr()
+ The "right" solution
- Requires language changes, won't happen soon if ever

2. estimate size
+ Works now with minimal effort
- Can be wrong
- Doesn't integrate with DMD

3. fork HashMap
+ Gives us full control
+ Also would allow fallible allocations
- Difficult, need to fork liballoc and lots of other stuff too, or 
  substantially change HashMap

4. use a different HashMap implementation, e.g. OrderedMap
+ Gives us full control
+ Also would allow fallible allocations
- We end up using multiple HashMap implementations, and have to change for
  every HashMap we want to measure

5. Use Iter(), and handle internal pointers in malloc_usable_size
? Not sure if it's possible for all block sizes
- DMD still requires start pointers; fixing that would be a big change

6. Use Iter(), and create an malloc_internal_to_start_ptr() function
+ Should be doable, and now
- A bit inelegant
- Requires writing the code

7. use unsafe code and knowledge of HashMap layout to find the pointer
+ Works now
- Dangerous; not future-proof

8. Use a per-HashMap custom allocator
- Currently not possible?


If fallible allocations are found to be important, 3 will be necessary anyway. Otherwise, 6 is also reasonable.
> If fallible allocations are found to be important, 3 will be necessary anyway.

Make that 3 or 4.
Depends on: 1389305
Blocks: 1281964
> 3. fork HashMap
> + Gives us full control
> + Also would allow fallible allocations
> - Difficult, need to fork liballoc and lots of other stuff too, or 
>   substantially change HashMap

w.r.t. HashMap's use of unstable features, froydnj says you can get a stable rust to allow unstable features by setting the RUSTC_BOOTSTRAP environment variables, and we already do this in some configurations in order to use SIMD features:
https://dxr.mozilla.org/mozilla-central/rev/7ff4c2f1fe11f6b98686f783294692893b1e1e8b/config/rules.mk#910-913

This is pretty gross and maybe not a road we want to travel further down, but I might as well chuck it onto the existing pile of bad options.
> 6. Use Iter(), and create an malloc_internal_to_start_ptr() function
> + Should be doable, and now
> - A bit inelegant
> - Requires writing the code

This is the approach I'm using now.
Depends on: 1393384
Comment on attachment 8904392 [details]
Bug 1387958 - Measure the stylist during memory reporting. .

https://reviewboard.mozilla.org/r/176192/#review181160

Looks good, thanks!

::: commit-message-0afab:13
(Diff revision 1)
> +> │  ├────142,336 B (00.07%) ── element-and-pseudos-maps
> +> │  ├─────14,336 B (00.01%) ── revalidation-selectors
> +> │  ├──────9,648 B (00.00%) ── other
> +> │  └──────3,552 B (00.00%) ── precomputed-pseudos
> +
> +This changes requires new code to measure HashMaps, which uses the new

*change
Attachment #8904392 - Flags: review?(cam) → review+
https://github.com/servo/servo/pull/18375 is the Servo PR for this bug.
Do I understand correctly that this code doesn't measure the heap memory used by spilled SmallVecs? If so, we're missing a big part of the picture here - the SmallVec thing is an optimization to store entries inline in the hashtable when we have only one entry, but a significant number of keys will have multiple entries, at which point we'll spill to out-of-line storage.
Flags: needinfo?(n.nethercote)
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/a068e2e9e539
Measure the stylist during memory reporting. r=heycam.
(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #16)
> Do I understand correctly that this code doesn't measure the heap memory
> used by spilled SmallVecs?

Correct. I haven't seen spilled SmallVecs showing up in DMD output, so to save time I ignored them for now.
Flags: needinfo?(n.nethercote)
https://hg.mozilla.org/mozilla-central/rev/a068e2e9e539
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.