Closed
Bug 1477579
Opened 6 years ago
Closed 6 years ago
Optimize memory usage in component/category manager data structures
Categories
(Core :: XPCOM, enhancement)
Core
XPCOM
Tracking
()
RESOLVED
FIXED
mozilla63
Tracking | Status | |
---|---|---|
firefox63 | --- | fixed |
People
(Reporter: kmag, Assigned: kmag)
References
(Blocks 1 open bug)
Details
(Whiteboard: [overhead:50K])
Attachments
(3 files)
Right now, we use over 200K per content process for the component and category managers. Ideally, we should probably significantly reduce the number of components we register, and probably use static hash tables for non-JS components. But we can also make significant gains by just optimizing some of our data structures.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 4•6 years ago
|
||
mozreview-review |
Comment on attachment 8994040 [details]
Bug 1477579: Part 2 - Use nsID* rather than nsID for mFactories hash keys.
https://reviewboard.mozilla.org/r/258618/#review265574
Code analysis found 1 defect in this patch:
- 1 defect found by clang-tidy
You can run this analysis locally with:
- `./mach static-analysis check path/to/file.cpp` (C/C++)
If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx
::: xpcom/ds/nsHashKeys.h:489
(Diff revision 1)
> + typedef const nsID* KeyType;
> + typedef const nsID* KeyTypePointer;
> +
> + explicit nsIDPointerHashKey(const nsID* aInID) : mID(aInID) {}
> + nsIDPointerHashKey(const nsIDPointerHashKey& aToCopy) : mID(aToCopy.mID) {}
> + ~nsIDPointerHashKey() {}
Warning: Use '= default' to define a trivial destructor [clang-tidy: modernize-use-equals-default]
~nsIDPointerHashKey() {}
^ ~~
= default;
Comment 5•6 years ago
|
||
mozreview-review |
Comment on attachment 8994039 [details]
Bug 1477579: Part 1 - Use literal strings for statically registered contract ID keys.
https://reviewboard.mozilla.org/r/258616/#review265694
::: xpcom/components/nsComponentManager.cpp:494
(Diff revision 1)
> +{
> + // The main thread's stack should be allocated at the top of our address
> + // space. Anything stack allocated should be above us on the stack, and
> + // therefore above our first argument pointer.
> + MOZ_ASSERT(NS_IsMainThread());
> + MOZ_ASSERT((char*)aPtr < (char*)&aPtr);
This is technically undefined behavior, since `aPtr` and `&aPtr` aren't pointers to (or just past) the same object. If you want to do it more legitimately, you need to cast to `uintptr_t` first.
Attachment #8994039 -
Flags: review?(nfroyd) → review+
Comment 6•6 years ago
|
||
mozreview-review |
Comment on attachment 8994040 [details]
Bug 1477579: Part 2 - Use nsID* rather than nsID for mFactories hash keys.
https://reviewboard.mozilla.org/r/258618/#review265700
Attachment #8994040 -
Flags: review?(nfroyd) → review+
Comment 7•6 years ago
|
||
mozreview-review |
Comment on attachment 8994041 [details]
Bug 1477579: Part 3 - Avoid duplicating static strings in category manager entries.
https://reviewboard.mozilla.org/r/258620/#review265702
Nice. How much does this win back?
::: xpcom/components/nsICategoryManager.idl:18
(Diff revision 1)
> +
> /*
> * nsICategoryManager
> */
>
> [scriptable, uuid(3275b2cd-af6d-429a-80d7-f0c5120342ac)]
Shall we just make this `builtinclass` while we're here? Or should that be a followup?
::: xpcom/components/nsICategoryManager.idl:74
(Diff revision 1)
> * nsISupportsCString.
> */
> nsISimpleEnumerator enumerateCategories();
> +
> + %{C++
> + template<int N>
I think the usual way we do this is with `size_t N` arguments. I don't think it makes any difference, but consistency is nice?
Attachment #8994041 -
Flags: review?(nfroyd) → review+
Assignee | ||
Comment 8•6 years ago
|
||
(In reply to Nathan Froyd [:froydnj] from comment #7)
> Nice. How much does this win back?
About 8K. Or, probably exactly 8K, since that's the chunk size for the arena all of this stuff is allocated in.
> ::: xpcom/components/nsICategoryManager.idl:18
> (Diff revision 1)
> > +
> > /*
> > * nsICategoryManager
> > */
> >
> > [scriptable, uuid(3275b2cd-af6d-429a-80d7-f0c5120342ac)]
>
> Shall we just make this `builtinclass` while we're here? Or should that be
> a followup?
Yeah, may as well, I suppose. Although it still works with JS wrappers as is.
Assignee | ||
Comment 9•6 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/8359f8fe418419c50ab0ed93496e7445b570ba9f
Bug 1477579: Part 1 - Use literal strings for statically registered contract ID keys. r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/5fb0b7746a5d56563b471e3061ccca124ea45485
Bug 1477579: Part 2 - Use nsID* rather than nsID for mFactories hash keys. r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/aa9a8f18e98f930a3d8359565eef02f3f6efc5f9
Bug 1477579: Part 3 - Avoid duplicating static strings in category manager entries. r=froydnj
Comment 10•6 years ago
|
||
Backed out 3 changesets (bug 1477579) for build bustages on xpcshell\selftest.py and crashtest failures on /components/nsComponentManager.cpp.
Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/1e02e6dcacf7568d373270178fe89548952bc2aa
Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=aa9a8f18e98f930a3d8359565eef02f3f6efc5f9&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=success&filter-resultStatus=pending&filter-resultStatus=running&selectedJob=189685876
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=189685876&repo=mozilla-inbound&lineNumber=1413
https://treeherder.mozilla.org/logviewer.html#?job_id=189681694&repo=mozilla-inbound&lineNumber=66769
Flags: needinfo?(kmaglione+bmo)
Assignee | ||
Comment 11•6 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/71f70ce6f9314111dc05fe8dceaac43edfda6659
Bug 1477579: Part 1 - Use literal strings for statically registered contract ID keys. r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/a431ef7d271b44d602ae7c403a91ae909789b2f0
Bug 1477579: Part 2 - Use nsID* rather than nsID for mFactories hash keys. r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/7d93ec7199583737c72bdc43e9c36ba2152829d2
Bug 1477579: Part 3 - Avoid duplicating static strings in category manager entries. r=froydnj
Assignee | ||
Comment 12•6 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/750d3a0ab6f054153dc5716e680577cbb9112527
Bug 1477579: Follow-up: Also skip stack allocation assertion on Android. r=me DONTBUILD
Comment 13•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/71f70ce6f931
https://hg.mozilla.org/mozilla-central/rev/a431ef7d271b
https://hg.mozilla.org/mozilla-central/rev/7d93ec719958
https://hg.mozilla.org/mozilla-central/rev/750d3a0ab6f0
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox63:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Updated•6 years ago
|
Flags: needinfo?(kmaglione+bmo)
You need to log in
before you can comment on or make changes to this bug.
Description
•