1515094 - migrate notification data to kvstore

Status: RESOLVED FIXED

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

Description

[Myk Melez [:myk] [@mykmelez]]

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.

Comment 1

[Myk Melez [:myk] [@mykmelez]]

Attachment: minimally convert notificationstore to kvstore

Comment 2

[Myk Melez [:myk] [@mykmelez]]

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

Comment 3

[:Gijs (he/him)]

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

Comment 4

[Myk Melez [:myk] [@mykmelez]]

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

Comment 5

[BugBot [:suhaib / :marco/ :calixte]]

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?

Comment 6

[Myk Melez [:myk] [@mykmelez]]

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

Comment 7

[Oana Pop-Rus]

Backed out for failures in browser_all_files_referenced.js

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception%2Crunnable&revision=8b50a00d1d225ef5aa7eaefa7bc2f7ba0ed91c64

Backout: https://hg.mozilla.org/integration/autoland/rev/11ef93d1b795e14bdd1206d97786e9e967e1a7d8

Comment 8

[Myk Melez [:myk] [@mykmelez]]

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

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0c95586ed7d5
minimally convert notificationstore to kvstore r=asuth,lina

Comment 10

[Andrei Ciure[:aciure]]

Backed out changeset 0c95586ed7d5 (bug 1515094) for causing test_notification_get.html failures CLOSED TREE

push that caused the failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&selectedJob=235328305&revision=0c95586ed7d524b6cace5478288bba1f5caca2f6

failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&fromchange=d66a126be7b677ef148dd30f20371a212fece804&selectedJob=235384677&searchStr=android%2C4.3%2Capi16%2B%2Cdebug%2Cmochitests%2Ctest-android-em-4.3-arm7-api-16%2Fdebug-mochitest-22%2Cm%2822%29

backout: https://hg.mozilla.org/integration/autoland/rev/0692b5b93271aa26737b771c6069f4d8511af636

Comment 11

[Myk Melez [:myk] [@mykmelez]]

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.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9d4348674d26
minimally convert notificationstore to kvstore r=asuth,lina

Comment 13

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/9d4348674d26