1438497 - stylo-chrome: Add xbl stuff to memory report so that we know what causes regression

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement)

Description

[Xidorn Quan [:xidorn] UTC+10]

When trying to enable stylo-chrome, there is some unclassified heap regression (1~2MB on chrome process) shows up in awsy. According to my previous investigation, that seems to be related to some XBL stuff.

We should add them into the memory report so that we know what are they.

Comment 1

[Xidorn Quan [:xidorn] UTC+10]

It looks like the main part isn't really XBL stuff... With my patch, bindings don't seem to be very large. But this patch is on top of bug 1436059 which may have optimized memory usage there a bit.

Comment 2

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1438497 - Add bindings into memory report.

Review commit: https://reviewboard.mozilla.org/r/220878/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/220878/

Comment 3

[Bobby Holley (:bholley)]

Comment on attachment 8951557 [details]
Bug 1438497 - Add bindings into memory report.

https://reviewboard.mozilla.org/r/220878/#review226898

Generally seems to hit the right bits, I assume you profiled with DMD. I'll let njn double-check the memory reporter mechanics.

Thanks for doing this!

::: dom/base/nsNodeInfoManager.cpp:519
(Diff revision 1)
> +  if (mBindingManager) {
> +    aSizes.mBindingsSize +=
> +      mBindingManager->SizeOfIncludingThis(aSizes.mState.mMallocSizeOf);
> +  }
> +
> +  // Measurement of the following members may be added later if DMD fins it

Nit: finds.

::: dom/xbl/nsBindingManager.cpp:1123
(Diff revision 1)
> +#define SHALLOW_SIZE_INCLUDING(field_) \
> +  n += field_ ? field_->ShallowSizeOfIncludingThis(aMallocSizeOf) : 0;
> +  SHALLOW_SIZE_INCLUDING(mBoundContentSet);
> +  SHALLOW_SIZE_INCLUDING(mWrapperTable);
> +  SHALLOW_SIZE_INCLUDING(mLoadingDocTable);
> +#undef SHALLOW_SIZE_INCLUDING

Nit: I'm not sure the macro is really worth it.

Comment 4

[Nicholas Nethercote [inactive]]

Comment on attachment 8951557 [details]
Bug 1438497 - Add bindings into memory report.

https://reviewboard.mozilla.org/r/220878/#review226966

Looks pretty good!

I'll second Bobby's suggestion to do a DMD run if you haven't already. Instructions are here: https://developer.mozilla.org/en-US/docs/Mozilla/Performance/DMD

By the way, did you know that DMD can do diffs? You just pass two filenames to dmd.py. That might be the best way to work out where the additional memory is -- do a run with Stylo and one without, see what the difference is.

::: dom/xbl/nsXBLPrototypeBinding.cpp:1709
(Diff revision 1)
> +{
> +  size_t n = aMallocSizeOf(this);
> +
> +  if (mResources) {
> +    n += mResources->SizeOfIncludingThis(aMallocSizeOf);
> +  }

You could write this as:

> n += mResources ? mResources->SizeOfIncludingThis(aMallocSizeOf) : 0;

::: dom/xbl/nsXBLPrototypeBinding.cpp:1719
(Diff revision 1)
> +      InnerAttributeTable* table = iter.UserData();
> +      n += table->ShallowSizeOfIncludingThis(aMallocSizeOf);
> +      for (auto iter2 = table->Iter(); !iter2.Done(); iter2.Next()) {
> +        nsXBLAttributeEntry* entry = iter2.UserData();
> +        while (entry) {
> +          n += aMallocSizeOf(entry);

It would be nicer to add a SizeOfIncludingThis() method to nsXBLAttributeEntry. And that method could be responsible for traversing the linked list.

::: dom/xbl/nsXBLPrototypeResources.cpp:255
(Diff revision 1)
> +    n += mRuleProcessor->SizeOfIncludingThis(aMallocSizeOf);
> +  }
> +#endif
> +  if (mServoStyles) {
> +    n += ServoAuthorStylesMallocSizeOf(mServoStyles.get());
> +    n += Servo_AuthorStyles_SizeOfExcludingThis(

Can you change this from Excluding to Including and move the line above inside Servo_AuthorStyles_SizeOfIncludingThis()?

::: servo/ports/geckolib/glue.rs:1192
(Diff revision 1)
> +    malloc_size_of: GeckoMallocSizeOf,
> +    malloc_enclosing_size_of: GeckoMallocSizeOf,
> +    styles: RawServoAuthorStylesBorrowed,
> +) -> usize {
> +    // MallocSizeOf cannot be put on the file level, otherwise
> +    // `Servo_StyleSheet_SizeOfIncludingThis` will use size_of from this trait.

I found this comment hard to read. Perhaps this would be better:

"We cannot `use` MallocSizeOf at the top level, otherwise..."

And I don't quite understand the second half of the sentence, about the trait.

Comment 5

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8951557 [details]
Bug 1438497 - Add bindings into memory report.

https://reviewboard.mozilla.org/r/220878/#review226966

I think I did a DMD run, but I didn't do the diffs... Probably I can try that. Thanks for the advice.

> Can you change this from Excluding to Including and move the line above inside Servo_AuthorStyles_SizeOfIncludingThis()?

I initially wanted to make it `IncludingThis`, and I figured out that `Box<_>` implements `MallocSizeOf` which is basically what I want... but there is no FFI sugar in the Servo side to get `&Box<_>` from FFI type, and it doesn't seem to be worth adding for this only usage...

But yeah probably we can just move that into the function...

> I found this comment hard to read. Perhaps this would be better:
> 
> "We cannot `use` MallocSizeOf at the top level, otherwise..."
> 
> And I don't quite understand the second half of the sentence, about the trait.

`Servo_StyleSheet_SizeOfIncludingThis` contains a line
```rust
StylesheetContents::as_arc(&sheet).size_of(&guard, &mut ops)
```
which is calling `StylesheetContents::size_of` function after deref.

But it seems somehow when `MallocSizeOf` is put at the top level, Rust tries to use `MallocSizeOf::size_of` there and complain about more arguments than desired. (But it doesn't seem to me `StylesheetContents` or `RawOffsetArc<StylesheetContents>` implements `MallocSizeOf`, so I have no idea what's happening now...)

Comment 6

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8951557 [details]
Bug 1438497 - Add bindings into memory report.

https://reviewboard.mozilla.org/r/220878/#review226898

> Nit: I'm not sure the macro is really worth it.

Expanding the macro would give us the same number of lines, but I think this way is prettier than breaking the ternary operator into two lines.

Comment 7

[Xidorn Quan [:xidorn] UTC+10]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=df369448981345d69dcb556a62c0ce94727ba64b

Comment 8

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Servo PR

Comment 9

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8951557 [details]
Bug 1438497 - Add bindings into memory report.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/220878/diff/1-2/

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/befae37d1035
Add bindings into memory report. r=bholley,njn

Comment 11

[Xidorn Quan [:xidorn] UTC+10]

Hmmm... actually I guess this patch is wrong... I saw some twice-reported in the dmd report after applying this patch. Some further investigation shows that nsXBLDocumentInfo is sharable among documents (somehow I thought it doesn't... and I was actually surprised about that...) and thus it shouldn't be included. But that's really the major part of bindings.

We should probably either backout it, or land some followup if it doesn't bust anything for now...

Comment 12

[Xidorn Quan [:xidorn] UTC+10]

I think I thought nsXBLDocumentInfo is bound to each document because it has a mDocument, and I thought that was the document it is bound to, but actually that is the document which provides the binding...

Comment 13

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #5)
> Comment on attachment 8951557 [details]
> Bug 1438497 - Add bindings into memory report.
> 
> https://reviewboard.mozilla.org/r/220878/#review226966
> 
> I think I did a DMD run, but I didn't do the diffs... Probably I can try
> that. Thanks for the advice.

Actually, I think I did use dmd.py to generate the comparison... And the impression from that time was that XBL seems to be a significant part of unreported memory.

Comment 14

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1438497 part 2 - Do not report bindings which are cached in XUL prototype cache.

Review commit: https://reviewboard.mozilla.org/r/221340/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/221340/

Comment 15

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1438497 part 3 - Add XUL prototype cache to memory report.

Review commit: https://reviewboard.mozilla.org/r/221342/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/221342/

Comment 16

[Xidorn Quan [:xidorn] UTC+10]

Actually stylesheets in xul cache also seems non-trivial, but it's quite tricky to count...

Comment 17

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/befae37d1035

Comment 18

[Bobby Holley (:bholley)]

Comment on attachment 8952109 [details]
Bug 1438497 part 2 - Do not report bindings which are cached in XUL prototype cache.

https://reviewboard.mozilla.org/r/221340/#review227244

I don't have good intuition on the propotion of bindings that are in the cache, but I'm assuming you're getting decent results with DMD.

Comment 19

[Nicholas Nethercote [inactive]]

Comment on attachment 8952110 [details]
Bug 1438497 part 3 - Add XUL prototype cache to memory report.

https://reviewboard.mozilla.org/r/221342/#review227248

r=me with the one issue below fixed.

::: dom/xul/nsXULPrototypeCache.cpp:669
(Diff revision 1)
> +{
> +  if (!sInstance) {
> +    return;
> +  }
> +
> +  MallocSizeOf mallocSizeOf = moz_malloc_size_of;

DMD doesn't get told about measurements done by `moz_malloc_size_of`.

Please use `MOZ_DEFINE_MALLOC_SIZE_OF` to generate a `moz_malloc_size_of`-like function that has DMD hooks in it, and use it here instead.

Comment 20

[Nicholas Nethercote [inactive]]

BTW, Xidorn: if you haven't already seen it, you might find https://developer.mozilla.org/en-US/docs/Mozilla/Performance/Memory_reporting helpful.

For example, you might be able to use the MOZ_COLLECT_REPORT macro.

Comment 21

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Bobby Holley (:bholley) from comment #18)
> Comment on attachment 8952109 [details]
> Bug 1438497 part 2 - Do not report bindings which are cached in XUL
> prototype cache.
> 
> https://reviewboard.mozilla.org/r/221340/#review227244
> 
> I don't have good intuition on the propotion of bindings that are in the
> cache, but I'm assuming you're getting decent results with DMD.

Actually, when the cache is enabled, basically all bindings are in that cache, and the document's binding would be left only the size of the tables, although those tables themselves would take ~15KB for browser.xul.

Comment 22

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8952110 [details]
Bug 1438497 part 3 - Add XUL prototype cache to memory report.

https://reviewboard.mozilla.org/r/221342/#review227248

> DMD doesn't get told about measurements done by `moz_malloc_size_of`.
> 
> Please use `MOZ_DEFINE_MALLOC_SIZE_OF` to generate a `moz_malloc_size_of`-like function that has DMD hooks in it, and use it here instead.

I'll fix that, but the reason you mentioned doesn't really match what I saw. It seems to me that DMD correctly catches things I report, because I saw twice-report from there.

Comment 23

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8952109 [details]
Bug 1438497 part 2 - Do not report bindings which are cached in XUL prototype cache.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/221340/diff/1-2/

Comment 24

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8952110 [details]
Bug 1438497 part 3 - Add XUL prototype cache to memory report.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/221342/diff/1-2/

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2c061c1f3319
part 2 - Do not report bindings which are cached in XUL prototype cache. r=bholley
https://hg.mozilla.org/integration/autoland/rev/240ccd1d0375
part 3 - Add XUL prototype cache to memory report. r=njn

Comment 26

[Nicholas Nethercote [inactive]]

> > Please use `MOZ_DEFINE_MALLOC_SIZE_OF` to generate a `moz_malloc_size_of`-like function that has DMD hooks in it, and use it here instead.
> 
> I'll fix that, but the reason you mentioned doesn't really match what I saw.
> It seems to me that DMD correctly catches things I report, because I saw
> twice-report from there.

There must be another explanation... the definition of moz_malloc_size_of (and moz_malloc_usable_size) is here:

https://searchfox.org/mozilla-central/source/memory/mozalloc/mozalloc.cpp#143-164

There are no DMD hooks there. In comparison, the definition of MOZ_DEFINE_MALLOC_SIZE_OF is here:

https://searchfox.org/mozilla-central/source/xpcom/base/nsIMemoryReporter.idl#531-563

It calls MOZ_REPORT, which has the DMD hooks in a DMD-enabled build.

Could these allocations be reported to DMD via a different path than what you expect? The data structures involved here do seem to be complicated...

Comment 27

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Nicholas Nethercote [:njn] from comment #26)
> Could these allocations be reported to DMD via a different path than what
> you expect? The data structures involved here do seem to be complicated...

But DMD reports exactly paths from this function. (There were cases where the same stylesheet gets reported twice from the function itself...)

Comment 28

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/2c061c1f3319
https://hg.mozilla.org/mozilla-central/rev/240ccd1d0375