stylo: Need to measure ComputedValues together during memory reporting

RESOLVED FIXED in Firefox 57

Status

()

Core
CSS Parsing and Computation
RESOLVED FIXED
7 months ago
6 months ago

People

(Reporter: bholley, Assigned: njn)

Tracking

(Blocks: 1 bug)

unspecified
mozilla57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

7 months ago
Continuing discussion from bug 1367854 comment 6.

My understanding of memory reports is patchy, so let me know if I'm getting this wrong.

The observable I would like is the following two per-window measurements:
(I) Memory overhead of all ComputedValues hanging off elements in that DOM.
(II) Memory overhead of all ComputedValues hanging off the frame tree that were not counted in (I).

To achieve this, it seems like we want to do the following, in order:
(1) Stop measuring mServoData during the normal DOM traversal (so we stop billing it to elements).
(2) During memory reporting, traverse the DOM (using a StyleChildrenIterator), measuring the mServoData of every element. The result of this gives us (I).
(3) Next, with the |seen| table already primed from (2), traverse the frame tree, measuring StyleContext() on every frame (style contexts and ComputedValues are the same data structure). This gives us (II).
(Reporter)

Comment 1

7 months ago
Nick, do I have this right? If so, is this something you can take?
Flags: needinfo?(n.nethercote)
(Reporter)

Updated

7 months ago
Summary: stylo: Need to measure ComputedValues together → stylo: Need to measure ComputedValues together during memory reporting
(Assignee)

Comment 2

7 months ago
I'm happy to take it. I'm not sure yet if the algorithm in comment 0 will work or not. I'll investigate once I've finished bug 1387958, which is easier than this one.
Assignee: nobody → n.nethercote
Flags: needinfo?(n.nethercote)
(Reporter)

Comment 3

7 months ago
Great, thanks!
(Reporter)

Comment 4

7 months ago
bz points out that it would be really nice to measure the weight of the style structs separately from the ComputedValues, which would e.g. be useful diagnosing issues like the hypothesis in bug 1367854 comment 19. Itemization of each style struct would be even nicer, but that might make the reporting code unwieldy. We could potentially break ComputedValues down into:
* inherited structs
* non-inherited structs
* rule nodes
* Everything else (including self)
(Assignee)

Comment 5

7 months ago
> Itemization of each style struct would be even nicer

I'm doing that, it's not too bad and we have similar stuff already.
(Reporter)

Comment 6

7 months ago
(In reply to Nicholas Nethercote [:njn] from comment #5)
> > Itemization of each style struct would be even nicer
> 
> I'm doing that, it's not too bad and we have similar stuff already.

Great! I realize now that the STYLE_STRUCT maros should actually make this relatively manageable.
(Assignee)

Updated

7 months ago
Depends on: 1388975
Comment hidden (mozreview-request)
(Assignee)

Comment 8

7 months ago
This is in good shape except for one thing: the step-backwards-over-the-Arc-refcount hack is busted for ServoStyleContext on Win32 and some Android configs: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3a2aa15c4e6453479717253723774682842b08eb
Comment hidden (mozreview-request)
(Assignee)

Comment 10

7 months ago
Ok, I've fixed the ServoStyleContext problem from the previous version.
(Reporter)

Comment 11

7 months ago
mozreview-review
Comment on attachment 8896177 [details]
Bug 1387956 - Overhaul ComputedValues measurement, and add style structs measurement. .

https://reviewboard.mozilla.org/r/167444/#review172946

This is really great work. I confess to skimming the details a little bit because I don't know the memory reporting code and I'm sleepy, but I wanted to unblock this. Feel free to flag someone else if you want a more careful review of that part.

::: commit-message-f50b8:55
(Diff revision 2)
> +ignored.) 'servo-style-structs/sundries' aggregates all the style structs that
> +are smaller than 8 KiB.

Does that mean the style structs that collectively consume less than 8KiB?

::: dom/base/nsINode.h:335
(Diff revision 2)
> -  virtual size_t SizeOfExcludingThis(mozilla::SizeOfState& aState) const;
> +  virtual void AddSizeOfExcludingThis(mozilla::SizeOfState& aState,
> +                                      nsStyleSizes& aSizes,
> +                                      size_t* aNodeSize) const;

Should this use the new macro?

::: js/xpconnect/src/XPCJSRuntime.cpp:2161
(Diff revision 2)
> +        // We combine the node size with nsStyleSizes here. It's not ideal, but
> +        // it's hard to get the style structs measurements out to
> +        // nsWindowMemoryReporter, and the number of orphan DOM nodes is
> +        // usually small.

It shouldn't matter. We drop mServoData in UnbindFromTree, so any non-in-tree element can't have any style data to mueasure.

::: layout/style/nsCSSPseudoElements.h:95
(Diff revision 2)
> +  // This must match EAGER_PSEUDO_COUNT in Rust code.
> +  static const size_t kEagerPseudoCount = 4;

Maybe put this right above IsEagerlyCascadedInServo, with no newline, so that it's harder to miss?

::: servo/components/style/data.rs:270
(Diff revision 2)
> +        // XXX: measure the EagerPseudoArray itself, but not the ComputedValues
> +        // within it.
> +
> +        0

Seems like the code to do this is missing?

::: servo/ports/geckolib/glue.rs:811
(Diff revision 2)
> +pub extern "C" fn Servo_Element_GetPseudoComputedValues(element: RawGeckoElementBorrowed,
> +                                                        index: usize) -> ServoStyleContextStrong
> +{
> +    let element = GeckoElement(element);
> +    let data = element.borrow_data().expect("Getting CVs on unstyled element");
> +    data.styles.pseudos.as_array()[index].as_ref().expect("Getting CVs on unstyled element")

This expect message is wrong.
Attachment #8896177 - Flags: review?(bobbyholley) → review+
(Assignee)

Comment 12

7 months ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/0823bc83b06b7e5a5e732c368157c2a8a848e0c7
Bug 1387956 (part 1) - Change |nsWindowSizes*| arguments to |nsWindowSizes&|. r=mccr8.

https://hg.mozilla.org/integration/mozilla-inbound/rev/a8aa2a5c2870498d9c21b05fed673d786591f817
Bug 1387956 (part 2) - Overhaul handling of nsWindowSizes. r=mccr8.
(Assignee)

Updated

7 months ago
No longer depends on: 1388975
Duplicate of this bug: 1388975
(Assignee)

Comment 14

7 months ago
(In reply to Nicholas Nethercote [:njn] from comment #12)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/
> 0823bc83b06b7e5a5e732c368157c2a8a848e0c7
> Bug 1387956 (part 1) - Change |nsWindowSizes*| arguments to
> |nsWindowSizes&|. r=mccr8.
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/
> a8aa2a5c2870498d9c21b05fed673d786591f817
> Bug 1387956 (part 2) - Overhaul handling of nsWindowSizes. r=mccr8.

These patches were r+'d in bug 1388975 but I accidentally landed them under this bug, so I have dup'd that bug to this one.
https://hg.mozilla.org/mozilla-central/rev/0823bc83b06b
https://hg.mozilla.org/mozilla-central/rev/a8aa2a5c2870
Status: NEW → RESOLVED
Last Resolved: 7 months ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
(Assignee)

Comment 16

7 months ago
Still more stuff to land here.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 17

7 months ago
> > +ignored.) 'servo-style-structs/sundries' aggregates all the style structs that
> > +are smaller than 8 KiB.
> 
> Does that mean the style structs that collectively consume less than 8KiB?

Depends what you mean by "collectively". For each style struct kind (Position, Font, etc.) if the total for the window is less than 8 KiB, it'll get lumped into sundries. So sundries can exceed 8 KiB. It's an approach already in use in a number of places in memory reporting code.
(Assignee)

Comment 18

7 months ago
I have opened https://github.com/servo/servo/pull/18065 for the Servo side of these changes.
Comment hidden (mozreview-request)
(Assignee)

Comment 20

7 months ago
mozreview-review-reply
Comment on attachment 8896177 [details]
Bug 1387956 - Overhaul ComputedValues measurement, and add style structs measurement. .

https://reviewboard.mozilla.org/r/167444/#review172946

> Should this use the new macro?

No, because it lacks the |override|.

> It shouldn't matter. We drop mServoData in UnbindFromTree, so any non-in-tree element can't have any style data to mueasure.

Ok, I updated the comment.

> Seems like the code to do this is missing?

The "XXX" communicates that it's a todo.

Comment 21

7 months ago
Pushed by nnethercote@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5f2f00d59868
Overhaul ComputedValues measurement, and add style structs measurement. r=bholley.
https://hg.mozilla.org/mozilla-central/rev/5f2f00d59868
Status: REOPENED → RESOLVED
Last Resolved: 7 months ago7 months ago
Resolution: --- → FIXED
and we see some memory improvements from this:
== Change summary for alert #8758 (as of August 14 2017 03:16 UTC) ==

Improvements:

  4%  Heap Unclassified summary linux64-stylo opt stylo     78,743,881.56 -> 75,415,126.79
  4%  Heap Unclassified summary macosx64-stylo opt stylo    97,335,159.26 -> 93,461,847.52
  4%  Heap Unclassified summary linux64-stylo-sequential opt stylo-sequential78,116,199.50 -> 75,255,957.13

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=8758
(Assignee)

Comment 24

6 months ago
(In reply to Joel Maher ( :jmaher) (UTC-5) from comment #23)
> and we see some memory improvements from this:

That is surprising! This bug just added some code to measure Stylo memory usage. It should not have changed Stylo memory usage.
(Reporter)

Comment 25

6 months ago
(In reply to Nicholas Nethercote [:njn] from comment #24)
> (In reply to Joel Maher ( :jmaher) (UTC-5) from comment #23)
> > and we see some memory improvements from this:
> 
> That is surprising! This bug just added some code to measure Stylo memory
> usage. It should not have changed Stylo memory usage.

It seems like Talos is measuring heap-unclassified as a separate metric (where lower is better). So this is just saying that the patch reduced heap-unclassified, as expected, by measuring more things.
(Assignee)

Comment 26

6 months ago
Ah, yes, I misread comment 23. Makes sense now.
(Assignee)

Updated

6 months ago
Blocks: 1281964
You need to log in before you can comment on or make changes to this bug.