Closed Bug 1464542 Opened 4 years ago Closed 2 years ago

Migrate Services.jsm to C++ and include common constructors

Categories

(Core :: XPCOM, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla80
Tracking Status
firefox80 --- fixed

People

(Reporter: kmag, Assigned: kmag)

References

(Blocks 2 open bugs)

Details

(Keywords: perf-alert, Whiteboard: [overhead:20k])

Attachments

(6 files)

According to the results in bug 1463569, we use a fair amount of memory in every process allocating reflectors for objects in Cc and Ci.

If we migrate Services.jsm to C++, we won't need to touch the JS reflectors for those classes or interfaces. And if we also add constructors for common classes that we createInstance, that should reduce the number of reflectors we need to create even more (to say nothing of cleaning up all of the XPCOM gunk that's currently required to create those instances).
Kris, do you have any idea how much overhead this is?
Flags: needinfo?(kmaglione+bmo)
It's hard to say. Just going by the retained size of Services.jsm and the various Cc and Ci classes from mccr8's memory tool, I'd say somewhere between 50 and 100K in a base content process.

Fully realizing that probably means making sure we don't touch Ci more than we have to, and probably optimizing generateQI to lookup IIDs directly rather than going through Ci.
Flags: needinfo?(kmaglione+bmo)
Whiteboard: [overhead:100k]
Looking again at the most recent output, this is probably closer to 20-50K. But I'm pretty confident of at least 20 if we can actually get rid of most of the Ci/Cc references and get everything using Services.
Whiteboard: [overhead:100k] → [overhead:20k]
Ideally this could be merged into the c++ generated from Services.py so C++ and JS code can share caches.

https://searchfox.org/mozilla-central/source/xpcom/build/Services.py

FYI I also have some WIP patches which should make Ci and Cc much cheaper in terms of both performance and memory use, so some of that overhead might disappear.
Note: if we do this, ideally this should be done in a way that makes it easy for ESLint to know what services are exposed as we already have a rule to ensure the use of it where possible:

https://searchfox.org/mozilla-central/rev/3fdc491e118c5cdfbaf6e2d52f3466d2b27ad1de/tools/lint/eslint/eslint-plugin-mozilla/lib/rules/use-services.js
Assignee: nobody → annygakhokidze
See Also: → 1425524
FWIW Anny's partial work in progress patch for this bug is posted here: https://bug1489024.bmoattachments.org/attachment.cgi?id=9015611
Assignee: agakhokidze → kmaglione+bmo

This makes it much simpler for the for the static JS services cache to store
and lookup interface IDs for components.

This builds on the existing static components infrastructure to allow defining
a Services.jsm-type services cache with no runtime memory overhead for any
services until they're accessed.

Any class entry with a 'js_name' attribute automatically becomes available on
the services cache with that name, and any interfaces listed in its
'interfaces' list are automatically queried on it.

Aside from making registration somewhat more efficient, this allows us to make
the services available on the new C++-implemented JS Services object, which
requires services to use the new static component registration system.

Pushed by maglione.k@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/bbecc16d08eb
Part 1 - Generate symbolic names for XPT interfaces and add lookup function. r=nika,froydnj
https://hg.mozilla.org/integration/autoland/rev/e84de1547c09
Part 2 - Add infrastructure for defining named services exposed to JS. r=mccr8
https://hg.mozilla.org/integration/autoland/rev/51ff93220a95
Part 3a - Modernize enterprise policy registration. r=mkaply
https://hg.mozilla.org/integration/autoland/rev/71c3475fcbc2
Part 3b - Add existing Services.jsm registrations to the new services cache. r=mccr8,geckoview-reviewers,agi
https://hg.mozilla.org/integration/autoland/rev/9d3a0ea2cf65
Part 3c - Change Services.jsm to use the C++-implemented services cache. r=mccr8
Pushed by maglione.k@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/b50af9005851
Temporarily remove user-services.js test to fix bustage.
Regressions: 1651774
Pushed by maglione.k@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/e3dc107c8af8
Part 1 - Generate symbolic names for XPT interfaces and add lookup function. r=nika,froydnj
https://hg.mozilla.org/integration/autoland/rev/98b3ba622432
Part 2 - Add infrastructure for defining named services exposed to JS. r=mccr8
https://hg.mozilla.org/integration/autoland/rev/f583d66b2813
Part 3a - Modernize enterprise policy registration. r=mkaply
https://hg.mozilla.org/integration/autoland/rev/4fdeb4d64741
Part 3b - Add existing Services.jsm registrations to the new services cache. r=mccr8,geckoview-reviewers,agi
https://hg.mozilla.org/integration/autoland/rev/671c2fd7e5a9
Part 3c - Change Services.jsm to use the C++-implemented services cache. r=mccr8
Pushed by richard.marti@gmail.com:
https://hg.mozilla.org/comm-central/rev/753b43f99f9f
Port bug 1464542 Part 3a - Modernize enterprise policy registration. rs=bustage-fix
Regressions: 1652029

== Change summary for alert #26476 (as of Fri, 10 Jul 2020 13:05:53 GMT) ==

Improvements:

1% Base Content JS macosx1014-64-shippable opt 3,551,187.00 -> 3,507,422.00
1% Base Content JS windows10-64-shippable opt 3,603,884.33 -> 3,567,582.00
1% Base Content JS windows10-64-shippable-qr opt 3,588,425.33 -> 3,554,250.00
1% Base Content JS windows7-32-shippable opt 2,761,914.67 -> 2,735,881.33
1% Base Content JS linux1804-64-shippable opt 3,527,639.67 -> 3,500,949.33
1% Base Content JS linux1804-64-shippable-qr opt 3,527,635.33 -> 3,500,932.00

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

Keywords: perf-alert
Blocks: 1651774
No longer regressions: 1651774
Flags: needinfo?(kmaglione+bmo)
Regressions: 1658188

This is great!

I'm wondering if we can use something like what mccr8 did in bug 767640 to make the property always available, instead of everywhere having to manually ChromeUtils.import Services.jsm ?

(Happy to file a follow-up bug, but I don't know if there was some reason it didn't happen here that I should be aware of.)

Flags: needinfo?(kmaglione+bmo)

(In reply to :Gijs (he/him) from comment #21)

I'm wondering if we can use something like what mccr8 did in bug 767640 to make the property always available, instead of everywhere having to manually ChromeUtils.import Services.jsm ?

(Happy to file a follow-up bug, but I don't know if there was some reason it didn't happen here that I should be aware of.)

Yes. I was considering it as a follow-up, but I wanted to do the simplest thing for the initial version since it already had enough chance of breaking things or landing with conflicts.

Feel free to file a follow-up.

Flags: needinfo?(kmaglione+bmo)
Blocks: 1667455
You need to log in before you can comment on or make changes to this bug.