Closed Bug 1387956 Opened 7 years ago Closed 7 years ago

stylo: Need to measure ComputedValues together during memory reporting

Categories

(Core :: CSS Parsing and Computation, enhancement)

Product:
Core
Component:
CSS Parsing and Computation
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla57
Tracking Flags:
Tracking Status
firefox57 --- fixed

People

(Reporter: bholley, Assigned: n.nethercote)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Bug 1387956 - Overhaul ComputedValues measurement, and add style structs measurement. .
(deleted), text/x-review-board-request
bholley
: review+
Details
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).
Nick, do I have this right? If so, is this something you can take?
Flags: needinfo?(n.nethercote)
Summary: stylo: Need to measure ComputedValues together → stylo: Need to measure ComputedValues together during memory reporting
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)
Great, thanks!
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)
> Itemization of each style struct would be even nicer I'm doing that, it's not too bad and we have similar stuff already.
(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.
Depends on: 1388975
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
Ok, I've fixed the ServoStyleContext problem from the previous version.
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+
No longer depends on: 1388975
(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.
Still more stuff to land here.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
> > +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.
I have opened https://github.com/servo/servo/pull/18065 for the Servo side of these changes.
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.
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
Closed: 7 years ago → 7 years 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
(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.
(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.
Ah, yes, I misread comment 23. Makes sense now.
Blocks: 1281964
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: