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)
WebExtensions
General
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
Reporter | ||
Updated•7 years ago
|
Component: Untriaged → WebExtensions: General
Product: Firefox → Toolkit
Reporter | ||
Comment 1•7 years ago
|
||
:kmag I see you are the owner of this related bug. Can you look over this memory regression?
Flags: needinfo?(kmaglione+bmo)
Assignee | ||
Comment 2•7 years ago
|
||
I suppose we need to improve memory reporting for StructuredCloneHolder...
Flags: needinfo?(kmaglione+bmo)
Comment 3•7 years ago
|
||
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 6•7 years ago
|
||
mozreview-review |
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 9•7 years ago
|
||
mozreview-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+
Reporter | ||
Comment 10•7 years ago
|
||
:kmag Can you provide update for this bug? We would like to conclude this until Monday's merge day.
Flags: needinfo?(kmaglione+bmo)
Comment 11•7 years ago
|
||
mozreview-review |
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+
Assignee | ||
Comment 12•7 years ago
|
||
(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)
Assignee | ||
Comment 13•7 years ago
|
||
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•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/6e59c41b3584 https://hg.mozilla.org/mozilla-central/rev/f6dc4970d280 https://hg.mozilla.org/mozilla-central/rev/f39eaceb3ca0
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Reporter | ||
Comment 15•7 years ago
|
||
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
Reporter | ||
Updated•7 years ago
|
Flags: needinfo?(kmaglione+bmo)
Assignee | ||
Comment 16•7 years ago
|
||
Unless I'm missing something, there were no JS regressions, only improvements.
Flags: needinfo?(kmaglione+bmo)
Updated•7 years ago
|
status-firefox54:
--- → unaffected
status-firefox55:
--- → unaffected
status-firefox-esr52:
--- → unaffected
Reporter | ||
Comment 17•7 years ago
|
||
(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.
Updated•6 years ago
|
Product: Toolkit → WebExtensions
You need to log in
before you can comment on or make changes to this bug.
Description
•