Closed Bug 1438497 Opened 2 years ago Closed 2 years ago

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

Categories

(Core :: CSS Parsing and Computation, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: xidorn, Assigned: xidorn)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 1 obsolete file)

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.
Assignee: nobody → xidorn+moz
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.
Depends on: 1436059
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.
Attachment #8951557 - Flags: review?(bobbyholley) → review+
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.
Attachment #8951557 - Flags: review?(n.nethercote) → review+
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 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.
Attached file Servo PR
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/befae37d1035
Add bindings into memory report. r=bholley,njn
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...
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...
Keywords: leave-open
(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.
Attachment #8951557 - Attachment is obsolete: true
Attachment #8952109 - Flags: review?(bobbyholley)
Actually stylesheets in xul cache also seems non-trivial, but it's quite tricky to count...
Blocks: 1439366
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.
Attachment #8952109 - Flags: review?(bobbyholley) → review+
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.
Attachment #8952110 - Flags: review?(n.nethercote) → review+
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.
(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 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.
Pushed by xquan@mozilla.com:
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
Keywords: leave-open
> > 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...
(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...)
Blocks: 1439507
https://hg.mozilla.org/mozilla-central/rev/2c061c1f3319
https://hg.mozilla.org/mozilla-central/rev/240ccd1d0375
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.