1777973 - Disable and remove MOZ_NEW_XULSTORE on all channels

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Jens Stutte [:jstutte]]

Currently MOZ_NEW_XULSTORE is only enabled in nightly.

It has been like this for quite a while now and we should enable it everywhere.

Comment 1

[Jens Stutte [:jstutte]]

Hi Neil, is there any known problem that hinders us to just enable this for all? Having it in nightly only means to ship a largely untested configuration to release since May 2019. I'd consider this long-term split configuration to be a defect, actually.

Comment 2

[Neil Deakin]

I don't know, I'm going to redirect to mossop who might know more about this.

Comment 3

[Dave Townsend [:mossop]]

I've done a quick review of the old (JSON file) and new (rkv safe mode) stores and the old store is arguably safer. When writing data the old store writes to a temporary file before moving into place while the new store simply overwrites the existing file so if Firefox crashes or something while writing we likely end up with a corrupt file with the new store. Both of them behave the same when reading a corrupt file, the data is just discarded. XUL store is not particularly critical though, if we lose data then we just reset window sizes and positions.

The other important question is performance, Florian is it possible for you (or pass it along to someone else) to take a quick look at the performance characteristics of the old and new xulstore implementations. We read this on startup so it would be good to know if the new implementation performs better or not.

Comment 4

[Jens Stutte [:jstutte]]

if Firefox crashes or something while writing

I'd assume/hope that this would happen rare enough to not really matter? At least I am not aware of any problems with corrupted rkv files (but maybe we are just recovering well everywhere from corrupted data). If we really think this might become an issue, we could also improve rkv to make it safer for all. In general I'd assume that having less code that fiddles directly with files eases maintenance in the long run, too.

Comment 5

[Dave Townsend [:mossop]]

(In reply to Jens Stutte [:jstutte] from comment #4)

if Firefox crashes or something while writing

I'd assume/hope that this would happen rare enough to not really matter? At least I am not aware of any problems with corrupted rkv files (but maybe we are just recovering well everywhere from corrupted data). If we really think this might become an issue, we could also improve rkv to make it safer for all. In general I'd assume that having less code that fiddles directly with files eases maintenance in the long run, too.

Probably, though one of the reasons we moved xulstore off of rdf was that it kept going corrupt so these things can happen.

The question in front of us is whether to migrate all release users over to the new xulstore and the risk averse part of me wants to understand what we actually gain by doing that. The point of the new xulstore was to use the lmdb based rkv which would have allowed for incremental reads and writes so potentially improving performance. But we don't have that feature anymore and sounds like it is no longer on the table. Without that the new xulstore is quite a bit more complex than the old. Some of that complexity is inside rkv and so shared by all consumers but there is a bunch that isn't, I'll admit that LoC is not a fantastic metric but still the new xulstore is four times the size of the old and the bits of the old store that touch the file system are all in other shared well tested components like IOUtils.

Comment 6

[Jens Stutte [:jstutte]]

Sure, the most important thing here is to have the same for nightly and release, whatever you decide.

Comment 7

[Florian Quèze [:florian]]

(In reply to Dave Townsend [:mossop] from comment #3)

The other important question is performance, Florian is it possible for you (or pass it along to someone else) to take a quick look at the performance characteristics of the old and new xulstore implementations. We read this on startup so it would be good to know if the new implementation performs better or not.

It should be possible to capture startup profiles (just run the build with MOZ_PROFILER_STARTUP=1 in the environment) with the old and new store, and compare the I/O behaviors. I guess for the test to be relevant we would need to pre-populate the store with a typical amount of data?

Forwarding the needinfo to Doug who worked on startup disk I/O performance (but feel free to forward again to someone else in the perf team).

Comment 8

[Alex Thayer [:alexical] (she/her)]

I'm confused. It sounds like in both cases we read it all up front during startup? I assume rkv produces smaller files than just a json blob, so it should be somewhat better, though am I understanding correctly that these files are typically tiny?

So yeah, I just have the same concern that Mossop expressed: do we actually have any reason for thinking the new version is better? Otherwise I vote for disabling it on all channels and nuking the code.

Comment 9

[Jens Stutte [:jstutte]]

(In reply to Doug Thayer [:dthayer] (he/him) from comment #8)

So yeah, I just have the same concern that Mossop expressed: do we actually have any reason for thinking the new version is better? Otherwise I vote for disabling it on all channels and nuking the code.

I'd just want to repeat "In general I'd assume that having less code that fiddles directly with files eases maintenance in the long run, too." as possible argument. But in particular I have no informed opinion on the quality of the old implementation, so if it is good and not known to give us trouble - fine.

Comment 10

[Dave Townsend [:mossop]]

(In reply to Doug Thayer [:dthayer] (he/him) from comment #8)

I'm confused. It sounds like in both cases we read it all up front during startup? I assume rkv produces smaller files than just a json blob, so it should be somewhat better, though am I understanding correctly that these files are typically tiny?

Yes both read in full at startup though one reads from JS (via IOUtils) and the other from Rust through some layers of abstraction. They're pretty small so I'm guessing that there isn't much difference in raw I/O here and maybe that is all that matters.

So yeah, I just have the same concern that Mossop expressed: do we actually have any reason for thinking the new version is better? Otherwise I vote for disabling it on all channels and nuking the code.

The one clear downside to doing that is that while there is a migration path from old -> new stores there is no reverse path. So if we switch nightly back to the old store all nightly users lose their xul store (or it reverts to a very old version). Given how little is stored in there I don't think that is a blocker but would make me want to announce any such change somewhere in advance.

(In reply to Jens Stutte [:jstutte] from comment #9)

(In reply to Doug Thayer [:dthayer] (he/him) from comment #8)

So yeah, I just have the same concern that Mossop expressed: do we actually have any reason for thinking the new version is better? Otherwise I vote for disabling it on all channels and nuking the code.

I'd just want to repeat "In general I'd assume that having less code that fiddles directly with files eases maintenance in the long run, too." as possible argument. But in particular I have no informed opinion on the quality of the old implementation, so if it is good and not known to give us trouble - fine.

And I generally agree. But neither the new nor old stores fiddle directly with files. The old uses Cu.readUTF8File and IOUtils.writeJSON for all its file I/O.

Comment 11

[Dave Townsend [:mossop]]

I haven't heard any strong reasoning why the rkv based xulstore is any better than the old, simpler, json implementation so I think we should morph this bug to remove the new version. The first step of that is going to be me writing an intent to unship message to firefox-dev noting that this change will lost folks window positions the first time they start Nightly after the change. Assuming no issues are raised from that we'll turn off the build config flag then after a while remove the new version and and clean-up the code.

Comment 12

[Dave Townsend [:mossop]]

I've posted notification of this plan to firefox-dev: https://groups.google.com/a/mozilla.org/g/firefox-dev/c/zJturJWnMAY

Comment 13

[Dave Townsend [:mossop]]

We'll take care of disabling this in bug 1805401 and then do the final removal here assuming no issues arise.

Comment 14

[Magnus Melin [:mkmelin]]

Just to note, this will have more impact on Thunderbird, since we have much more windows and toolbar customization that rely in xulstore persistence.

Comment 15

[Magnus Melin [:mkmelin]]

Hmm, it wouldn't seem impossible to create code to - before disabling - at least dump the current state of the store into a json file, and then write migration code for attributes one cares about, so that everything isn't just reset.

Comment 16

[Magnus Melin [:mkmelin]]

Oh, if the new xulstore was never enabled on ESR, just ignore the above :)

Comment 17

[Dave Townsend [:mossop]]

(In reply to Magnus Melin [:mkmelin] from comment #15)

Hmm, it wouldn't seem impossible to create code to - before disabling - at least dump the current state of the store into a json file, and then write migration code for attributes one cares about, so that everything isn't just reset.
Oh, if the new xulstore was never enabled on ESR, just ignore the above :)

It's true writing the migration code is not strictly hard but you have to have the migration happening for a period in nightly before you disable and then rely on folks not skipping over that period. It's just not worth it for Firefox IMO and yes it was never enabled in ESR, this has been strictly nightly only.

Comment 18

[Dave Townsend [:mossop]]

Attachment: Bug 1777973: Remove MOZ_NEW_XULSTORE implementation. r=NeilDeakin!

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b83d4004d8cf
Remove MOZ_NEW_XULSTORE implementation. r=NeilDeakin

Comment 20

[Marian-Vasile Laza]

https://hg.mozilla.org/mozilla-central/rev/b83d4004d8cf