Closed Bug 1513892 Opened 11 months ago Closed 11 months ago

LSNG: Optimize origin initialization

Categories

(Core :: Storage: localStorage & sessionStorage, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla66
Tracking Status
firefox66 --- fixed

People

(Reporter: janv, Assigned: janv)

References

Details

Attachments

(5 files, 5 obsolete files)

Right now, we need to query data.sqlite in QuotaClient::InitOrigin to get usage.
When I artificially disable the query and just return a hard coded value, I see approximately 25% reduction in temp storage initialization time (I have about 500 origin directories and 100 data.sqlite files in my profile).

One option is to keep usage in a separate file that would be used to get usage instead of querying the database. We have something similar for the paddings in DOM cache implementation.

Another option is to keep all usages in one common database, use a cross database transaction to keep data consistent and load everything at once before origins are initialized.
Blocks: 1513881
Temp storage initialization time can be checked in about:telemetry (QM_REPOSITORIES_INITIALIZATION_TIME).
Another option is to just cache the usage in the "database" table, so the query for InitOrigin would be much cheaper.

I'll see tomorrow, maybe we want this before enabling on nightly.
(In reply to Jan Varga [:janv] from comment #2)
> Another option is to just cache the usage in the "database" table, so the
> query for InitOrigin would be much cheaper.

No, just opening of the primary database is still noticeably slower than opening and reading from a separate binary file for usage.
Priority: -- → P3
So I have a patch that adds a "usage" column to the "database" table. I think it would be better to land it before we flip the pref since it involves sqlite schema changes.
Temp storage initialization is a bit faster with the additional column and I think the column is needed for the standalone "usage" file anyway.

I also found 2 issues with the current usage calculation:
1. "select sum(length(key) + length(value)) from data" returns null if there are no rows in the "data" table
This may lead to issues like bug 1505798.
The solution is to use total() instead of sum().

2. I found a site that stores weird content in LS, it contains a lot of stuff like "\0003", etc.
The problem is that length(value) in SQL and value.Length() in C++ return different results!
So I came to a conclusion that we should not rely on length() provided by sqlite.
Attached patch patch (obsolete) — Splinter Review
Assignee: nobody → jvarga
Status: NEW → ASSIGNED
Attachment #9031595 - Flags: review?(bugmail)
Blocks: 1510410
No longer blocks: 1513881
(In reply to Jan Varga [:janv] from comment #0)
> One option is to keep usage in a separate file that would be used to get
> usage instead of querying the database. We have something similar for the
> paddings in DOM cache implementation.

I have a patch for this, it uses two files .usage and .usage-journal.

So, the "usage" column in the "database" table + ".usage" file lowered my temp
storage initialization time to 63% of the original value, which is quite nice win.
I have over 200 origin directories with LS data in my profile.

There's one more thing I need to do: write a new xpcshell test to check
that .usage/.usage-journal files are correctly handled.
Attached patch part 1 (obsolete) — Splinter Review
Attachment #9031595 - Attachment is obsolete: true
Attachment #9031595 - Flags: review?(bugmail)
Attachment #9031832 - Flags: review?(bugmail)
Attachment #9031832 - Attachment is patch: true
Attachment #9031832 - Attachment mime type: application/octet-stream → text/plain
Comment on attachment 9031832 [details] [diff] [review]
part 1

Review of attachment 9031832 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/localstorage/ActorsParent.cpp
@@ +3819,5 @@
> +        gUsages->Put(origin, usage);
> +      });
> +
> +  MOZ_ALWAYS_SUCCEEDS(
> +      quotaManager->IOThread()->Dispatch(runnable, NS_DISPATCH_NORMAL));

Cached usage is now updated when data changes have been flushed to disk, so it now doesn't contain the pre-allocated quota usage.
Attached patch part 2 (obsolete) — Splinter Review
I'm still working on a test, submitting C++ changes for now.
Attachment #9031833 - Flags: review?(bugmail)
Comment on attachment 9031833 [details] [diff] [review]
part 2

Review of attachment 9031833 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/localstorage/ActorsParent.cpp
@@ +137,5 @@
> +
> +#define USAGE_JOURNAL_FILE_NAME ".usage-journal"
> +
> +static const uint32_t kUsageFileSize = 12;
> +static const uint32_t kUsageFileCookie = 0x420a420a;

I borrowed the cookie approach from here:
https://searchfox.org/mozilla-central/rev/1a4a7745fd4a14402a0de0e9abc040724d4fcf8f/dom/asmjscache/AsmJSCache.cpp#1552

The cookie + the empty journal file should provide really good consistency.

I'm also happy, that I didn't have to add another mutex. InitOrigin can only run when there are no Datastores (and no Connections). GetUsageForOrigin uses a cache so it never touches data.sqlite.

@@ -6965,5 @@
>    if (NS_WARN_IF(NS_FAILED(rv))) {
>      return rv;
>    }
>  
> -  rv = file->Exists(&exists);

I found out that Exists doesn't use the stat system call, so it's better to avoid it.

@@ +7285,5 @@
> +  }
> +
> +  // Report unknown files in debug builds, but don't fail, just warn.
> +
> +#ifdef DEBUG

I think we don't need this in opt builds, we only care about one specific database, it's not like IDB when each .sqlite file can potentially be an IDB database.
Comment on attachment 9031833 [details] [diff] [review]
part 2

Review of attachment 9031833 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good!

I do think we should be caching the origin's usage in QuotaManager in storage.sqlite, but I think we want to make sure we get that right and I agree it's easier and cleaner to just do this here for now for many reasons.

I'm going to r- this for now just because I'd like to double-check the changes to the journal file and make sure we're not gratuitously fsync-ing.

::: dom/localstorage/ActorsParent.cpp
@@ +132,5 @@
>   * The journal corresponding to DATA_FILE_NAME.  (We don't use WAL mode.)
>   */
>  #define JOURNAL_FILE_NAME "data.sqlite-journal"
>  
> +#define USAGE_FILE_NAME ".usage"

I don't think we want to name any more of our files with a dot prefix now that we're adding heuristics that potentially ignore user-created dot-files in QuotaManager.  I suggest just calling this "usage".  (Also, the only reason to name something starting with a dot is to hide it from users on POSIXy platforms, but in theory we're assuming people won't go poking around in these directories, so there's no benefit to hiding things from them.)

Please also add a comment like:

This file contains the current usage of the LocalStorage database as defined by the byte-length totals of all keys and values for the database, which differs from the actual size on disk.  We store this value in a separate file as a cache so that we can initialize the QuotaClient faster.  In the future, this file will be eliminated and the information will be stored in PROFILE/storage.sqlite or similar QuotaManager-wide storage.

The file contains a binary verification cookie (32-bits) followed by the actual usage (64-bits).

@@ +134,5 @@
>  #define JOURNAL_FILE_NAME "data.sqlite-journal"
>  
> +#define USAGE_FILE_NAME ".usage"
> +
> +#define USAGE_JOURNAL_FILE_NAME ".usage-journal"

This should also be renamed.

I'd suggest adding a comment here too, like:

Following a QuotaManager idiom, this journal file's existence is a marker that the usage file was in the process of being updated and is currently invalid.  This file is created prior to updating the usage file and only deleted after the usage file has been written and closed and any pending database transactions have been committed.  Note that this idiom is expected to work if Gecko crashes in the middle of a write, but is not expected to be foolproof in the face of a system crash, as we do not explicitly attempt to fsync the directory containing the journal file.

If the journal file is found to exist at origin initialization time, the usage will be re-computed from the current state of DATA_FILE_NAME.

@@ +137,5 @@
> +
> +#define USAGE_JOURNAL_FILE_NAME ".usage-journal"
> +
> +static const uint32_t kUsageFileSize = 12;
> +static const uint32_t kUsageFileCookie = 0x420a420a;

As an aside, the cookie used by AsmJSCache assumes a simplified model of how writes will land on disk and is probably not safe in the face of a system crash.

For the usage file, since it's less than a disk sector, it's pretty likely that our contents will hit disk atomically.  If you wanted to be more paranoid, it would probably make sense for the cookie on disk to be 64-bits and to XOR the cookie's 64-bits with the usage so that you could XOR on read and make sure they matched up.  But I don't think that actually accomplishes anything.  I mainly view the cookie as a helpful way of avoiding weird situations where users create their own usage file in notepad :)

@@ +1038,5 @@
> +
> +  nsCOMPtr<nsIBinaryOutputStream> binaryStream =
> +      NS_NewObjectOutputStream(stream);
> +
> +  rv = binaryStream->Write32(0);

Doing the 0-write, the flush, and then the seek and the close is overkill here since a disk sector is at least 512 bytes.  There's no chance of anything but the disk drive itself of fragmenting these writes (given write buffering).  This should just be a straightforward series of writes and then a Close().

You could put a Flush() in there, but given that Flush() really means fsync() and that has serious performance costs, I think it's important for us to explicitly have a "threat model" of what our fsync's are attempting to address and to document it in the file.  A big advantage of the journal file model is that really the creation of the journal file is the only thing that matters... as long as it hits disk persistently, then in the event of crash, we should rebuild things correctly.  So the threats are that the journal file never hits disk or that its removal hits disk without the usage file or database actually having been updated.

Given the very small amount of *dervied* data we're dealing with and that we're not willing to put SQLite's levels of testing or analysis into this, I don't think we need to worry about those too much as long as we make sure that we always re-derive the actual usage from the database state whenever localStorage is mutated.  Which is basically what we do.  The usage reported by the database after applying the delta is written to the usage file, so corruption of the usage file becomes irrelevant.  That said, it would be nice to get a MOZ_ASSERT that Datastore's mSizeOfItems matches what the database thinks.  (And under testing, we would also expect the usage file to match that as well at startup in PrepareDatastoreOp, but I think we do need to be prepared for that to experience a mismatch in the field).
Attachment #9031833 - Flags: review?(bugmail) → review-
Comment on attachment 9031832 [details] [diff] [review]
part 1

Review of attachment 9031832 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good but length() needs to be fixed and I'll need to review that change as a peer of mozStorage, so r- for now but that'll be a quick next review.

::: dom/localstorage/ActorsParent.cpp
@@ +3162,3 @@
>    AssertIsOnConnectionThread();
>    MOZ_ASSERT(aConnection);
> +  MOZ_ASSERT(aUsage);

If you're asserting this, why not just use a reference for aUsage?  Also, maybe rename it aOutUsage?

@@ +5768,5 @@
>  
>      rv = connection->CreateStatement(
> +        NS_LITERAL_CSTRING(
> +            "UPDATE database SET usage = ("
> +            "SELECT total(length(key) + length(value)) FROM data"

As you noted, the SQLite `length()` statement is explicitly about characters while nsTString::Length() is explicitly about code units (bytes for CString, char16_t for String).  I think this means we need a custom function.  I'd suggest just adding something like MozCStringLength to https://searchfox.org/mozilla-central/source/storage/mozStorageSQLFunctions.cpp so that all connections can have it.

And then that should be used here and above in the migration logic.
Attachment #9031832 - Flags: review?(bugmail) → review-
Attached patch updated part 1 (obsolete) — Splinter Review
Attachment #9031832 - Attachment is obsolete: true
Attachment #9032444 - Flags: review?(bugmail)
Attached patch part 1 interdiff (obsolete) — Splinter Review
(In reply to Andrew Sutherland [:asuth] from comment #12)
> ::: dom/localstorage/ActorsParent.cpp
> @@ +3162,3 @@
> >    AssertIsOnConnectionThread();
> >    MOZ_ASSERT(aConnection);
> > +  MOZ_ASSERT(aUsage);
> 
> If you're asserting this, why not just use a reference for aUsage?  Also,
> maybe rename it aOutUsage?

Yeah, it's now a ref and aOutUsage, but it's not in the interdiff.

> @@ +5768,5 @@
> >  
> >      rv = connection->CreateStatement(
> > +        NS_LITERAL_CSTRING(
> > +            "UPDATE database SET usage = ("
> > +            "SELECT total(length(key) + length(value)) FROM data"
> 
> As you noted, the SQLite `length()` statement is explicitly about characters
> while nsTString::Length() is explicitly about code units (bytes for CString,
> char16_t for String).  I think this means we need a custom function.  I'd
> suggest just adding something like MozCStringLength to
> https://searchfox.org/mozilla-central/source/storage/mozStorageSQLFunctions.
> cpp so that all connections can have it.

Yeah, we need a custom function, but length() in sqlite returns the same result
as nsTString::Length in most of the cases. I extracted the problematic item from
my profile and created a test based on that. For some reason sqlite length()
returns 25636 and nsTString::Length return 25637 in that case.
The custom function is called mozLength, since all existing custom functions have
quite short names. Let me know if I should change it.
(In reply to Andrew Sutherland [:asuth] from comment #11)
> I don't think we want to name any more of our files with a dot prefix now

Ok, "usage" sounds good.

> Please also add a comment like:
> 
> This file contains the current usage of the LocalStorage database as defined
> by the byte-length totals of all keys and values for the database, which

Added, but I replaced "byte-length" with "mozLength", is that ok ?

> @@ +134,5 @@
> >  #define JOURNAL_FILE_NAME "data.sqlite-journal"
> >  
> > +#define USAGE_FILE_NAME ".usage"
> > +
> > +#define USAGE_JOURNAL_FILE_NAME ".usage-journal"
> 
> This should also be renamed.
> 
> I'd suggest adding a comment here too, like:

Ok, renamed and added the comment.

> I mainly view the cookie as a helpful way of avoiding weird
> situations where users create their own usage file in notepad :)

Yeah, it's better than nothing.

> @@ +1038,5 @@
> > +
> > +  nsCOMPtr<nsIBinaryOutputStream> binaryStream =
> > +      NS_NewObjectOutputStream(stream);
> > +
> > +  rv = binaryStream->Write32(0);
> 
> Doing the 0-write, the flush, and then the seek and the close is overkill
> here since a disk sector is at least 512 bytes.  There's no chance of
> anything but the disk drive itself of fragmenting these writes (given write
> buffering).  This should just be a straightforward series of writes and then
> a Close().

Ok, I over-engineered it a bit :)

> That said, it would be
> nice to get a MOZ_ASSERT that Datastore's mSizeOfItems matches what the
> database thinks.  (And under testing, we would also expect the usage file to
> match that as well at startup in PrepareDatastoreOp, but I think we do need
> to be prepared for that to experience a mismatch in the field).

We already have that:
https://searchfox.org/mozilla-central/rev/2e5e28f518524f8af5b158ddb605022b6a2d68cf/dom/localstorage/ActorsParent.cpp#6002
It actually helped me to identify some of the problems we are dealing with before enabling LSNG.

But I can add one more assertion somewhere that queries the database.usage column
in debug builds even when usage file is ok, probably in a followup bug.
Attached patch updated part 2Splinter Review
Attachment #9031833 - Attachment is obsolete: true
Attachment #9032531 - Flags: review?(bugmail)
Attached patch part 2 interdiffSplinter Review
(In reply to Jan Varga [:janv] from comment #15)
> Yeah, we need a custom function, but length() in sqlite returns the same
> result
> as nsTString::Length in most of the cases. I extracted the problematic item
> from
> my profile and created a test based on that. For some reason sqlite length()
> returns 25636 and nsTString::Length return 25637 in that case.
> The custom function is called mozLength, since all existing custom functions
> have
> quite short names. Let me know if I should change it.

Your example has the (unassigned) unicode character 0xd1d11 in it.  This is represented as 2 utf-16 code points and so will have a JS string length of 2, an nsAString::Length() of 2, and a SQLite length() of 1.  The same thing will happen for any characters outside the Basic Multilingual Plane (BMP).  Since most characters fall in the BMP, the answers will usually be the same.  https://en.wikipedia.org/wiki/Plane_(Unicode) is a good reference to quickly identify what's in the various pages.

As quick check examples:
- In SQLite:
  sqlite> select length(char(0xd1d11));
  1
- In devtools console:
  > '\u{0d1d11}'.length
  2
Comment on attachment 9032444 [details] [diff] [review]
updated part 1

Review of attachment 9032444 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/localstorage/test/unit/test_stringLength.js
@@ +2,5 @@
> + * Any copyright is dedicated to the Public Domain.
> + * http://creativecommons.org/publicdomain/zero/1.0/
> + */
> +
> +async function testSteps()

As this test reads, the only explicit check is the check at the end of the file and that doesn't appear to actually check the new SQL function, it's just checking that the JS string length function works.

I believe our goals are to verify our usage is consistently done in utf-16 code units across all possible locations:
a. Not-yet-imported archived origin scope which dynamically runs a SQLite query.
b. The persisted "usage" value in the SQLite database that gets populated on import on first use.
c. The dynamic updates based on using nsAString::Length().
d. Paranoia: That the JS string length matches those prior values.

Thanks to the `MOZ_ASSERT(mUsage == mDEBUGUsage);`[1] assertion you pointed out, we have coverage of 'b', and thanks to the test at the bottom of this file, we have 'd'.  How would you feel about adding some getUsage() calls so that we can also get coverage of 'a' and 'c'?  So it would look like:

- import profile
- run getUsage() to verify the archived origin scope
- trigger LocalStorage to prepare the datastore.
- run getUsage() to verify after import.
- add a second copy of the value to LocalStorage
- run getUsage() to verify that the usage doubled.
- check the value's length like you already do.

1: https://searchfox.org/mozilla-central/rev/2e5e28f518524f8af5b158ddb605022b6a2d68cf/dom/localstorage/ActorsParent.cpp#6002

::: storage/mozStorageSQLFunctions.cpp
@@ +341,5 @@
>      ::sqlite3_result_error(aCtx, "User function returned error code", -1);
>    }
>  }
>  
> +void mozLengthFunction(sqlite3_context *aCtx, int aArgc,

Sorry for waffling on things here.  But as I look at your implementation here, I think we can/should do the following things:

- We should be able to just return the value of `sqlite3_value_bytes16(aArgv[0])` directly.  If we're using sqlite3_value_text16, we're already having SQLite do the utf8-to-utf16 conversion, so there's really no added benefit to sticking the string in a nsString class, as the Length() getter just returns the mLength it's initialized with.  (Also, since we're not mutating the string, we probably could use nsDependentString here if we wanted to look at its contents, but we don't, so sqlite3_value_bytes16 is fine.)
  - The docs link for the functions is at https://www.sqlite.org/c3ref/value_blob.html for our reference.
- Accordingly, I think maybe we should just rename the function as exposed to "utf16Length" and maybe name the C++ function "Utf16LengthFunction" or something like that.
Attachment #9032444 - Flags: review?(bugmail) → review+
Comment on attachment 9032531 [details] [diff] [review]
updated part 2

Review of attachment 9032531 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/localstorage/test/unit/test_corruptedDatabase.js
@@ +17,5 @@
> +  await requestFinished(request);
> +
> +  info("Installing package");
> +
> +  // The profile contains one localStorage, all localStorage related files, a

Great descriptive comment here, thank you!

(And this is also a very nice test of this edge-case!)

::: dom/localstorage/test/unit/test_originInit.js
@@ +2,5 @@
> + * Any copyright is dedicated to the Public Domain.
> + * http://creativecommons.org/publicdomain/zero/1.0/
> + */
> +
> +async function testSteps()

This is an amazing test.  Thorough checks, very readable in the code, and very informative to anyone reading the log output what might be going wrong.  Thank you for your thoroughness!

@@ +61,5 @@
> +      reader.readAsArrayBuffer(file);
> +    });
> +
> +    let view = new DataView(buffer, 8, 4);
> +    return view.getUint32();

It merits a comment here that you're manually getting the lower 32-bits because of the lack of support for 64-bit values currently from DataView/JS (other than the BigInt support that's currently behind a flag).
Attachment #9032531 - Flags: review?(bugmail) → review+
(In reply to Andrew Sutherland [:asuth] from comment #21)
> a. Not-yet-imported archived origin scope which dynamically runs a SQLite
> query.
> b. The persisted "usage" value in the SQLite database that gets populated on
> import on first use.
> c. The dynamic updates based on using nsAString::Length().
> d. Paranoia: That the JS string length matches those prior values.
> 
> Thanks to the `MOZ_ASSERT(mUsage == mDEBUGUsage);`[1] assertion you pointed
> out, we have coverage of 'b', and thanks to the test at the bottom of this
> file, we have 'd'.  How would you feel about adding some getUsage() calls so
> that we can also get coverage of 'a' and 'c'?  So it would look like:

I believe that the test does 'a' too.
- webappsstore.sqlite gets imported by installPackage()
- storage.getItem triggers datastore preparation which triggers quota manager's storage initialization which creates ls-archive.sqlite from webappsstore.sqlite [1]
- datastore preparation finds out there's data for migration [2]
- C++ GetUsage() is called for archived data (this uses the mozLength SQL function) [3]
- the patch here adds this C++ block to the |if (hasDataForMigration)| branch:
"UPDATE database SET usage = (SELECT total(mozLength(key) + mozLength(value)) FROM data"

On a second look, I think we can actually avoid calculating usage here again, we can just do:
"UPDATE database SET usage = :usage" and bind "newUsage" to it
newUsage is calculated by calling GetUsage() above

[1] https://searchfox.org/mozilla-central/rev/232ced2697b8938073fa79b8e6aa3718876c0b69/dom/quota/ActorsParent.cpp#4456
[2] https://searchfox.org/mozilla-central/rev/232ced2697b8938073fa79b8e6aa3718876c0b69/dom/localstorage/ActorsParent.cpp#5651
[3] https://searchfox.org/mozilla-central/rev/232ced2697b8938073fa79b8e6aa3718876c0b69/dom/localstorage/ActorsParent.cpp#5660

> 
> - import profile
> - run getUsage() to verify the archived origin scope
> - trigger LocalStorage to prepare the datastore.
> - run getUsage() to verify after import.
> - add a second copy of the value to LocalStorage
> - run getUsage() to verify that the usage doubled.
> - check the value's length like you already do.

Ok, I'll look at this and try to improved the tes even more.

> > +void mozLengthFunction(sqlite3_context *aCtx, int aArgc,
> 
> Sorry for waffling on things here.  But as I look at your implementation
> here, I think we can/should do the following things:
> 
> - We should be able to just return the value of
> `sqlite3_value_bytes16(aArgv[0])` directly.  If we're using
> sqlite3_value_text16, we're already having SQLite do the utf8-to-utf16
> conversion, so there's really no added benefit to sticking the string in a
> nsString class, as the Length() getter just returns the mLength it's
> initialized with.  (Also, since we're not mutating the string, we probably
> could use nsDependentString here if we wanted to look at its contents, but
> we don't, so sqlite3_value_bytes16 is fine.)
>   - The docs link for the functions is at
> https://www.sqlite.org/c3ref/value_blob.html for our reference.

I thought the idea is to let nsString to compute the string length.
Anyway, I tried to do this in the function:
int len = ::sqlite3_value_bytes16(aArgv[0]);
::sqlite3_result_int(aCtx, len);

As the documentation says, it returns "Size of UTF-16 TEXT in bytes"
so I get doubled results for the length when running test_stringLength.js
Once I divide it by 2, I get correct results.

However, I would rather let nsDependentString to do the length computation.
I understand you want to make it faster, but in practice this custom function
will only ever run in data migration code path (I know we can be synchronously
blocking the main thread in content process, but this is not I/O operation,
any string conversion and length computation done in memory should be fast).

> - Accordingly, I think maybe we should just rename the function as exposed
> to "utf16Length" and maybe name the C++ function "Utf16LengthFunction" or
> something like that.

utf16Length sounds good to me.
(In reply to Jan Varga [:janv] from comment #23)
> On a second look, I think we can actually avoid calculating usage here
> again, we can just do:
> "UPDATE database SET usage = :usage" and bind "newUsage" to it
> newUsage is calculated by calling GetUsage() above

Sounds good!

> However, I would rather let nsDependentString to do the length computation.
> I understand you want to make it faster, but in practice this custom function
> will only ever run in data migration code path (I know we can be
> synchronously
> blocking the main thread in content process, but this is not I/O operation,
> any string conversion and length computation done in memory should be fast).

For posterity, I agreed on IRC that it's reasonable enough to use nsDependentString here.  Since SQLite reuses internal buffers for this or at least explicitly frees them.  And we expect the database pages to already be in memory so we're not expecting much in the way of I/O magnification.
Attachment #9032444 - Attachment is obsolete: true
Attachment #9032802 - Flags: review?(bugmail)
Attached patch Part 1 interdiffSplinter Review
Attachment #9032445 - Attachment is obsolete: true
Comment on attachment 9032802 [details] [diff] [review]
Part 1: Cache usage in the database table

Review of attachment 9032802 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/localstorage/test/unit/test_stringLength.js
@@ +43,5 @@
> +
> +  // The profile contains storage.sqlite and webappsstore.sqlite.
> +  installPackage("stringLength_profile");
> +
> +  await checkUsage(0);

We actually don't support getting usage for archived data. It's probably doable in theory, but quite complicated.
Comment on attachment 9032802 [details] [diff] [review]
Part 1: Cache usage in the database table

Review of attachment 9032802 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/localstorage/test/unit/test_stringLength.js
@@ +43,5 @@
> +
> +  // The profile contains storage.sqlite and webappsstore.sqlite.
> +  installPackage("stringLength_profile");
> +
> +  await checkUsage(0);

Ah, right, gotcha, it's just for migration at https://searchfox.org/mozilla-central/rev/9528360768d81b1fc84258b5fb3601b5d4f40076/dom/localstorage/ActorsParent.cpp#5660
Attachment #9032802 - Flags: review?(bugmail) → review+
Pushed by jvarga@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2a2c0b1ebd5c
Part 1: Cache usage in the database table; r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/7bff7a09483d
Part 2: Cache usage in a standalone file; r=asuth
https://hg.mozilla.org/mozilla-central/rev/2a2c0b1ebd5c
https://hg.mozilla.org/mozilla-central/rev/7bff7a09483d
Status: ASSIGNED → RESOLVED
Closed: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
You need to log in before you can comment on or make changes to this bug.