Closed Bug 1477579 Opened 2 years ago Closed 2 years ago

Optimize memory usage in component/category manager data structures

Categories

(Core :: XPCOM, enhancement)

enhancement
Not set

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 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 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 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 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+
(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.
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
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
https://hg.mozilla.org/integration/mozilla-inbound/rev/750d3a0ab6f054153dc5716e680577cbb9112527
Bug 1477579: Follow-up: Also skip stack allocation assertion on Android. r=me DONTBUILD
See Also: → 1478124
Flags: needinfo?(kmaglione+bmo)
You need to log in before you can comment on or make changes to this bug.