Closed Bug 886907 (killSyncContentPrefs) Opened 7 years ago Closed 2 years ago

Remove old synchronous contentPrefService from the tree

Categories

(Toolkit :: General, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla57
Iteration:
57.2 - Aug 29
Tracking Status
firefox57 --- fixed

People

(Reporter: mak, Assigned: emk, Mentored)

References

(Blocks 3 open bugs)

Details

(Keywords: addon-compat, Whiteboard: [reserve-photon-performance] [good first bug] [lang=js] [qf:p2])

Attachments

(1 file, 1 obsolete file)

a couple versions after the deprecation in bug 850219 we should remove the old synchronous service and, if possible, merge the new service module into the service. As reported in the module:

17 // The plan is to eventually remove nsIContentPrefService and its
18 // implementation, nsContentPrefService.js.  At such time this file can stop
19 // being a JSM, and the "_cps" parts that ContentPrefService2 relies on and
20 // NSGetFactory and all the other XPCOM initialization goop in
21 // nsContentPrefService.js can be moved here.
Blocks: StorageJank
Alias: killSyncContentPrefs
note also Services.contentPrefs, I'm not sure if we should just remove it, replace it, or introduce a contentPrefs2. It is only used in a couple places in our codebase, so maybe it's not worth to have it in Services.
Depends on: 895189
Depends on: 974295
This should also be good to kill the nsContentPrefService.js compartment at startup
No longer blocks: fxdesktoptriage
Whiteboard: p=0
No longer blocks: fxdesktopbacklog
Flags: firefox-backlog+
Whiteboard: p=0 → p=8
Points: --- → 8
Whiteboard: p=8
Drew, would you be interested in mentoring the remaining work here?
Flags: needinfo?(adw)
Sure!
Mentor: adw
Flags: needinfo?(adw)
I wonder if we can call this a "first" bug...
Whiteboard: [good next bug][lang=js]
Drew, is this something easy enough to make it a [good first bug]?
Flags: needinfo?(adw)
Sure, probably. :-)
Flags: needinfo?(adw)
Whiteboard: [good next bug][lang=js] → [good first bug][lang=js]
Blocks: 887889
Raj contacted me through email saying he'd like to work on this bug. I've assigned him.
Assignee: nobody → meghpararajkumar
Hey Raj,

Do you have a build of Firefox on your machine ready to go? If not, we can help you through that step.
Flags: needinfo?(meghpararajkumar)
I've been communicating with Raj over email, and is getting his build set up.

In the meantime, adw, did you have some starting tips for Raj?
Flags: needinfo?(adw)
There's kind of a lot of stuff to do, but fortunately it's pretty straightforward I think.  The steps are below.  And I'm probably forgetting things.  Let me know when you run into problems or need more details.

1. Modify ContentPrefService2.jsm

This file implements nsIContentPrefService2.  There's a separate file, nsContentPrefService.js, that implements the old service we're removing.  There's overlap in the functionality of the old and new service, so currently, to avoid reimplementing that overlap, ContentPrefService2.jsm relies on some implementation bits in nsContentPrefService.js.  We need to copy those bits into ContentPrefService2.jsm so that nsContentPrefService.js can be removed.  Please read the big comment at the top of ContentPrefService2.jsm for some info on that.

Sub-steps:

* Rename ContentPrefService2.jsm to ContentPrefService2.js (take off the "m" at the end): https://dxr.mozilla.org/mozilla-central/source/toolkit/components/contentprefs/ContentPrefService2.jsm

* Remove the big comment starting here: https://dxr.mozilla.org/mozilla-central/source/toolkit/components/contentprefs/ContentPrefService2.jsm#5

* Remove EXPORTED_SYMBOLS: https://dxr.mozilla.org/mozilla-central/source/toolkit/components/contentprefs/ContentPrefService2.jsm#25

* Replace the usage of the _cps member with the bits copied over from nsContentPrefService.js.  That includes the HostnameGrouper prototype from nsContentPrefService.js and the "XPCOM Plumbing" at the bottom of that file.

2. Update nsContentPrefService.manifest

https://dxr.mozilla.org/mozilla-central/source/toolkit/components/contentprefs/nsContentPrefService.manifest

Replace the "nsContentPrefService.js" references in this file with "ContentPrefService2.js".

3. Remove the old service

* Remove the idl file: https://dxr.mozilla.org/mozilla-central/source/dom/interfaces/base/nsIContentPrefService.idl

* Remove the reference to that file here, in this moz.build file: https://dxr.mozilla.org/mozilla-central/source/dom/interfaces/base/moz.build

* Remove this whole directory, which contains tests for the old service: https://dxr.mozilla.org/mozilla-central/source/toolkit/components/contentprefs/tests/unit

* Remove the reference to that directory here: https://dxr.mozilla.org/mozilla-central/source/toolkit/components/contentprefs/moz.build#8

* Remove ContentPrefInstance.jsm, since it's a companion to the old service, and there aren't any non-test consumers of it in m-c anymore: https://dxr.mozilla.org/mozilla-central/source/toolkit/components/contentprefs/ContentPrefInstance.jsm

* Remove the reference to that file here: https://dxr.mozilla.org/mozilla-central/source/toolkit/components/contentprefs/moz.build#22

* Remove this test file: https://dxr.mozilla.org/mozilla-central/source/toolkit/components/contentprefs/tests/unit_cps2/test_service.js

* Remove the reference to that file here, in the tests manifest: https://dxr.mozilla.org/mozilla-central/source/toolkit/components/contentprefs/tests/unit_cps2/xpcshell.ini#7

4. Update places that reference the old service

* Find cpp files that #include nsIContentPrefService.h, remove the #include, and if the file does not already #include nsIContentPrefService2.h (note the 2), add an #include for it.  You can find those cpp files here (there are only two): https://dxr.mozilla.org/mozilla-central/search?q=%23include%20%22nsIContentPrefService.h%22

* This line should reference nsIContentPrefService2 (the new service), not nsIContentPrefService (the old one): https://dxr.mozilla.org/mozilla-central/source/toolkit/modules/Services.jsm#71

* Same here: https://dxr.mozilla.org/mozilla-central/source/toolkit/modules/tests/xpcshell/test_Services.js#36

* Remove this other deprecated method that relies on the old service: https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/downloads/DownloadLastDir.jsm#113
Status: NEW → ASSIGNED
Flags: needinfo?(adw)
okay sir,@adw Thanks a lot..!!
hello sir @adw in your instructions :
          
Replace the usage of the _cps member with the bits copied over from nsContentPrefService.js.  That includes the HostnameGrouper prototype from nsContentPrefService.js and the "XPCOM Plumbing" at the bottom of that file.

In this instruction, what does _cps member mean ?Sir,Can you please explain this in detail ?
Attached patch All changes are done..!!! (obsolete) — Splinter Review
Hello sir,I've created this uncommited.patch file,please check it and suggest me anything is wrong..Thank you..!!
Comment on attachment 8825048 [details] [diff] [review]
All changes are done..!!!

Requesting review on behalf of Raj.
Attachment #8825048 - Flags: review?(adw)
Whiteboard: [good first bug][lang=js] → [good first bug][lang=js][qf]
Flags: firefox-backlog+ → qe-verify-
Priority: -- → P1
Whiteboard: [good first bug][lang=js][qf] → [photon-performance] [good first bug] [lang=js] [qf]
Points: 8 → ---
Whiteboard: [photon-performance] [good first bug] [lang=js] [qf] → [photon-performance] [good first bug] [lang=js] [qf:p3]
(qf:p3 since this is removing dead code as far as I can tell. ;-)
See Also: → 1361095
Whiteboard: [photon-performance] [good first bug] [lang=js] [qf:p3] → [photon-performance] [good first bug] [lang=js] [qf:p1]
Whiteboard: [photon-performance] [good first bug] [lang=js] [qf:p1] → [reserve-photon-performance] [good first bug] [lang=js] [qf:p1]
Whiteboard: [reserve-photon-performance] [good first bug] [lang=js] [qf:p1] → [reserve-photon-performance] [good first bug] [lang=js] [qf:p2]
Drew, do you still intend to mentor this? If this is indeed the way to resolve the startup perf hit I'm seeing in bug 1361095 comment 5, it's pretty important to resolve this for photon.
Flags: needinfo?(adw)
Sure.
Flags: needinfo?(adw)
Raj, sorry for not reviewing this.  Would you still like to work on it?
Flags: needinfo?(meghpararajkumar)
Raj, see comment #19. I'll take over if you don't respond within two weeks.
Flags: needinfo?(meghpararajkumar)
(In reply to Masatoshi Kimura [:emk] from comment #20)
> Raj, see comment #19. I'll take over if you don't respond within two weeks.

Raj's last bugzilla activity was in January, I don't think you need to wait more to take it over.

To remind how important this bug is, here is a startup profile where this blocks us for 1617ms: https://perfht.ml/2uCYbfB
OK, taking.
Assignee: meghpararajkumar → VYV03354
Flags: needinfo?(meghpararajkumar)
Attachment #8825048 - Attachment is obsolete: true
Attachment #8825048 - Flags: review?(adw)
Blocks: 1351070
Comment on attachment 8896331 [details]
Bug 886907 - Remove old synchronous contentPrefService from the tree.

https://reviewboard.mozilla.org/r/167592/#review173724

Thanks Masatoshi, looks good.
Attachment #8896331 - Flags: review?(adw) → review+
BTW there seems to be the idea in this bug that removing the old service alone will improve performance, but that's unlikely IMO, at least not significantly.  The perf problem is the main-thread storage/SQLite calls that the service makes on init, and this patch doesn't change that (rightly, since that should be done separately).
Pushed by VYV03354@nifty.ne.jp:
https://hg.mozilla.org/integration/autoland/rev/f92c249cae8b
Remove old synchronous contentPrefService from the tree. r=adw
Backed out for failing editor/libeditor/tests/test_bug1368544.html on Android 4.3 debug:

https://hg.mozilla.org/integration/autoland/rev/122fa30ec6605756d1221e91facd661e4b8069d7

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=f92c249cae8b855ff46035135785b73474bbc4ac&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=runnable&filter-resultStatus=usercancel
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=123248311&repo=autoland

[task 2017-08-15T12:30:21.407119Z] 12:30:21     INFO -  78 INFO None79 INFO TEST-START | editor/libeditor/tests/test_bug1368544.html
[task 2017-08-15T12:35:32.798392Z] 12:35:32     INFO -  Buffered messages logged at 12:30:16
[task 2017-08-15T12:35:32.798477Z] 12:35:32     INFO -  80 INFO must wait for load
[task 2017-08-15T12:35:32.798718Z] 12:35:32     INFO -  Buffered messages finished
[task 2017-08-15T12:35:32.799494Z] 12:35:32     INFO -  81 INFO TEST-UNEXPECTED-FAIL | editor/libeditor/tests/test_bug1368544.html | Test timed out.
[task 2017-08-15T12:35:32.800084Z] 12:35:32     INFO -      reportError@SimpleTest/TestRunner.js:121:7
[task 2017-08-15T12:35:32.800731Z] 12:35:32     INFO -      TestRunner._checkForHangs@SimpleTest/TestRunner.js:142:7
Flags: needinfo?(VYV03354)
I have no idea why it fails...
Assignee: VYV03354 → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(VYV03354)
Priority: P1 → P3
Ah, I got it. Mobile has its own package-manifest.in.
Assignee: nobody → VYV03354
Status: NEW → ASSIGNED
Priority: P3 → P1
Pushed by VYV03354@nifty.ne.jp:
https://hg.mozilla.org/integration/autoland/rev/832df1d6017e
Remove old synchronous contentPrefService from the tree. r=adw
https://hg.mozilla.org/mozilla-central/rev/832df1d6017e
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Iteration: --- → 57.2 - Aug 29
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/8e0c7bfb03ce
Port bug 886907 to TB/IB/SM: Package ContentPrefService2.js/manifest instead of nsContentPrefService.js/manifest. rs=bustage-fix
You need to log in before you can comment on or make changes to this bug.