Closed
Bug 934967
Opened 11 years ago
Closed 7 years ago
[Session Restore] Read/write data with lz4
Categories
(Firefox :: Session Restore, defect)
Firefox
Session Restore
Tracking
()
RESOLVED
FIXED
Firefox 56
Tracking | Status | |
---|---|---|
firefox56 | --- | fixed |
People
(Reporter: Yoric, Assigned: beekill)
References
(Blocks 1 open bug)
Details
Attachments
(4 files)
Reading/writing data with lz4 should decrease the amount of time spent in I/O in session restore as well as battery usage, so this should be a nice win.
Updated•11 years ago
|
Flags: firefox-backlog?
Updated•11 years ago
|
Flags: firefox-backlog? → firefox-backlog+
Comment 1•10 years ago
|
||
It seems that this could be a problem with upgrades and downgrades. If somebody uses Nightly and then uses Release with the same profile, they will not be able to read their Session Store data. To support this, we would actually need to write out two versions of the data (compressed and uncompressed), until we feel that the migration period has lasted long enough to drop the uncompressed version.
Another question about this is if we need to worry about supporting the usecase of hand-editing these files. I don't think that this is something that we need to worry about, but it is worth bringing up.
Comment 2•10 years ago
|
||
We didn't really settle on a decision, yet. All the concerns brought up by you are totally valid and that's why we didn't really move forward here.
The hand-editing use case is probably nothing we should be too worried about. However, there are no tools like "unzip" or "tar" or anything that can read lz4 which makes it even worse should you once in a year want to take a look at its contents.
Comment 3•10 years ago
|
||
(In reply to Tim Taubert [:ttaubert] from comment #2)
> The hand-editing use case is probably nothing we should be too worried
> about. However, there are no tools like "unzip" or "tar" or anything that
> can read lz4 which makes it even worse should you once in a year want to
> take a look at its contents.
I suppose it wouldn't be hard to write an add-on that can do the extracting/editing/compressing through Firefox though.
Comment 4•10 years ago
|
||
That's true but it doesn't quite *feel* right :)
Updated•10 years ago
|
Whiteboard: p=5
Comment 5•10 years ago
|
||
(In reply to Tim Taubert [:ttaubert] from comment #2)
> The hand-editing use case is probably nothing we should be too worried
> about. However, there are no tools like "unzip" or "tar" or anything that
> can read lz4 which makes it even worse should you once in a year want to
> take a look at its contents.
I do e.g. see |lz4| executables linked here: https://code.google.com/p/lz4/
Comment 6•10 years ago
|
||
(In reply to Georg Fritzsche [:gfritzsche] from comment #5)
> I do e.g. see |lz4| executables linked here: https://code.google.com/p/lz4/
Also shipping with some Linux distros already apparently (either as lz4c or lz4).
Comment 7•10 years ago
|
||
(In reply to Georg Fritzsche [:gfritzsche] from comment #5)
> I do e.g. see |lz4| executables linked here: https://code.google.com/p/lz4/
I was under the impression that we're doing something nonstandard with our lz4, and this tool won't actually work with our files (I'm not sure we even have a command line tool which can handle our lz4).
Comment 8•10 years ago
|
||
(In reply to Steven MacLeod [:smacleod] from comment #7)
> (In reply to Georg Fritzsche [:gfritzsche] from comment #5)
> > I do e.g. see |lz4| executables linked here: https://code.google.com/p/lz4/
>
> I was under the impression that we're doing something nonstandard with our
> lz4, and this tool won't actually work with our files (I'm not sure we even
> have a command line tool which can handle our lz4).
Yoric, do you know about this?
Flags: needinfo?(dteller)
Reporter | ||
Comment 9•10 years ago
|
||
Yes, for historical reasons, we use the lz4 algorithm but with our own wrapper. There are plans to upgrade to something more standard, once we have adopted the updated upstream library to a more recent version.
Flags: needinfo?(dteller)
Updated•10 years ago
|
Points: --- → 5
Whiteboard: p=5
Comment 10•8 years ago
|
||
Bug 1304389 is newer, but has a patch from bkelly.
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → DUPLICATE
Updated•7 years ago
|
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Updated•7 years ago
|
Assignee: nobody → nnn_bikiu0707
Status: NEW → ASSIGNED
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 14•7 years ago
|
||
I tested the patch locally, there were some errors. But I want know whether I'm going the right track.
Comment 15•7 years ago
|
||
mozreview-review |
Comment on attachment 8872878 [details]
Bug 934967 - Part 1: Compress session store files using lz4.
https://reviewboard.mozilla.org/r/144388/#review148308
The SessionWorker changes are looking good to me. The SessionFile changes need some more work, I think ;-)
I've written down a few ideas below.
::: browser/components/sessionstore/SessionFile.jsm:101
(Diff revision 1)
>
> var SessionFileInternal = {
> Paths: Object.freeze({
> // The path to the latest version of sessionstore written during a clean
> // shutdown. After startup, it is renamed `cleanBackup`.
> - clean: Path.join(profileDir, "sessionstore.js"),
> + clean: Path.join(profileDir, "sessionstore.jslz4"),
While we're here, let's standardize on '.jsonlz4' to conform to similar feature usage elsewhere.
::: browser/components/sessionstore/SessionFile.jsm:218
(Diff revision 1)
>
> let result;
> let noFilesFound = true;
> + let useOldExtension = false;
> // Attempt to load by order of priority from the various backups
> + do {
Instead of using a loop here, I'd recommend abstracting away the body of the for...of loop below into a separate method that can be called with a path and returns `result`. If `result` is null, you can call it again with the path containing the old extension.
::: browser/components/sessionstore/SessionFile.jsm:229
(Diff revision 1)
> - let startMs = Date.now();
> + let startMs = Date.now();
>
> - let source = await OS.File.read(path, { encoding: "utf-8" });
> - let parsed = JSON.parse(source);
> + let source;
> + let parsed;
> + if (useOldExtension) {
> + path = this.Paths[key].replace("lz4", "");
So this'd become `.replace("jsonlz4", "js")`
::: browser/components/sessionstore/SessionFile.jsm:246
(Diff revision 1)
> - }
> + }
> - result = {
> + result = {
> - origin: key,
> + origin: key,
> - source,
> + source,
> - parsed
> + parsed,
> + useOldExtension: useOldExtension
As you can see used above, a new JS feature is that you can replace `useOldExtension: useOldExtension` with just `useOldExtension` if the name of the key and variable that holds the key's value are the same.
::: browser/components/sessionstore/SessionFile.jsm:254
(Diff revision 1)
> <span class="indent">>></span> add(false);
> <span class="indent">>></span> Telemetry.getHistogramById("FX_SESSION_RESTORE_READ_FILE_MS").
> <span class="indent">>></span> add(Date.now() - startMs);
> <span class="indent">>></span> break;
> <span class="indent">>></span> } catch (ex) {
> <span class="indent">>></span> if (ex instanceof OS.File.Error && ex.becauseNoSuchFile) {
We also need to take care not to pollute our telemetry data with a huge amount of file not found errors when we migrate. Perhaps it's an idea to put the `useOldExtension` retry logic in here?
::: browser/components/sessionstore/SessionWorker.js:314
(Diff revision 1)
> let exn = null;
>
> // Erase main session state file
> try {
> File.remove(this.Paths.clean);
> + File.remove(this.Paths.clean.replace("lz4", ""), {ignoreAbsent: true}); // remove old extension one
nit: Please put comments on their own line and format them correctly: "Remove old extension ones."
Attachment #8872878 -
Flags: review?(mdeboer)
Comment 16•7 years ago
|
||
mozreview-review |
Comment on attachment 8872879 [details]
Bug 934967 - Part 2: Make test cases read lz4 compressed version of session store files and create a frequently use function in head.js.
https://reviewboard.mozilla.org/r/144390/#review148318
Looks good! However, I would keep the sessionstore_invalid.json and sessionstore_valid.json files and compress them inside the test instead - that way we're keeping things readable and make sure that the tests can be changed easily when necessary.
Attachment #8872879 -
Flags: review?(mdeboer)
Comment 17•7 years ago
|
||
mozreview-review |
Comment on attachment 8872880 [details]
Bug 934967 - Part 3: Add test cases to check session store's migration to using lz4 compression.
https://reviewboard.mozilla.org/r/144392/#review148320
::: browser/components/sessionstore/test/unit/xpcshell.ini:10
(Diff revision 1)
> support-files =
> data/sessionCheckpoints_all.json
> data/sessionstore_invalid.js
> data/sessionstore_valid.js
> + data/sessionstore_invalid.jslz4
> + data/sessionstore_valid.jslz4
This belongs in the previous patch.
::: browser/components/sessionstore/test/unit/xpcshell.ini:18
(Diff revision 1)
> [test_histogram_corrupt_files.js]
> [test_shutdown_cleanup.js]
> [test_startup_nosession_async.js]
> [test_startup_session_async.js]
> [test_startup_invalid_session.js]
> +[test_migration.js]
Please use a more descriptive filename, like 'test_migration_lz4compression.js' for example.
Comment 18•7 years ago
|
||
mozreview-review |
Comment on attachment 8872880 [details]
Bug 934967 - Part 3: Add test cases to check session store's migration to using lz4 compression.
https://reviewboard.mozilla.org/r/144392/#review148322
Published too soon!
I think this is already looking really good and ready to review when you push your updates next time.
Attachment #8872880 -
Flags: review?(mdeboer)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 22•7 years ago
|
||
mozreview-review |
Comment on attachment 8872878 [details]
Bug 934967 - Part 1: Compress session store files using lz4.
https://reviewboard.mozilla.org/r/144388/#review150164
Almost there! Next version will most probably be an r+, so great work so far!
Please don't forget to mark open issues here in ReviewBoard as 'fixed' when you in fact fixed them. That helps your reviewer quite a bit as well. Thanks!
::: browser/components/sessionstore/SessionFile.jsm:210
(Diff revision 2)
> } catch (ex) {
> return undefined;
> }
> },
>
> - // Find the correct session file, read it and setup the worker.
> + async loadSessionFile(useOldExtension) {
nit: please rename this to `_readInternal`
::: browser/components/sessionstore/SessionFile.jsm:235
(Diff revision 2)
> + source = await OS.File.read(path, {encoding: "utf-8"});
> + } else {
> + path = this.Paths[key];
> + source = await OS.File.read(path, {encoding: "utf-8", compression: "lz4"});
> + }
> + parsed = JSON.parse(source);
1. Please leave the `let parsed` (variable declaration) here.
2. Please keep the `source = ` line above here as well:
```js
let source = await OS.File.read(path, {encoding: "utf-8", compression: useOldExtension ? null : "lz4"});
let parsed = JSON.parse(source);
```
::: browser/components/sessionstore/SessionFile.jsm:269
(Diff revision 2)
> corrupted = true;
> }
> } finally {
> if (exists) {
> noFilesFound = false;
> + if (!useOldExtension) {
Please revert this change.
::: browser/components/sessionstore/SessionWorker.js:83
(Diff revision 2)
> * we have started by loading the corresponding file.
> */
> state: null,
>
> /**
> + * A flag indicate we loaded a session file with ext .js
nit: `A flag that indicates that we loaded a session file with the derprecated .js extension.`
::: browser/components/sessionstore/SessionWorker.js:178
(Diff revision 2)
> // Move $Path.clean out of the way, to avoid any ambiguity as
> // to which file is more recent.
> + if (!this.useOldExtension) {
> - File.move(this.Paths.clean, this.Paths.cleanBackup);
> + File.move(this.Paths.clean, this.Paths.cleanBackup);
> + } else {
> + let oldCleanPath = this.Paths.clean.replace("jsonlz4", "js");
nit: please add a comment here that explains what this code is doing and why it's there.
::: browser/components/sessionstore/SessionWorker.js:220
(Diff revision 2)
> + compression: "lz4"
> });
> }
>
> telemetry.FX_SESSION_RESTORE_WRITE_FILE_MS = Date.now() - startWriteMs;
> telemetry.FX_SESSION_RESTORE_FILE_SIZE_BYTES = data.byteLength;
This will now report incorrect numbers! Could you change this to record the bytes that were actually written to disk?
This number is returned by `File.writeAtomic()`, so there's actually quite little you need to change to correct it.
::: browser/components/sessionstore/SessionWorker.js:313
(Diff revision 2)
> let exn = null;
>
> // Erase main session state file
> try {
> File.remove(this.Paths.clean);
> + // Remove old extension ones
nit: missing dot.
Attachment #8872878 -
Flags: review?(mdeboer) → review-
Comment 23•7 years ago
|
||
mozreview-review |
Comment on attachment 8872879 [details]
Bug 934967 - Part 2: Make test cases read lz4 compressed version of session store files and create a frequently use function in head.js.
https://reviewboard.mozilla.org/r/144390/#review150180
Also super close! Please don't forget to resolve the ReviewBoard issues when you push your next version. Thanks!
::: commit-message-f585e:1
(Diff revision 2)
> +Bug 934967 - Part 2: Change session store files extension from .js to .jsonlz4. r?mikedeboer
This is not describing what this patch changes properly.
Please add proper, descriptive commit messages. If it can't fit on a single line, feel free to write a longer message below it, separated from the first line by a newline.
::: browser/components/sessionstore/test/unit/test_backup_once.js:29
(Diff revision 2)
> run_next_test();
> }
>
> +// Compress the source file using lz4 and put the result to destination file
> +// After that, source file is deleted
> +function promise_compress(source, destination) {
You can simplify this as:
```js
async function writeCompressedFile(source, destination) {
let data = await OS.File.read(source);
await OS.File.writeAtomic(destination, data, {compression: "lz4"});
await OS.File.remove(source);
}
```
::: browser/components/sessionstore/test/unit/test_shutdown_cleanup.js:34
(Diff revision 2)
> platformVersion: "",
> });
>
> add_task(async function setup() {
> - let source = do_get_file("data/sessionstore_valid.js");
> - source.copyTo(profd, "sessionstore.js");
> + let source = do_get_file("data/sessionstore_valid.jslz4");
> + source.copyTo(profd, "sessionstore.jslz4");
"jslz4"? Is there something wrong in this test?
I can why it _might_ work - it's because you revert to the .js version if it can't find the compressed one.
So please do the same as in 'test_backup_once.js' and put the shared function in 'browser/components/sessionstore/test/unit/head.js'.
::: browser/components/sessionstore/test/unit/test_startup_invalid_session.js:19
(Diff revision 2)
>
> - do_test_pending();
> + // Compress sessionstore.js to sessionstore.jsonlz4
> + // and remove sessionstore.js
> + let oldExtSessionFile = SessionFile.Paths.clean.replace("jsonlz4", "js");
> + let readPromise = OS.File.read(oldExtSessionFile);
> + readPromise.then(function onSucess(content) {
Please make the shared function you will put in head.js usable here too.
::: browser/components/sessionstore/test/unit/test_startup_session_async.js:25
(Diff revision 2)
>
> - do_test_pending();
> + // Compress sessionstore.js to sessionstore.jsonlz4
> + // and remove sessionstore.js
> + let oldExtSessionFile = SessionFile.Paths.clean.replace("jsonlz4", "js");
> + let readPromise = OS.File.read(oldExtSessionFile);
> + readPromise.then(function onSucess(content) {
Same here! DRY is a good thing here I think.
Attachment #8872879 -
Flags: review?(mdeboer) → review-
Comment 24•7 years ago
|
||
mozreview-review |
Comment on attachment 8872880 [details]
Bug 934967 - Part 3: Add test cases to check session store's migration to using lz4 compression.
https://reviewboard.mozilla.org/r/144392/#review150190
On the existing code I only have a few small comments. I'd like to see two more tests:
1. one that simulates reading the sessionfile(s) with the lz4 ones in the profile already (in other words: like a startup with already converted files in the profile dir)
2. and one that simulates an empty profile dir and making sure that empty, compressed session files are written to disk. (and `read()` doesn't fail).
::: browser/components/sessionstore/test/unit/test_migration_lz4compression.js:1
(Diff revision 2)
> +/* Any copyright is dedicated to the Public Domain.
The license block is not necessary anymore for test files, so you can remove it here.
::: browser/components/sessionstore/test/unit/test_migration_lz4compression.js:4
(Diff revision 2)
> +/* Any copyright is dedicated to the Public Domain.
> + http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +/* Copied from test_backup_once.js */
This comment is not necessary. But good that you started from an existing test!
::: browser/components/sessionstore/test/unit/test_migration_lz4compression.js:10
(Diff revision 2)
> +
> +"use strict";
> +
> +var {OS} = Cu.import("resource://gre/modules/osfile.jsm", {});
> +var {XPCOMUtils} = Cu.import("resource://gre/modules/XPCOMUtils.jsm", {});
> +var {SessionWorker} = Cu.import("resource:///modules/sessionstore/SessionWorker.jsm", {});
s/var/const/ for variables that are unchanged throughout the scope of this file.
::: browser/components/sessionstore/test/unit/test_migration_lz4compression.js:12
(Diff revision 2)
> +
> +var {OS} = Cu.import("resource://gre/modules/osfile.jsm", {});
> +var {XPCOMUtils} = Cu.import("resource://gre/modules/XPCOMUtils.jsm", {});
> +var {SessionWorker} = Cu.import("resource:///modules/sessionstore/SessionWorker.jsm", {});
> +
> +var File = OS.File;
This is not really necessary, right? Just use OS.File everywhere.
::: browser/components/sessionstore/test/unit/test_migration_lz4compression.js:46
(Diff revision 2)
> + let actual = await OS.File.read(path, { encoding: "utf-8", compression: "lz4" });
> + Assert.deepEqual(JSON.parse(actual), expect, `File ${path} contains the expected data.`);
> + })();
> +}
> +
> +// check whether the migration from .js to .jslz4 is correct
nit: Please start comment sentences with a capital and end with a dot.
::: browser/components/sessionstore/test/unit/test_migration_lz4compression.js:71
(Diff revision 2)
> + Assert.deepEqual(result.parsed, parsed, "result.parse contains expected data");
> +
> + // initiate a write to ensure we write the compressed version
> + await SessionFile.write(parsed);
> + await promise_check_exist(Paths.backups, true);
> + await promise_check_exist(Paths.clean, false);
Why doesn't this file exist?
Attachment #8872880 -
Flags: review?(mdeboer) → review-
Assignee | ||
Comment 25•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8872880 [details]
Bug 934967 - Part 3: Add test cases to check session store's migration to using lz4 compression.
https://reviewboard.mozilla.org/r/144392/#review150190
> Why doesn't this file exist?
We performed a SessionFile.write, this write is not final write, so in [1] the Path.clean will be move to Path.cleanBackup
[1]: http://searchfox.org/mozilla-central/source/browser/components/sessionstore/SessionWorker.js#166-167
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 29•7 years ago
|
||
mozreview-review |
Comment on attachment 8872878 [details]
Bug 934967 - Part 1: Compress session store files using lz4.
https://reviewboard.mozilla.org/r/144388/#review151284
Cool! This is already looking very good. I'd like to see the final changes one more time. This also gives David a chance to look at this.
::: browser/components/sessionstore/SessionFile.jsm:233
(Diff revision 3)
> + }
> + source = await OS.File.read(path, {encoding: "utf-8"});
> + } else {
> + path = this.Paths[key];
> + source = await OS.File.read(path, {encoding: "utf-8", compression: "lz4"});
> + }
You forgot to make the changes here:
```js
let options = { encoding: "utf-8" };
if (useOldExtension) {
//...
} else {
path = this.Paths[key];
options.compression = "lz4";
}
let source = await OS.File.read(path, options);
let parsed = JSON.parse(source);
```
::: browser/components/sessionstore/SessionWorker.js:197
(Diff revision 3)
> // We are shutting down. At this stage, we know that
> // $Paths.clean is either absent or corrupted. If it was
> // originally present and valid, it has been moved to
> // $Paths.cleanBackup a long time ago. We can therefore write
> // with the guarantees that we erase no important data.
> - File.writeAtomic(this.Paths.clean, data, {
> + bytesWritten = File.writeAtomic(this.Paths.clean, data, {
Are you 300% sure that this API doesn't simply return a Promise because it's async? I don't know, because this is inside a worker and my gut tells me 'no', but flagging David to be sure...
::: browser/components/sessionstore/SessionWorker.js:224
(Diff revision 3)
> + compression: "lz4"
> });
> }
>
> telemetry.FX_SESSION_RESTORE_WRITE_FILE_MS = Date.now() - startWriteMs;
> - telemetry.FX_SESSION_RESTORE_FILE_SIZE_BYTES = data.byteLength;
> + telemetry.FX_SESSION_RESTORE_FILE_SIZE_BYTES = bytesWritten;
When this lands, I'll flag Harald to add this telemetry metric to the Quantum Flow dashboard :-)
Attachment #8872878 -
Flags: review?(mdeboer) → review-
Comment 30•7 years ago
|
||
mozreview-review |
Comment on attachment 8872879 [details]
Bug 934967 - Part 2: Make test cases read lz4 compressed version of session store files and create a frequently use function in head.js.
https://reviewboard.mozilla.org/r/144390/#review151288
Looks good to me!
::: commit-message-f585e:1
(Diff revision 3)
> +Bug 934967 - Part 2: Make test cases read lz4 compressed version of session store files and
Please keep this on a single line
Attachment #8872879 -
Flags: review?(mdeboer) → review+
Comment 31•7 years ago
|
||
mozreview-review |
Comment on attachment 8872880 [details]
Bug 934967 - Part 3: Add test cases to check session store's migration to using lz4 compression.
https://reviewboard.mozilla.org/r/144392/#review151292
Yay! Thanks for the added tests - very re-assuring.
Attachment #8872880 -
Flags: review?(mdeboer) → review+
Updated•7 years ago
|
Attachment #8872878 -
Flags: feedback?(dteller)
Assignee | ||
Comment 32•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8872878 [details]
Bug 934967 - Part 1: Compress session store files using lz4.
https://reviewboard.mozilla.org/r/144388/#review151284
> Are you 300% sure that this API doesn't simply return a Promise because it's async? I don't know, because this is inside a worker and my gut tells me 'no', but flagging David to be sure...
It's my mistake. The File.writeAtomic doesn't return anything [1], so we can't just retrieve the bytes written here. Therefore, I changed to using File.stat instead [2].
[1]: https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/OSFile.jsm/OS.File_for_workers#OS.File.writeAtomic()
[2]: https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/OSFile.jsm/OS.File_for_workers#OS.File.stat()
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 36•7 years ago
|
||
mozreview-review |
Comment on attachment 8872878 [details]
Bug 934967 - Part 1: Compress session store files using lz4.
https://reviewboard.mozilla.org/r/144388/#review152828
LGTM! It looks like you accidentally cleared the feedback flag for David, but I'd still like to have him look at this patch, just in case you or I missed something. Re-adding.
Good work so far! :)
Attachment #8872878 -
Flags: review?(mdeboer) → review+
Updated•7 years ago
|
Attachment #8872878 -
Flags: feedback?(dteller)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 39•7 years ago
|
||
I fixed some errors on previous try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=11cfb78d16e810b4637cad4688bd45b738677b32
This is new try result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=807d8e666a6773fcbeb3bafd0f080b998f6c4841
Reporter | ||
Comment 40•7 years ago
|
||
mozreview-review |
Comment on attachment 8872878 [details]
Bug 934967 - Part 1: Compress session store files using lz4.
https://reviewboard.mozilla.org/r/144388/#review153888
The patch looks mostly good. However, there is one subtle safety problem that needs to be addressed, as well as an open question.
Also, a few nitpicks.
::: browser/components/sessionstore/SessionFile.jsm:224
(Diff revision 4)
> - let path = this.Paths[key];
> + let path;
> let startMs = Date.now();
>
> - let source = await OS.File.read(path, { encoding: "utf-8" });
> + let options = {encoding: "utf-8"};
> + if (useOldExtension) {
> + if (key === "recoveryBackup") {
Nit: I'd rather have
```js
this.Paths[key]
.replace("jsonlz4", "js")
.replace("baklz4", "bak");
```
than the hardcoded key.
::: browser/components/sessionstore/SessionFile.jsm:245
(Diff revision 4)
> }
> result = {
> origin: key,
> source,
> - parsed
> + parsed,
> + useOldExtension
Side-note: my intuition (for a followup bug?) is that we would be interested in knowing how many uses of `useOldExtension` succeed.
::: browser/components/sessionstore/SessionFile.jsm:280
(Diff revision 4)
> +
> + // Find the correct session file, read it and setup the worker.
> + async read() {
> + this._initializationStarted = true;
> +
> + let {result, noFilesFound} = await this._readInternal(false);
Nit: It would be nice to have a comment explaining why we try once with `false` and once with `true`.
::: browser/components/sessionstore/SessionWorker.js:183
(Diff revision 4)
> + // Since we are migrating from .js to .jsonlz4,
> + // we need to compress the deprecated $Path.clean
> + // and move it to $Path.cleanBackup.
> + let oldCleanPath = this.Paths.clean.replace("jsonlz4", "js");
> + let data = File.read(oldCleanPath);
> + File.remove(oldCleanPath);
**Safety warning** We should remove *after* `writeAtomic` has succeeded. I'd suggest doing this in a `setTimeout`, at least 20 seconds after the success of `writeAtomic`, to avoid losing the data in case of a power outage just at that moment.
Mmmhhhh... come to think about it, I believe that we should **not remove the file at all** – if there is a bug, users will be pretty **** off if we can't recover something somewhat useful. So, I would add the cleanup as a separate step, in another version of Firefox, rather than removing this immediately upon startup. What do you think?
Comment 41•7 years ago
|
||
(In reply to David Teller [:Yoric] (please use "needinfo") from comment #40)
> **Safety warning** We should remove *after* `writeAtomic` has succeeded. I'd
> suggest doing this in a `setTimeout`, at least 20 seconds after the success
> of `writeAtomic`, to avoid losing the data in case of a power outage just at
> that moment.
>
> Mmmhhhh... come to think about it, I believe that we should **not remove the
> file at all** – if there is a bug, users will be pretty pissed off if we
> can't recover something somewhat useful. So, I would add the cleanup as a
> separate step, in another version of Firefox, rather than removing this
> immediately upon startup. What do you think?
Thanks for taking a look David!!
I totally agree! Let's not remove the old file at all.
Assignee | ||
Comment 42•7 years ago
|
||
mozreview-review |
Comment on attachment 8872878 [details]
Bug 934967 - Part 1: Compress session store files using lz4.
https://reviewboard.mozilla.org/r/144388/#review151182
::: browser/components/sessionstore/SessionFile.jsm:235
(Diff revision 2)
> + source = await OS.File.read(path, {encoding: "utf-8"});
> + } else {
> + path = this.Paths[key];
> + source = await OS.File.read(path, {encoding: "utf-8", compression: "lz4"});
> + }
> + parsed = JSON.parse(source);
There's probably something wrong with compression : useOldExtension ? null : "lz4". That expression makes some tests failed so I'll keep the old one.
Assignee | ||
Comment 43•7 years ago
|
||
(In reply to Bao Quan [:beekill] from comment #42)
> Comment on attachment 8872878 [details]
> Bug 934967 - Part 1: Compress session store files using lz4.
>
> https://reviewboard.mozilla.org/r/144388/#review151182
>
> ::: browser/components/sessionstore/SessionFile.jsm:235
> (Diff revision 2)
> > + source = await OS.File.read(path, {encoding: "utf-8"});
> > + } else {
> > + path = this.Paths[key];
> > + source = await OS.File.read(path, {encoding: "utf-8", compression: "lz4"});
> > + }
> > + parsed = JSON.parse(source);
>
> There's probably something wrong with compression : useOldExtension ? null :
> "lz4". That expression makes some tests failed so I'll keep the old one.
Oops. My mistake, this should have been posted long ago.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 47•7 years ago
|
||
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 52•7 years ago
|
||
mozreview-review |
Comment on attachment 8879465 [details]
Bug 934967 - Part 4: Change session file extension in FirefoxProfileMigrator to jsonlz4 and make SessionMigration read and write functions use lz4 compression.
https://reviewboard.mozilla.org/r/150770/#review156218
Attachment #8879465 -
Flags: review?(mdeboer) → review+
Assignee | ||
Comment 53•7 years ago
|
||
Try server result looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=15d50d9943b1. Mike, can you add check-in flag?
Flags: needinfo?(mdeboer)
Comment 54•7 years ago
|
||
Pushed by mdeboer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/733cfc278628
Part 1: Compress session store files using lz4. r=mikedeboer
https://hg.mozilla.org/integration/autoland/rev/7de7f8f5903a
Part 2: Make test cases read lz4 compressed version of session store files and create a frequently use function in head.js. r=mikedeboer
https://hg.mozilla.org/integration/autoland/rev/77d479c2426c
Part 3: Add test cases to check session store's migration to using lz4 compression. r=mikedeboer
https://hg.mozilla.org/integration/autoland/rev/5e40f7e49396
Part 4: Change session file extension in FirefoxProfileMigrator to jsonlz4 and make SessionMigration read and write functions use lz4 compression. r=mikedeboer
Updated•7 years ago
|
Flags: needinfo?(mdeboer)
Comment 55•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/733cfc278628
https://hg.mozilla.org/mozilla-central/rev/7de7f8f5903a
https://hg.mozilla.org/mozilla-central/rev/77d479c2426c
https://hg.mozilla.org/mozilla-central/rev/5e40f7e49396
Status: ASSIGNED → RESOLVED
Closed: 8 years ago → 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
Comment 56•7 years ago
|
||
some users have reported lost sessions, this change may be responsible.
Comment 57•7 years ago
|
||
I think this bug should be made dependent on :
Bug 1376983 - Firefox 54.0 doesn't restore session of Nightly 2017-06-28, shows the Firefox start page
See comment #5 and comment #6 for a suggested plan to make the session restore backward/forward compatible.
You need to log in
before you can comment on or make changes to this bug.
Description
•