Closed Bug 1515094 Opened 9 months ago Closed 5 months ago

migrate notification data to kvstore

Categories

(Core :: DOM: Push Notifications, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla68
Tracking Status
firefox68 --- fixed

People

(Reporter: myk, Assigned: myk)

References

Details

Attachments

(1 file)

Notifications are currently stored in a JSON file notificationstore.json, using a custom storage implementation in NotificationDB.jsm.  To improve robustness and performance, we should migrate that datastore to the key-value storage engine that will be exposed by the kvstore.jsm module in bug 1490496.
Priority: -- → P3
PerfHerder results for Windows and Mac (Linux blocked by bug 1512541):

https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&originalRevision=1a7fb4c17553813a8b337afc1ec9e742fd2b3f21&newProject=try&newRevision=2f53ebacbf439e342c4074f099bafe9448b4e530&framework=1

Important changes:

* medium confidence regression of tpaint (2.62%) and tscrollx (5.60%) on windows10-64
* high confidence improvement of tresize on windows7-32 (-12.00%)

Other potentially-notable changes:

* low confidence regression of cpstartup on osx-10-10 (27.31%)
* low confidence improvement of "Noise Metric" on all tested platforms: osx-10-10 (-43.42%), windows10-64 (-23.50%), windows10-64-qr (-21.25%), and windows7-32 (-27.82%)
* low confidence improvement of tp5n main_normal_fileio on windows10-64 (-10.86%) and windows7-32 (-25.80%)
Depends on: 1512541
(In reply to Myk Melez [:myk] [@mykmelez] from comment #2)
> PerfHerder results for Windows and Mac (Linux blocked by bug 1512541):
> 
> https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-
> central&originalRevision=1a7fb4c17553813a8b337afc1ec9e742fd2b3f21&newProject=
> try&newRevision=2f53ebacbf439e342c4074f099bafe9448b4e530&framework=1

You need a baseline trypush. Don't compare to mozilla-central; the combination of pgo results etc. means that any comparison is going to be off. The tresize improvement makes no sense (it measures browser window resizes, which this change won't impact at all).

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

You need a baseline trypush. Don't compare to mozilla-central; the
combination of pgo results etc. means that any comparison is going to be
off. The tresize improvement makes no sense (it measures browser window
resizes, which this change won't impact at all).

Here's a comparison against a baseline pushed to tryserver with the same trychooser arguments:

https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=21361e80bec84be9c865a0058c3b7b84355e18c7&newProject=try&newRevision=a17cefba6ebf1a73468730d61b7ae8a7d07e1f85&framework=1&showOnlyComparable=1

It shows no important changes. I still don't have Linux results, however.

Depends on: 1522609
Depends on: 1522614
Depends on: rkv

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:myk, could you have a look please?

Flags: needinfo?(myk)

(In reply to Release mgmt bot [:sylvestre / :calixte] from comment #5)

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:myk, could you have a look please?

Yep, thanks for the reminder. I didn't land the patch earlier because I wanted to land it at the beginning of a cycle for risk mitigation reasons. I've now queued it for landing https://lando.services.mozilla.com/D13568/, and it'll do so once the tree reopens.

Flags: needinfo?(myk)

I've fixed the test failure, reopened the Phabricator revision (https://phabricator.services.mozilla.com/D13568), pushed the fix to the revision, and re-queued the revision for landing (https://lando.services.mozilla.com/D13568/).

Flags: needinfo?(myk)
Pushed by myk@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0c95586ed7d5
minimally convert notificationstore to kvstore r=asuth,lina

That link to the failures doesn't seem to work for me. But I think they're all like this one:

https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&revision=0c95586ed7d524b6cace5478288bba1f5caca2f6&selectedJob=235384677

And that one is bug 1531887. So we'll need to either fix that bug or work around it here before relanding this patch.

Depends on: 1531887
Flags: needinfo?(myk)
Depends on: 1542888
No longer depends on: 1531887
Depends on: 1543795
Pushed by myk@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9d4348674d26
minimally convert notificationstore to kvstore r=asuth,lina
Status: ASSIGNED → RESOLVED
Closed: 5 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68
You need to log in before you can comment on or make changes to this bug.