1382645 - 5.35 - 7.07% Heap Unclassified (linux64, windows10-64, windows7-32) regression on push 7066bee0381d62764ba8fc55aa4270eb5a241b20 (Thu Jul 20 2017)

Status: RESOLVED FIXED

(WebExtensions :: General, defect)

Description

[Ionuț Goldan [:igoldan]]

We have detected an awsy regression from push:

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=7066bee0381d62764ba8fc55aa4270eb5a241b20

As author of one of the patches included in that push, we need your help to address this regression.

Regressions:

  7%  Heap Unclassified summary windows7-32 opt      46,301,621.60 -> 49,574,527.56
  7%  Heap Unclassified summary linux64 opt          60,570,945.75 -> 64,626,345.64
  5%  Heap Unclassified summary windows10-64 opt     53,533,795.39 -> 56,395,692.82

Improvements:

  2%  JS summary windows10-64 opt      138,445,412.80 -> 135,404,053.83
  1%  JS summary windows7-32 opt       102,627,838.76 -> 101,108,714.58


You can find links to graphs and comparison views for each of the above tests at: https://treeherder.mozilla.org/perf.html#/alerts?id=8105

On the page above you can see an alert for each affected platform as well as a link to a graph showing the history of scores for this test. There is also a link to a treeherder page showing the jobs in a pushlog format.

To learn more about the regressing test(s), please see: https://developer.mozilla.org/en-US/docs/Mozilla/Performance/AWSY

Comment 1

[Ionuț Goldan [:igoldan]]

:kmag I see you are the owner of this related bug. Can you look over this memory regression?

Comment 2

[Kris Maglione [:kmag]]

I suppose we need to improve memory reporting for StructuredCloneHolder...

Comment 3

[Joel Maher ( :jmaher ) (UTC -8)]

I see a talos improvement here:
== Change summary for alert #8106 (as of July 20 2017 04:10 UTC) ==

Improvements:

  4%  ts_paint_webext windows7-32 opt e10s     1,067.33 -> 1,028.42

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

Comment 4

[Kris Maglione [:kmag]]

Attachment: Bug 1382645: Part 1 - Add memory reporter for StructuredCloneHolder binding implementation.

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

Comment 5

[Kris Maglione [:kmag]]

Attachment: Bug 1382645: Part 2 - Throw away schema StructuredCloneHolders in content process after deserialization.

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

Comment 6

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8888529 [details]
Bug 1382645: Part 2 - Throw away schema StructuredCloneHolders in content process after deserialization.

https://reviewboard.mozilla.org/r/159508/#review164856

I'm not super familiar with our schema code, however this is quite self explanatory, and I beleive WebExt would generally blow up if it were wrong.  So r+

Comment 7

[Kris Maglione [:kmag]]

Attachment: Bug 1382645: Part 3 - Throw away description strings before blobbifying schema JSON.

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

Comment 8

[Kris Maglione [:kmag]]

Comment on attachment 8888537 [details]
Bug 1382645: Part 3 - Throw away description strings before blobbifying schema JSON.

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

Comment 9

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8888537 [details]
Bug 1382645: Part 3 - Throw away description strings before blobbifying schema JSON.

https://reviewboard.mozilla.org/r/159514/#review165226

Comment 10

[Ionuț Goldan [:igoldan]]

:kmag Can you provide update for this bug? We would like to conclude this until Monday's merge day.

Comment 11

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8888528 [details]
Bug 1382645: Part 1 - Add memory reporter for StructuredCloneHolder binding implementation.

https://reviewboard.mozilla.org/r/159506/#review166354

::: dom/base/StructuredCloneBlob.cpp:195
(Diff revision 1)
> +NS_IMETHODIMP
> +StructuredCloneBlob::CollectReports(nsIHandleReportCallback* aHandleReport,
> +                                    nsISupports* aData, bool aAnonymize)
> +{
> +  MOZ_COLLECT_REPORT(
> +    "explicit/dom/structured-clone-holder", KIND_HEAP, UNITS_BYTES,

Why not call it structured-clone-blob? StructuredCloneHolder is an existing thing that's not really related to this, which is confusing.

::: dom/base/StructuredCloneBlob.cpp:197
(Diff revision 1)
> +                                    nsISupports* aData, bool aAnonymize)
> +{
> +  MOZ_COLLECT_REPORT(
> +    "explicit/dom/structured-clone-holder", KIND_HEAP, UNITS_BYTES,
> +    MallocSizeOf(this) + SizeOfExcludingThis(MallocSizeOf),
> +    "Memory used by StructuredCloneHolder DOM objects.");

Same here.

::: dom/base/StructuredCloneHolder.h:120
(Diff revision 1)
>    {
>      MOZ_ASSERT(mBuffer, "Write() has never been called.");
>      return mBuffer->data();
>    }
>  
> +  size_t SizeOfExcludingThis(mozilla::MallocSizeOf aMallocSizeOf)

Please add a comment to StructuredCloneHolder saying that it inherits SizeOfExcludingThis from StructuredCloneHolderBase, and that fields like mBlobImplArray are ignored because... well, I guess we don't expect them to be significant for StructuredCloneBlobs. Hopefully no one starts using this code for anything else.

Comment 12

[Kris Maglione [:kmag]]

(In reply to Bill McCloskey (:billm) from comment #11)
> Comment on attachment 8888528 [details]
> Bug 1382645: Part 1 - Add memory reporter for StructuredCloneHolder binding
> implementation.
> 
> https://reviewboard.mozilla.org/r/159506/#review166354
> 
> ::: dom/base/StructuredCloneBlob.cpp:195
> (Diff revision 1)
> > +NS_IMETHODIMP
> > +StructuredCloneBlob::CollectReports(nsIHandleReportCallback* aHandleReport,
> > +                                    nsISupports* aData, bool aAnonymize)
> > +{
> > +  MOZ_COLLECT_REPORT(
> > +    "explicit/dom/structured-clone-holder", KIND_HEAP, UNITS_BYTES,
> 
> Why not call it structured-clone-blob? StructuredCloneHolder is an existing
> thing that's not really related to this, which is confusing.
> 
> ::: dom/base/StructuredCloneBlob.cpp:197
> (Diff revision 1)
> > +                                    nsISupports* aData, bool aAnonymize)
> > +{
> > +  MOZ_COLLECT_REPORT(
> > +    "explicit/dom/structured-clone-holder", KIND_HEAP, UNITS_BYTES,
> > +    MallocSizeOf(this) + SizeOfExcludingThis(MallocSizeOf),
> > +    "Memory used by StructuredCloneHolder DOM objects.");
> 
> Same here.

The class is exposed to JS as StructuredCloneHolder since it's basically a JS reflection of the built-in StructuredCloneHolder class. I suppose at this point we may as well just rename it StructuredCloneBlob to match the native binding, but I think that should be a separate bug.

> ::: dom/base/StructuredCloneHolder.h:120
> (Diff revision 1)
> >    {
> >      MOZ_ASSERT(mBuffer, "Write() has never been called.");
> >      return mBuffer->data();
> >    }
> >  
> > +  size_t SizeOfExcludingThis(mozilla::MallocSizeOf aMallocSizeOf)
> 
> Please add a comment to StructuredCloneHolder saying that it inherits
> SizeOfExcludingThis from StructuredCloneHolderBase, and that fields like
> mBlobImplArray are ignored because... well, I guess we don't expect them to
> be significant for StructuredCloneBlobs. Hopefully no one starts using this
> code for anything else.

I actually ignored them because they might be shared, and MemoryBlobImpls have their own memory reporters. For other BlobImpls, the amount of memory they hold is quite small.

Comment 13

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6e59c41b3584fb692f0237701034cea162d5df8b
Bug 1382645: Part 1 - Add memory reporter for StructuredCloneHolder binding implementation. r=billm

https://hg.mozilla.org/integration/mozilla-inbound/rev/f6dc4970d2803673baab24a23f3dea9a08fbd342
Bug 1382645: Part 2 - Throw away schema StructuredCloneHolders in content process after deserialization. r=mixedpuppy

https://hg.mozilla.org/integration/mozilla-inbound/rev/f39eaceb3ca0d81717297e52e493eb2fca7df2ea
Bug 1382645: Part 3 - Throw away description strings before blobbifying schema JSON. r=mixedpuppy

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6e59c41b3584
https://hg.mozilla.org/mozilla-central/rev/f6dc4970d280
https://hg.mozilla.org/mozilla-central/rev/f39eaceb3ca0

Comment 15

[Ionuț Goldan [:igoldan]]

This fixed the Heap regressions. But the JS ones are still there; can we improve on those too, or should we accept them?

== Change summary for alert #8287 (as of July 25 2017 21:27 UTC) ==

Improvements:

 11%  Heap Unclassified summary windows7-32 pgo      52,868,846.87 -> 47,062,847.76
  6%  Heap Unclassified summary windows7-32 opt      53,732,518.48 -> 50,689,895.38
  5%  Heap Unclassified summary windows10-64 opt     61,352,856.34 -> 58,585,439.27
  4%  Heap Unclassified summary windows10-64 pgo     61,329,279.07 -> 58,651,399.02

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

Comment 16

[Kris Maglione [:kmag]]

Unless I'm missing something, there were no JS regressions, only improvements.

Comment 17

[Ionuț Goldan [:igoldan]]

(In reply to Kris Maglione [:kmag] from comment #16)
> Unless I'm missing something, there were no JS regressions, only
> improvements.

You are right. I misread the description of this bug.