Closed Bug 1382645 Opened 7 years ago Closed 7 years ago

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

Categories

(WebExtensions :: General, defect)

defect
Not set
normal

Tracking

(firefox-esr52 unaffected, firefox54 unaffected, firefox55 unaffected, firefox56 fixed)

RESOLVED FIXED
mozilla56
Tracking Status
firefox-esr52 --- unaffected
firefox54 --- unaffected
firefox55 --- unaffected
firefox56 --- fixed

People

(Reporter: igoldan, Assigned: kmag)

References

Details

(Keywords: perf, regression)

Attachments

(3 files)

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
Component: Untriaged → WebExtensions: General
Product: Firefox → Toolkit
:kmag I see you are the owner of this related bug. Can you look over this memory regression?
Flags: needinfo?(kmaglione+bmo)
I suppose we need to improve memory reporting for StructuredCloneHolder...
Flags: needinfo?(kmaglione+bmo)
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 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+
Attachment #8888529 - Flags: review?(mixedpuppy) → review+
Comment on attachment 8888537 [details]
Bug 1382645: Part 3 - Throw away description strings before blobbifying schema JSON.

https://reviewboard.mozilla.org/r/159514/#review165226
Attachment #8888537 - Flags: review?(mixedpuppy) → review+
:kmag Can you provide update for this bug? We would like to conclude this until Monday's merge day.
Flags: needinfo?(kmaglione+bmo)
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.
Attachment #8888528 - Flags: review?(wmccloskey) → review+
(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.
Assignee: nobody → kmaglione+bmo
Flags: needinfo?(kmaglione+bmo)
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
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
Flags: needinfo?(kmaglione+bmo)
Unless I'm missing something, there were no JS regressions, only improvements.
Flags: needinfo?(kmaglione+bmo)
(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.
Product: Toolkit → WebExtensions
Blocks: 1381711
You need to log in before you can comment on or make changes to this bug.