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

Status: RESOLVED FIXED

(Core :: XPCOM, enhancement)

Description

[Kris Maglione [:kmag]]

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).

Comment 1

[Eric Rahm [:erahm]]

Kris, do you have any idea how much overhead this is?

Comment 2

[Kris Maglione [:kmag]]

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.

Comment 3

[Kris Maglione [:kmag]]

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.

Comment 4

[Nika Layzell [:nika] (ni? for response)]

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.

Comment 5

[Mark Banner (:standard8)]

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

Comment 6

[(no longer active)]

FWIW Anny's partial work in progress patch for this bug is posted here: https://bug1489024.bmoattachments.org/attachment.cgi?id=9015611

Comment 7

[Kris Maglione [:kmag]]

Attachment: Bug 1464542: Part 1 - Generate symbolic names for XPT interfaces and add lookup function. r=nika

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

Comment 8

[Kris Maglione [:kmag]]

Attachment: Bug 1464542: Part 2 - Add infrastructure for defining named services exposed to JS. r=mccr8

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.

Comment 9

[Kris Maglione [:kmag]]

Attachment: Bug 1464542: Part 3a - Modernize enterprise policy registration. r=mkaply

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.

Comment 10

[Kris Maglione [:kmag]]

Attachment: Bug 1464542: Part 3b - Add existing Services.jsm registrations to the new services cache. r=mccr8

Comment 11

[Kris Maglione [:kmag]]

Attachment: Bug 1464542: Part 3c - Change Services.jsm to use the C++-implemented services cache. r=mccr8

Comment 12

[Pulsebot]

Pushed by [e-mail]:
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

Comment 13

[Kris Maglione [:kmag]]

Attachment: Bug 1464542: Temporarily remove user-services.js test to fix bustage. r=bustage

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b50af9005851
Temporarily remove user-services.js test to fix bustage.

Comment 15

[Natalia Csoregi [:nataliaCs]]

Backed out for xpcshell failures on test_Services.js

backout: https://hg.mozilla.org/integration/autoland/rev/07ab7d77bf31bbf3e74f8aab2b4cccf29b971681

push: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=9d3a0ea2cf657a837ad38f43e3cad5eb498fad8f&searchStr=os%2Cx%2C10.14%2Cdebug%2Cxpcshell%2Ctests%2Ctest-macosx1014-64%2Fdebug-xpcshell-e10s-1%2Cx%28x1%29

failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=309191685&repo=autoland&lineNumber=5513

Comment 16

[Natalia Csoregi [:nataliaCs]]

Also failing https://treeherder.mozilla.org/#/jobs?repo=autoland&fromchange=6c8547a37a638627bfef90a36236edd74f5ffaf7&searchStr=a11y&selectedTaskRun=TZvIeSRvQ5-Ycch6IKZlJg.0

Push with other failures : https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=366f7bfe267cb4a7781c41cdb5a691d75c018622&searchStr=os%2Cx&group_state=expanded&selectedTaskRun=RLBCPM0iQNWSKuGJvtqpew.0

Comment 17

[Pulsebot]

Pushed by [e-mail]:
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

Comment 18

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/e3dc107c8af8
https://hg.mozilla.org/mozilla-central/rev/98b3ba622432
https://hg.mozilla.org/mozilla-central/rev/f583d66b2813
https://hg.mozilla.org/mozilla-central/rev/4fdeb4d64741
https://hg.mozilla.org/mozilla-central/rev/671c2fd7e5a9

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/753b43f99f9f
Port bug 1464542 Part 3a - Modernize enterprise policy registration. rs=bustage-fix

Comment 20

[Florin Strugariu [:Bebe]]

== 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

Comment 21

[:Gijs (out for now; he/him)]

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.)

Comment 22

[Kris Maglione [:kmag]]

(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.