Jan Varga [:janv] Assignee
	Description • 8 years ago

The next-generation should solve a bunch of problems:
- unification with other storage APIs (using QuotaManager)
- per origin database files
- database files in temporary storage (eviction of old data)
- e10s multi support
- move from PContent to PBackground

	Richard Newman [:rnewman]
	Comment 2 • 8 years ago

My main profile's 100MB webappstore.sqlite has 2,659 distinct originKeys.

Does that imply that my profile will have a couple of thousand SQLite databases, and a WAL for each one that's open?

That might not work out on mobile, even if you manage to put these files in the Android Cache directory. Heck, it might not be too fun on my MacBook Pro…!

Am I misunderstanding your approach?

	Jan Varga [:janv] Assignee
	Comment 3 • 8 years ago

I have a similar profile (60MB, around 1800 domains), so I'm testing with real world data.
Yes, local storage is special in many regards. It's synchronous API which makes it quite hard to plug into quota manager, but it's been here for some time and many sites use it. However, we never evicted unused data, so we ended up with 2-3 thousand distinct domains.
I'm thinking about creating separate databases only for new local storage and about converting origins with existing local storage lazily. I believe many of those distinct domains won't be ever converted, so we will have to delete them when other domains need to store more data.

	Jan Varga [:janv] Assignee
	Comment 4 • 8 years ago

And btw, I'm not going to use SQLite for new local storage implementation :)

	Richard Newman [:rnewman]
	Comment 5 • 8 years ago

\o/

I was worried about the per-database overhead for multi-tab mobile users! Glad to hear you have it all in hand :D

	Jan Varga [:janv] Assignee
	Comment 6 • 8 years ago

multi-tab shouldn't be such a big concern since local storage will be preloaded in memory so the per origin database can be "closed" after preloading

	Jan Varga [:janv] Assignee
	Comment 7 • 7 years ago

(In reply to Jan Varga [:janv] from comment #0)
> The next-generation should solve a bunch of problems:
> - unification with other storage APIs (using QuotaManager)
> - per origin database files
> - database files in temporary storage (eviction of old data)
> - e10s multi support
> - move from PContent to PBackground

Just a quick update:
- e10s multi support landed, although there is room for improvements
- move from PContent to PBackground landed too, bug 1350637

	Andrew Sutherland [:asuth] (he/him)
	Comment 8 • 7 years ago

I'm updating the subject to make this easier to find, and I want to give a dump of my current understanding of :janv's plan, as I think the design has a lot of wins, but there are trade-offs involved and we want any discussion/debate to happen sooner rather than later.

## Plan
My mental model of the overhaul plan is (Jan, please correct me if I'm wrong!):
- All (canonical) LocalStorageCache data state is held in the parent process.
- We have actors per-LocalStorageCache which are per-principal (that is, origin+attributes) instead of the current "one actor per process that is conceptually just a remoted database connection" so that we can scope events properly.
- All localStorage reads happen via sync IPC to PBackground.  (Or, if necessary, a new PLocalStorageIsDumbAndNeedsItsOwnThread.)
- localStorage writes probably also happen via sync IPC[1].
- Events happen via async IPC if there are any "storage" event listeners in a content process.  The subscription should happen synchronously for maximum correctness, but it's fine to subscribe async under the spec.  This means that a change can be perceived by reading localStorage for which a "storage" event has not yet been received.  This is fine and consistent with how LocalStorage already works in Chromium/Blink (and probably webkit too?).  It's also how we've worked for a while.
- We use QuotaManager, but the origin never gets more than 5MiB of LocalStorage space (+slack) no matter what.

1: At least some writes could be optimized to be async if we're willing to allow the origin to exceed quota.  For example, Chromium allows quota to be exceeded by 100k because of multi-process races.  Our implementation could similarly allow each process some "slack" quota that permits async write messages up to that quota (and assuming no space would be freed by overwrites) as long as we're (sufficiently) under quota.  If a write would exceed the slack size, a sync write is performed and resets the outstandingSlackBytes to 0.  In the best-case scenario, outstandingSlackBytes never exceeds the threshold, and is continually re-zeroed by async IPC messages that confirm the writes and current quota usage for the given cache.

## Upsides
We get the following wins out of this implementation strategy versus our current strategy:
- Reduced memory usage.  Right now each browser process that has a window/iframe loaded for an origin that uses LocalStorage will carry the memory burden of the origin's entire LocalStorage usage in every process with the window/iframe loaded.  For sites that use localStorage and are very popular or are embedded as 3rd-party iframes (and assuming we don't double-key them), this could be a lot of memory[2].
- Globally consistent, sequential view of localStorage across all processes (unless we make any caching optimizations).  The HTML spec provides us basically infinite wiggle room when multiple processes are involved, but I know of at least one instance of LocalStorage being used by a site as a mutex between multiple windows.  There are arguably no downsides 
- Improved page load performance where the origin has a ton of data stored in localStorage but only needs a small amount of it near startup.  If the origin has 5MiB stored and it uses localStorage at all, all 5 MiB needs to be loaded from disk and received by the thread responsible for storage.  And if we go synchronous (in the current implementation), that could mean a simple boolean check has to wait on 5 megs of data.  Based on our current DB strategy, the load is still unavoidable, but we avoid having to ship it over IPC or process that on the content process main thread.

2: Note that this regressed in bug 1285898 out of necessity.  Previously, we'd drop the storage if a page didn't actually use LocalStorage within ~20 seconds.  This could be added back without the overhaul by addressing bug 1350071.

## Downsides
- Synchronous IPC is synchronous IPC, and this approach means more of it, even if it's in smaller bite-sized pieces.  Although we can mitigate by letting Quantom DOM context-switch to other TabGroups, it's not a win if we're perceptibly stalling a foreground TabGroup the user cares about.

## Potential Mitigations
- Use some kind of clever multi-process shared-memory map to make all reads except the preload non-blocking, and all writes async.  This would increase complexity and lose the globally consistent sequence.
- Create a second mode of operation for localStorage-heavy origins.  Once an activity threshold is crossed,  cache the entirety of the state in the content process and rely on the "storage" events (which must be subscribed to even if not delivered to content pages) to update the local cache.  If there's heavy write-load, consider coalescing the writes locally by tracking dirty keys and flushing on a timer, much like we do in the database thread already.

## Meta: Metrics / Benchmarks / Synthetic Benchmarks

From my perspective, we don't have a good understanding of how LocalStorage is used in the wild or a clear goal to optimize for than to reduce the scalar telemetry LOCALDOMSTORAGE_GETVALUE_BLOCKING_MS and make the boolean telemetry LOCALDOMSTORAGE_PRELOAD_PENDING_ON_FIRST_ACCESS as false as possible.  These are mainly questions of moving the preload trigger as early as possible in the pipeline, optimizing the database queries, and reducing database fragmentation.  These are all orthogonal to whether we do the in-parent overhaul or instead massively clean-up the existing implementation.  (Although our current implementation is limited in how early we can move the preload trigger because of how broadcast is implemented.)

It might be beneficial to instrument our LocalStorage implementation with MOZ_LOG statements that can generate optionally-anonymized traces of localstorage usage.  They'd log all reads/writes as `origin "${hashy(perUserRandomKey, origin}" key "${hashy(perUserRandomKey, key)}" ${key.length} ${readOrWriteOrClear} ${value && value.length}`.  Some kind of "about:networking" "logging"-tab-style affordance could turn on the logs for helpful people, who could then provide us privately with their logs with the understanding that we could reverse things if we brute-forced the per-user-key/salt or did traffic-analysis, but that we would not do so and this would largely avoid us accidentally learning anything private when skimming or otherwise analyzing logs.

## Analytical tools.
### Per-origin via Devtools console
Paste this into a dev-tools console of any browser on any origin to get a report on what localStorage is up to:
```
(function() { var valueBytes=0; var keyBytes=0; for (var i=0; i < localStorage.length; i++) { var key = localStorage.key(i); keyBytes += key.length; valueBytes += localStorage[key].length; } console.log("localStorage has", localStorage.length, "keys and uses", keyBytes, "key bytes,", valueBytes, "value bytes,", keyBytes+valueBytes, "total bytes-ish"); })();
```
ex, my google.com in my Firefox primary profile:
localStorage has 177 keys and uses 5738 key bytes, 17683 value bytes, 23421 total bytes-ish
ex, my google.com in my Google Chrome MoCo profile:
localStorage has 168 keys and uses 3295 key bytes, 2623 value bytes, 5918 total bytes-ish

### Whole profile via browser console
Open up a browser console via ctrl-shift-j (you may need to enable it first): paste this in and run it:
```
Components.utils.import("resource://gre/modules/Sqlite.jsm"); (async function() { const conn = await Sqlite.openConnection({ path: "webappsstore.sqlite", sharedMemoryCache: false }); await conn.execute("select sum(length(key) + length(value))/1024 AS total, originkey from webappsstore2 GROUP BY originKey HAVING total > 1024 ORDER BY total ASC", [], (row) => { console.log(row.getResultByIndex(0), row.getResultByIndex(1)); }); conn.close(); console.log("That's it."); })()
```

Note that if you have no origins with over 1 meg, you'll just see "That's it."  You can adjust the query as needed.

### Whole profile via sqlite3 command line tool
Same as the browser console invocation above.

Run this inside a `sqlite3 webappsstore.sqlite` to get a rough list of per (reversed) origin localStorage usage in KiB for sites using more than a meg:
```
select sum(length(key) + length(value))/1024 AS total, originkey from webappsstore2 GROUP BY originKey HAVING total > 1024 ORDER BY total ASC;
```
or direct from the command-line in your profile directory:
```
sqlite3 webappsstore.sqlite "select sum(length(key) + length(value))/1024 AS total, originkey from webappsstore2 GROUP BY originKey HAVING total > 1024 ORDER BY total ASC;"
```

	Jan Varga [:janv] Assignee
	Comment 9 • 7 years ago

(In reply to Andrew Sutherland [:asuth] from comment #8)

Thanks for the plan description, nice written!
As others can see, this is not trivial at all.

I have some more comments:
1. Regarding sync/async writes. We could avoid the slack if decide to keep the precise quota checks strategy by making QuotaObject::MaybeUpdateSize to work on PBackground thread (or special LocalStorage thread dedicated for IPC in the parent process).
The idea is that in LocalStorage it's easy to calculate how much data (or at least maximum) will be stored on disk. It's a simple key.length + value.length calculation. So we can try to synchronously pre-allocate the size (which would check the group limit and possibly free some space). Writes would then be asynchronously finished and after we know real size we can again update quota object (we could also try to compress in content process and send compressed bytes across IPC).
This can't work well with SQLite because it's hard to estimate how much data will actually be stored on disk, so I'm also considering to drop SQLite in LocalStorage and replace it with own simple db system.
All we need is to write key/value pairs using snappy compression and splitting these pairs across multiple files (to avoid 5 MB writes when only 1 byte is changed in the worst case). The splitting can be done based on hashing the key or something like that.
I already experimented with the simple db system. (Note this is not simpledb, bug 1361330).

2. Hopefully sync IPC reads won't be a performance problem since it makes everything much cleaner and less complicated.

3. Local storage db can contain thousands of unique domains. The current quota manager storage placement on disk is a directory per origin and these directories must be scanned during default/temporary storage initialization.
However, we don't need to create all of them. We can do it on demand when they are actually needed. Something like lazy upgrade of old Local Storage db.
This can be improved even more and have a zip file with compressed (unused) origin directories and unzip them when needed.
Finally, we can also cover a case when there's plenty of space on disk so origin directories never get evicted, but the number of origin directories gets quite big after some time. We can use the same archive/zip file to archive unused origin directories.

Of course, we don't have to implement everything at once.

	Andrew Sutherland [:asuth] (he/him)
	Comment 10 • 7 years ago

(In reply to Jan Varga [:janv] from comment #9)
> This can't work well with SQLite because it's hard to estimate how much data
> will actually be stored on disk, so I'm also considering to drop SQLite in
> LocalStorage and replace it with own simple db system.
> All we need is to write key/value pairs using snappy compression and
> splitting these pairs across multiple files (to avoid 5 MB writes when only
> 1 byte is changed in the worst case). The splitting can be done based on
> hashing the key or something like that.
> I already experimented with the simple db system. (Note this is not
> simpledb, bug 1361330).

Can we dig into this a bit more?

I agree that given our usage pattern for localStorage, the main thing SQLite is doing is bounding I/O for small writes, as you say.  But I don't view the SQLite disk overhead as particularly concerning[1].  At least not worth the maintenance overhead of implementing a persistence layer that has to juggle multiple files.

That's not to say there isn't a gap in our set of persistence solutions, especially from C++ code.  We basically have SQLite and the ServiceWorkerRegistrar-style "sketchily read and write a text-file that's a downgrade nightmare" strategies.  JS is in better shape because it has easy JSON flat-file serialization and IndexedDB.

My thoughts in this area were that now that we can write rust code we can use its "serde" serialization/deserialization library to allow our platform code to do easily do JSON-style serialization.  And it seems conceivable that there is some middle-ground between flat-file and SQLite, but I think we want there to be an existing open source project we build around that's aligned with our use-case.  Or at least, it should be more than just you taking on an embedded database engine as part of your day job :)

Note that MoCo is a member of the SQLite consortium (https://sqlite.org/consortium.html) which means we actually can get a lot of their time; we're already paying for it, we're just not using it.  I'm sure they'd be happy to advise us on how to optimize for the localStorage case.  And it's worth pointing out that SQLite has a lot of extensibility.  For example, there's the CSV virtual table at https://sqlite.org/csv.html, plus, as you know, there's the VFS layer.  And as a virtual table, we'd get to use all of our existing mozStorage bindings, any rust bindings we adopt, etc.

1: The localStorage 5meg limit is in many ways orthogonal to the QuotaManager limit.  It's a policy decision from the pre-QuotaManager era that now is more about bounding memory use from LocalStorage-sites than it's about disk space.  To avoid breaking on sites that depend on the limit as we previously implemented it, we need to give them that space as we originally measured it, roughly speaking.  QuotaManager does need to deal in real-world disk usage, but there's no reason it has to insist on the localStorage on-disk usage being exactly 5 megabytes.  It can give it 10.  It should enforce our more generous upper limits, which we may need to boost slightly with the localStorage move to QM, but those are more abstract as far as the various origins are concerned.

	Jan Varga [:janv] Assignee
	Comment 11 • 7 years ago

I haven't made any decision regarding database system for LocalStorage, I just experimented with own one :)
I'm all for an existing open source system that fits our needs.

	Richard Newman [:rnewman]
	Comment 12 • 7 years ago

2¢ on this: adding another database tool of almost any kind is a non-trivial installer size bump, and that really matters on mobile, particularly if it's a tool that's only used for one specific case.

That means I tend to see fewer middle grounds between flat files (which have lots of problems) and SQLite (which solves lots of problems) -- the only acceptable middle grounds are those that have a future across a number of components and kill a bunch of other code, because we don't want to pay a few MB of library size just for one feature.

I say that as someone who very much thinks adding a suitable database tool to the codebase is a good idea!

	Jan Varga [:janv] Assignee
	Comment 13 • 7 years ago

If we go the SQLite route here's proposed db schema:

CREATE TABLE database (
  origin TEXT NOT NULL
  last_vacuum_time INTEGER NOT NULL DEFAULT 0,
  last_analyze_time INTEGER NOT NULL DEFAULT 0,
  last_vacuum_size INTEGER NOT NULL DEFAULT 0
)  WITHOUT ROWID;

CREATE TABLE data (
  key TEXT PRIMARY KEY,
  value TEXT NOT NULL,
  keyCompressed INTEGET NOT NULL,
  valueCompressed INTEGET NOT NULL,
  lastAccessTime INTEGER NOT NULL DEFAULT 0
);

CREATE INDEX data_key ON data(key);

Features (some of them just for consideration):
- WAL is probably not needed for such small databases
- queue changes coming from content processes and apply them to SQLite periodically or when the size gets over a threshold
- after a database update, check if it's worth to vacuum the database (or vacuum only when we get the idle notification)
- compress keys and values if the length is greater than a threshold, if compressed data isn't significantly smaller, keep key/value uncompressed
- compression/decompression would happen in content processes
- pre-loaded data could stay compressed to send less bytes across IPC either for normal operations or for distributing the change events
- every key/value pair would have last access time, if such a pair is not used for long time, it doesn't have to be preloaded next time we preload data for entire origin
- lastAccessTime may also serve for clearing session data based on time constraints ?

It would be nice if we could calculate the size needed by SQLite on disk for given key/value pair in the worst case (no free blocks in the database).
That way we could just synchronously pre-allocate space using QuotaManager before setItem() call is finished.
The pre-allocated space would then be freed once a database update is finished.

	Jan Varga [:janv] Assignee
	Comment 14 • 7 years ago

backup

	Jan Varga [:janv] Assignee
	Comment 15 • 7 years ago

backup

	Jan Varga [:janv] Assignee
	Comment 16 • 7 years ago

backup

	Jan Varga [:janv] Assignee
	Comment 24 • 6 years ago

Ok, Part 6 implements persistence on disk

TODO (may miss some items):
- quota checks
- migration of old data
- support for private browsing mode
- storage events

	Marion Daly [:mdaly]
	Comment 25 • 6 years ago

:janv update the bug with latest info please?

	Jan Varga [:janv] Assignee
	Comment 34 • 6 years ago

(In reply to Marion Daly [:mdaly] from comment #25)
> :janv update the bug with latest info please?

Sure, here are current patches with some description:

Part 1: Allow BackgroundChild::GetOrCreateForCurrentThread() to use a custom main event target

Part 2: Add IsOnDOMFileThread() and AssertIsOnDOMFileThread() generic helper methods

Part 3: New basic (memory only) implementation of LocalStorage;

The implementation is based on a cache (datastore) living in the parent process and sync IPC calls initiated from content processes.
IPC communication is done using per principal/origin database actors which connect to the datastore.
The synchronous blocking of the main thread is done by creating a nested event target and spinning the event loop.

Part 4: Basic integration with QuotaManager

This adds a new quota client implementation, but only implements ShutdownWorkThreads.
At shutdown we wait for all running operations to finish including database actors which are closed by using an extra IPC message which avoids races between the parent and child.
Databases are dropped on the child side as soon as they are not used (e.g. after unlinking by the cycle collector).

Part 5: More integration with QuotaManager

Preparation of datastores now creates real database files on disk. The LocalStorage directory is protected by a directory lock.
Infrastructure for database schema upgrades is in place too.
Database actors are now explicitely tracked by the datastore. When the last actor finishes the directory lock is released.
Added also implementation for QuotaClient::GetUsageForOrigin() and QuotaClient::AbortOperations().

Part 6: Fix a dead lock in the single process case

In the single process case, pass the nested main event target across IPC.
The event target is then used by the parent side to process runnables which need to run on the main thread.
After this change we don't have use hacks like getting profile directory path on the child side and sending it to the parent. The parent side can now do it freely even in the single process case.

Part 7: Teach QuotaManager's OriginParser to handle file://UNIVERSAL_FILE_URI_ORIGIN

See bug 1340710 which introduced this kind of URI.

Part 8: Persist datastores to disk; r=asuth

Introduced a Connection and a ConnectioThread object.
All I/O requests are processed by a new single thread managed by ConnectionThread object.
Connection objects are prepared by the prepare datastore operation and then kept alive by datastores just like directory locks.
All datastore I/O operations are wrapped in a transaction which automaticaly commits after 5 seconds.
Datastore preparation now also loads all items from the database.


So most of the stuff that is mentioned in my initial description of this bug and in comment 8 is implemented except:
- storage event notifications
- quota checks
- private browsing
- data migration

I'm currently working on:
- storage event notifications

In patch Part 3 I disabled tests that currently don't pass on try, they are mostly related to storage events not working.

My goal is to have all of this in FF 61, if I have time, I'd like to also implement data compression and some other performance and memory enhancements.

	Jan Varga [:janv] Assignee
	Comment 35 • 6 years ago

From the last update in comment 24 I was mostly working on:
1. Datastore/cache sharing by multiple tabs
This took a bit more time than I originally thought, but it's now working nicely. A datastore object is not tight to an actor, so it can be "preloaded" much sooner or can be kept alive for some time.

2. Bullet-proof quota client shutdown
I paid extra attention to this because we see many problems/crashes related to this. Some of the ideas/approaches/fixes can be used in other quota clients, especially in IndexedDB

3. Make it pass on try
I had to fix a problem with quota manager initialization ending in a dead lock in the single process case. Hopefully I fixed it in a way that we won't have to use hacks to handle the single process case.
Then I discovered that we introduced file://UNIVERSAL_FILE_URI_ORIGIN and some LocalStorage tests use that, so I had to fix the OriginParser. We might find out that some IndexedDB initialization issues are related to this special URI.
And a bunch of other small problems.

4. Code cleanup
I cleaned up the code and added many comments. The attached patches are ready to reviewed I think

	Jan Varga [:janv] Assignee
	Comment 36 • 6 years ago

Progress update:

1. storage event notifications
- implemented, but needs cleanup

2. private browsing
- fixed

TODO:
- quota checks
- data migration
- support for session only local storage (can we just drop this feature ?)
- fix clearing triggered by extension API, preferences, etc.

	Marion Daly [:mdaly]
	Comment 37 • 6 years ago

Session only local storage: research if others use it, talk to :asuth about it, if no one uses it we can safely dump it. Who would be stakeholders here?

	Jan Varga [:janv] Assignee
	Comment 38 • 6 years ago

We will have to do something for CPOWs, some tests still use it.

Currently it can lead to a dead lock when local storage in the content process is synchronously blocking the main thread.

Here's the scenario:
- a content process touches local storage for the first time
- the content process issues a request to the parent process to prepare a datastore for local storage
- the content process synchronously blocks the main thread while waiting for a response
- the parent process sends a CPOW synchronous message (on the main thread) to the content process 
- the parent process receives the local storage request (on the PBackground thread) and starts a state machine
- one of the steps of the state machine is work on the main thread in the parent process, so a runnable is dispatched to the main thread
- the runnable is not executed because the main thread is doing a synchronous CPOW request
- the state machine can't continue, so both ends wait for each other

I found out that if I send a synchronous message from the content process to the parent while spinning the event loop, then the CPOW request gets processed. However, I'm not sure if it's safe and we want to allow it.
My understanding is that we want to get rid of CPOWs at some point and they are already disabled for normal web content.
So we should rewrite those tests that use local storage using CPOWs and somehow disable CPOWs when local storage is going to synchronously block the main thread (to rather throw an exception instead of deadlocking).

	Andrew Sutherland [:asuth] (he/him)
	Comment 39 • 6 years ago

(In reply to Marion Daly [:mdaly] from comment #37)
> Session only local storage: research if others use it, talk to :asuth about
> it, if no one uses it we can safely dump it. Who would be stakeholders here?

I think the consensus reached at the Storage UX meeting in December at MozAustin, as I reported at bug 1400678 comment 3, is that clear-on-shutdown is sufficient for our session-only semantics.

I think it's also somewhat agreed that the current semantics of session-only LocalStorage bootstrapping from whatever is already persisted to LocalStorage on disk is weird and defies user expectations.  So, specifically, if the user goes to the canonical private browsing example of an engagement ring site, does something that persists data, then switches the site (or global preference) to session storage, the session storage will be bootstrapped from that already-persisted data each time, which is unexpected.

So:
- If we implement QuotaManager clear-on-shutdown in bug 1400678, I think we can remove the concept of session-only storage from LocalStorage here.
- If we don't implement bug 1400678, we might be able to simplify session-only to non-persistent in-memory-only.  (And since PrivateBrowsing is segregated by OriginAttribute, that further eliminates the need for slots.)

	Jan Varga [:janv] Assignee
	Comment 40 • 6 years ago

Thanks Andrew, that makes perfect sense.

	Jan Varga [:janv] Assignee
	Comment 41 • 6 years ago

Latest try looks good:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7e5604be21b713dc4f0fe82a1e6679c07fe27b1c

Progress since last update:
- storage events implementation optimized, cleaned up and heavily tested
- local storage clearing fixed except browser_ext_browsingData_localStorage.js
- implemented cancellable datastore preparation to fix problems with CPOWs
- removed tests for session only support
- added checks for dom.storage.enabled pref and StorageAllowedForPrincipal()
- added support for low disk space mode

Tests that are still temporarily disabled:
test_bug600307-DBOps.html
test_localStorageFromChrome.xhtml
browser_ext_browsingData_localStorage.js
test_localStorageQuota.html
test_localStorageQuotaPrivateBrowsing_perwindowpb.html
dom/tests/browser/browser_localStorage_e10s.js

which results in this TODO list:
- broadcast domstorage-test-flushed message
- fix LocalStorage in chrome
- enhance nsIQuotaManagerService::clearStoragesForPrincipal() to accept a quota client argument
- finish quota checks
- database migration

	Jan Varga [:janv] Assignee
	Comment 42 • 6 years ago

(In reply to Jan Varga [:janv] from comment #41)
> - fix LocalStorage in chrome

fixed

> - enhance nsIQuotaManagerService::clearStoragesForPrincipal() to accept a quota client argument

fixed

	Jan Varga [:janv] Assignee
	Comment 43 • 6 years ago

(In reply to Jan Varga [:janv] from comment #41)
> - broadcast domstorage-test-flushed message

fixed

> - finish quota checks

origin quota checks - done
quota manager quota checks:
- group limit - done
- global limit - needs more work

All tests now pass except some devtools related ones that still use CPOWs

Yesterday asuth and I had a pre-review meeting to discuss current patches.

The new implementation is not fully complete (migration is still missing), but I'll start submitting patches for review following days.

	Jan Varga [:janv] Assignee
	Comment 44 • 6 years ago

Andrew, the new local storage implementation uses a nested event queue for synchronous blocking of the main thread in content processes. At the same time we dispatch to a DOM file thread that communicates with the parent process/PBackground thread asynchronously. The problem happens when GetOrCreateForCurrentThread() is called on the DOM file thread and a new runnable is dispatched to the main thread. The runnable connects the newly created PBackgroundChild with the parent process. However, the runnable never gets processed since we created a nested event queue on the main thread. This patch allows GetOrCreateForCurrentThread() to use a custom main event target, so we can pass our nested event queue.

	Jan Varga [:janv] Assignee
	Comment 47 • 6 years ago

Sorry, attaching a new version, the previous patch doesn't build with latest m-c.

	Andrew McCreight [:mccr8]
	Comment 52 • 6 years ago

Comment on attachment 8956397 [details] [diff] [review]
Part 1: Allow BackgroundChild::GetOrCreateForCurrentThread() to use a custom main event target

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

I don't know much about event targets but this looks reasonable.

::: ipc/glue/BackgroundChild.h
@@ +11,5 @@
>  #include "mozilla/Attributes.h"
>  #include "mozilla/ipc/Transport.h"
>  
>  class nsIDOMBlob;
> +class nsIEvenTarget;

nsIEventTarget

::: ipc/glue/BackgroundImpl.cpp
@@ +1460,2 @@
>  {
> +  MOZ_ASSERT_IF(NS_IsMainThread(), !aMainEventTarget);

Should this assert also be on BlockAndGetResults()? That also does a dispatch of this new argument.

	Jan Varga [:janv] Assignee
	Comment 53 • 6 years ago

(In reply to Andrew McCreight [:mccr8] from comment #52)

Thanks for quick review!

> ::: ipc/glue/BackgroundImpl.cpp
> @@ +1460,2 @@
> >  {
> > +  MOZ_ASSERT_IF(NS_IsMainThread(), !aMainEventTarget);
> 
> Should this assert also be on BlockAndGetResults()? That also does a
> dispatch of this new argument.

Yeah, but I think that method shouldn't be called on the main thread at all, I added |AssertIsNotOnMainThread()| there.

	Marion Daly [:mdaly]
	Comment 57 • 6 years ago

What sort performance regressions should we expect from this?

	Jan Varga [:janv] Assignee
	Comment 58 • 6 years ago

(In reply to Marion Daly [:mdaly] from comment #57)
> What sort performance regressions should we expect from this?

Page load time can be affected by this.
In theory, with all patches applied, especially the one that triggers preloading in ContentParent::AboutToLoadHttpFtpWyciwygDocumentForChild(), the synchronous waiting for I/O should happen less.
Storage events are also distributed in a more efficient manner.

Anyway, I'll have to do some talos testing.

	Jan Varga [:janv] Assignee
	Comment 59 • 6 years ago

Andrew, patches 1-10 implement most of the stuff and most of the tests pass.
Here's latest try for pathes 1-10:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6b0cd9168e2b8476f76c06fc66139814eaf5a478

I'll be now attaching patches to fix the rest of tests and also patches with some improvements.
I'll attach them one by one as I finish final cleanup and self-review.

	Jan Varga [:janv] Assignee
	Comment 69 • 6 years ago

Comment on attachment 8957303 [details] [diff] [review]
Part 19: Implement a helper method for datastore preloading verification

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

Andrew, I did only the minimum in browser_localStorage_e10s.js
Feel free to suggest what needs to be changed/removed or you can even do it yourself if you have a bit of time.
It's your test and you know best what needs to be done :)

	Jan Varga [:janv] Assignee
	Comment 71 • 6 years ago

Comment on attachment 8957133 [details] [diff] [review]
Part 18: Verify that data is persisted on disk

Andrea, the Storage.webidl changes need to be reviewed by a DOM peer. Can you take a look ?

	Jan Varga [:janv] Assignee
	Comment 72 • 6 years ago

Bobby, the universal file origin was introduced in bug 1340710.
The base domain is currently not handled if strict file origin policy is not in effect which confuses quota manager.
I noticed this problem when I was testing new local storage implementation, but I guess it could already cause (hidden) problems with IndexedDB too.

	Andrea Marchesini [:baku]
	Comment 73 • 6 years ago

Comment on attachment 8957133 [details] [diff] [review]
Part 18: Verify that data is persisted on disk

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

r+ for the webidl. I leave all the rest to Andrew.

	Bobby Holley (:bholley)
	Comment 74 • 6 years ago

Comment on attachment 8957324 [details] [diff] [review]
Part 21: Base domain needs to be handled too if strict file origin policy is not in effect

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

Hm, why do we need this boolean on every codebase principal, rather than just querying dynamically? Is it to handle the pref changing mid-run? I don't think I really care to support that, so how about just checking GetStrictFileOriginPolicy directly in GetBaseDomain?

	Jan Varga [:janv] Assignee
	Comment 75 • 6 years ago

(In reply to Bobby Holley (:bholley) from comment #74)
> Comment on attachment 8957324 [details] [diff] [review]
> Part 21: Base domain needs to be handled too if strict file origin policy is
> not in effect
> 
> Review of attachment 8957324 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Hm, why do we need this boolean on every codebase principal, rather than
> just querying dynamically? Is it to handle the pref changing mid-run? I
> don't think I really care to support that, so how about just checking
> GetStrictFileOriginPolicy directly in GetBaseDomain?

Yeah, that's what I did initially, but I got assertions on try, because some
doc group code gets the base domain before the pref for file
origin policy changes and then gets it again and compares with the previous
value after the pref changes.
So I think the only way is to snapshot the pref setting.

	Jan Varga [:janv] Assignee
	Comment 76 • 6 years ago

Here's the failing try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=85ad51bbb284dcf2538b27a82a15867db8c4ed3f&selectedJob=164439785
See all the R7 and Ru7 failures.

	Bobby Holley (:bholley)
	Comment 77 • 6 years ago

Comment on attachment 8957324 [details] [diff] [review]
Part 21: Base domain needs to be handled too if strict file origin policy is not in effect

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

Ugh, ok. Add a comment explaining why this is there, then?

	Jan Varga [:janv] Assignee
	Comment 78 • 6 years ago

(In reply to Bobby Holley (:bholley) from comment #77)
> Comment on attachment 8957324 [details] [diff] [review]
> Part 21: Base domain needs to be handled too if strict file origin policy is
> not in effect
> 
> Review of attachment 8957324 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Ugh, ok. Add a comment explaining why this is there, then?

Ok, will do. Thanks!

	Jan Varga [:janv] Assignee
	Comment 83 • 6 years ago

Ok, the only patch that is not yet ready for review is data migration.
It should be ready in coming days.

	Thomas Wisniewski
	Comment 84 • 6 years ago

Jan, I haven't had a chance to investigate this patchset, but will it add a concept of last-accessed timestamps to localStorages so that we can implement support for the "since" parameter of the browsingData extension API for clearing localStorages? If not, do you think it's worth adding it now to reduce the number of necessary data migrations down the line?

	Jan Varga [:janv] Assignee
	Comment 86 • 6 years ago

(In reply to Thomas Wisniewski from comment #84)
> Jan, I haven't had a chance to investigate this patchset, but will it add a
> concept of last-accessed timestamps to localStorages so that we can
> implement support for the "since" parameter of the browsingData extension
> API for clearing localStorages? If not, do you think it's worth adding it
> now to reduce the number of necessary data migrations down the line?

There's a lastAccessTime column, but it's not used right now. I could add another patch that initializes it, but I wouldn't bet that it's enough for the feature you are talking about.

	Marion Daly [:mdaly]
	Comment 87 • 6 years ago

(In reply to Thomas Wisniewski from comment #84)
> Jan, I haven't had a chance to investigate this patchset, but will it add a
> concept of last-accessed timestamps to localStorages so that we can
> implement support for the "since" parameter of the browsingData extension
> API for clearing localStorages? If not, do you think it's worth adding it
> now to reduce the number of necessary data migrations down the line?

Can this be a feature we implement in a future release? It isn't necessarily related to this patch set.

	Thomas Wisniewski
	Comment 88 • 6 years ago

>Can this be a feature we implement in a future release?

Oh I'm certainly not against that, and wouldn't expect the full feature to be implemented right now. I was just wondering if it was worth minimizing potential data-migrations (and starting to let the data be annotated in user profiles ahead of time), if it was trivial to extend this patch right now.

>I could add another patch that initializes it, but I wouldn't bet that it's enough for the feature you are talking about.

That's fine, I just think it's worth giving it a shot since we know it will be necessary, and we want to implement the feature anyway. But if it's not trivial and will delay this patchset, then let's just skip it for now.

	Andrea Marchesini [:baku]
	Comment 89 • 6 years ago

Thomas, something you could be interesting in, is bug 1252998 where I'm introducing a new service able to store which principal has done storage operation. This is meant to be used for clear history, but probably it's fine for you too.

Those patches are not landed because I don't have time to fix 1 test failure... but if you want to help, that would be great!

	Thomas Wisniewski
	Comment 90 • 6 years ago

Thanks baku, I'll see if I can scrounge up the time to help rebase that patchset and get it landed (but I'll be posting my XHR implementation patchset first).

And with that bug in mind, I suppose it's not worth delaying this one, so I retract my request.

	Jan Varga [:janv] Assignee
	Comment 91 • 6 years ago

(In reply to Jan Varga [:janv] from comment #85)
> Created attachment 8960540 [details] [diff] [review]
> data migration WIP

So, in the end I didn't implement a zip archive, I just move webappsstore.sqlite to <profile>/storage/ls-archive.sqlite and then lazily migrate data from it when it's needed. It works quite well. It's nice that SQLite supports transaction across multiple databases, so ensuring that stuff is done atomically is quite easy.
I also believe that we don't have to do a major storage upgrade (only minor), so it won't affect users switching between release/beta/nightly.
I'll attach a new patch once I finish final cleanup.

I also have a patch that removes old local storage implementation, just to be sure that everything uses new implementation.

	Marion Daly [:mdaly]
	Comment 92 • 6 years ago

(In reply to Jan Varga [:janv] from comment #91)
> (In reply to Jan Varga [:janv] from comment #85)
> > Created attachment 8960540 [details] [diff] [review]
> > data migration WIP
> 
> So, in the end I didn't implement a zip archive, I just move
> webappsstore.sqlite to <profile>/storage/ls-archive.sqlite and then lazily
> migrate data from it when it's needed. It works quite well. It's nice that
> SQLite supports transaction across multiple databases, so ensuring that
> stuff is done atomically is quite easy.
> I also believe that we don't have to do a major storage upgrade (only
> minor), so it won't affect users switching between release/beta/nightly.
> I'll attach a new patch once I finish final cleanup.
> 
> I also have a patch that removes old local storage implementation, just to
> be sure that everything uses new implementation.

any performance implicates either positive or negative with this change in strategy?

	Jan Varga [:janv] Assignee
	Comment 93 • 6 years ago

(In reply to Marion Daly [:mdaly] from comment #92)
> (In reply to Jan Varga [:janv] from comment #91)
> > (In reply to Jan Varga [:janv] from comment #85)
> > > Created attachment 8960540 [details] [diff] [review]
> > > data migration WIP
> > 
> > So, in the end I didn't implement a zip archive, I just move
> > webappsstore.sqlite to <profile>/storage/ls-archive.sqlite and then lazily
> > migrate data from it when it's needed. It works quite well. It's nice that
> > SQLite supports transaction across multiple databases, so ensuring that
> > stuff is done atomically is quite easy.
> > I also believe that we don't have to do a major storage upgrade (only
> > minor), so it won't affect users switching between release/beta/nightly.
> > I'll attach a new patch once I finish final cleanup.
> > 
> > I also have a patch that removes old local storage implementation, just to
> > be sure that everything uses new implementation.
> 
> any performance implicates either positive or negative with this change in
> strategy?

The performance is much better since the old database is just moved during a minor
storage upgrade (done by quota manager after startup).
Then, during a preload, old data is accessed using an index and inserted into new
per origin database.

The downside of this approach is that the old database is kept around uncompressed.
My multi year app profile contains a 125 MB database with roughly 3000 unique origins.

So, performance should be much better overall, we just keep a bit more data in user's
profile.

I have some ideas what to do with the old database long term. For example, we can try
to shrink it by vacuuming when the app is idle or just remove the whole file if it's
not touched for 6 months (or so).

	Andrew Sutherland [:asuth] (he/him)
	Comment 94 • 6 years ago

Per https://bugzilla.mozilla.org/show_bug.cgi?id=1373244#c30 it sounds like we're not going to have profile-per-channel in time for the landing of this, so we do need to be more concerned about "profile from the future" than I was hoping.  Setting needinfo on myself to explicitly address the migration logic in the context of this.

:janv, please feel free to chime in with any thoughts too.

And for context for others, our general plan for situations like this has been :bkelly's idea to do the following when we encounter "from the future" things:
- Invoke a hook that can be used by system extensions to attempt to handle the data from the future and make it work in the current, downgraded version.
- If there was no hook or the hook failed/gave up, wipe the data in question.

In the context of QuotaManager we'd expect the above to happen at a few possible granularity levels: profile-wide, origin-wide, QM-client-for-an-origin.


A general issue is that in an ideal world, we'd need the hook mechanism and wipe mechanism in place for at least one release prior to landing anything that would make things look like they're from the future.  Although there's always uplifts...

	Jan Varga [:janv] Assignee
	Comment 95 • 6 years ago

Andrew, hopefully we won't have this problem with new local storage since I think a *minor* storage upgrade should be enough.
See comment 91.

	Marion Daly [:mdaly]
	Comment 96 • 6 years ago

Would this be prefed on or off in FF 61?

	Jan Varga [:janv] Assignee
	Comment 97 • 6 years ago

So I found performance regression on linux tp6_google. Obviously google.com uses LocalStorage and tp6_google on linux is very low number (the page loads very fast compared to other platforms), so any additional overhead is noticeable.
I already optimized PBackgroundIDBDatabase creation, now there's only once actor per origin and per process (there use to be one per DOM global) and various other small optimizations. However, it seems it's still not enough.
Originally I wanted to do another optimization in a follow up bug, but due to the tp6_google regression on linux I changed my mind. I want to do what Andrew described in comment 8

> ... consider coalescing the writes locally by tracking
> dirty keys and flushing on a timer, much like we do in the database thread
> already.

I'll try to do a basic (lightweight) version of this, I just want to create a small per origin and content process cache that coalesces setItem/removeItem calls and flush changes to the parent process after let's say 100ms.

	Jan Varga [:janv] Assignee
	Comment 98 • 6 years ago

(In reply to Marion Daly [:mdaly] from comment #96)
> Would this be prefed on or off in FF 61?

We have roughly 5 weeks until m-c becomes FF 62, I think it's still feasible to have it enabled in FF 61.

	Marion Daly [:mdaly]
	Comment 99 • 6 years ago

Status?

	Jan Varga [:janv] Assignee
	Comment 100 • 6 years ago

(In reply to Marion Daly [:mdaly] from comment #99)
> Status?

I implemented the micro cache on the content side and I also verified that all tests pass cross platform:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d9412df3564c5db60e41d184090bb314c65bb288

I also tested performance and it looks good:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=73348ff8d0338cc4261b2575e4891b39997a759d
https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&newProject=try&newRevision=73348ff8d0338cc4261b2575e4891b39997a759d&framework=1&selectedTimeRange=172800

However, the micro cache currently supports only "full snapshots", that is that all data is sent to the content process.
I plan to implement lazy loading of data for micro cache if data is over 4K.
So that should be the last patch.

I'm currently trying to solve a shutdown crash related to quota manager, once I fix it, I'll get back to the micro cache.

	Marion Daly [:mdaly]
	Comment 101 • 6 years ago

:jan will this be a pref?

	Jan Varga [:janv] Assignee
	Comment 102 • 6 years ago

(In reply to Marion Daly [:mdaly] from comment #101)
> :jan will this be a pref?

Yeah, I'm leaning towards to have a pref for this, but I have to investigate what to do with migrated data. We will need a new patch for that.

	Marion Daly [:mdaly]
	Comment 103 • 6 years ago

Can you create a bug for the pref then and add it as a dependency?

	Jan Varga [:janv] Assignee
	Comment 104 • 6 years ago

(In reply to Marion Daly [:mdaly] from comment #103)
> Can you create a bug for the pref then and add it as a dependency?

Existing patches for new implementation depend on the fact that the old implementation is replaced with the new one. So I'm reworking stuff here in this bug to have a pref for new LS implementation.

	Marion Daly [:mdaly]
	Comment 105 • 6 years ago

status?

	Jan Varga [:janv] Assignee
	Comment 106 • 6 years ago

(In reply to Marion Daly [:mdaly] from comment #105)
> status?

Finishing work on the shadow database writing, so the new LS implementation will write data changes to new per origin databases and also to the old one (webappstore.sqlite).

	Jan Varga [:janv] Assignee
	Comment 107 • 6 years ago

r=mccr8

	Jan Varga [:janv] Assignee
	Comment 124 • 6 years ago

r=baku

	Jan Varga [:janv] Assignee
	Comment 127 • 6 years ago

r=bholley

	Marion Daly [:mdaly]
	Comment 132 • 6 years ago

Timeframe for bringing into nightly?

	Jan Varga [:janv] Assignee
	Comment 133 • 6 years ago

(In reply to Marion Daly [:mdaly] from comment #132)
> Timeframe for bringing into nightly?

Given the fact that we can land it preffed off, I think 2 weeks is realistic timeframe.