Bug 186104 - Stop using StorageTracker.db in LocalStorageDatabaseTracker
Summary: Stop using StorageTracker.db in LocalStorageDatabaseTracker
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit API (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sihui Liu
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-05-30 11:31 PDT by Sihui Liu
Modified: 2018-06-01 13:24 PDT (History)
6 users (show)

See Also:


Attachments
Patch (21.96 KB, patch)
2018-05-30 11:49 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff
Patch (22.29 KB, patch)
2018-05-30 13:24 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff
Patch (22.25 KB, patch)
2018-05-30 13:31 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff
Patch (22.68 KB, patch)
2018-05-30 16:52 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sihui Liu 2018-05-30 11:31:27 PDT
StorageTracker.db acts as an cache for searching files in LocalStorage directory. There are a few reasons we may not want StorageTracker.db any more:
1. We could compute file name from origin, so as long as we have origin information(m_origins), we don't need StorageTracker.db for mapping from origin to file path. This help us simplify the code.
2. Maintaining(open/delete/update) a database could be expensive. 
3. Currently StorageTracker.db stores full paths, which causes problems like not deleting correct files after restoring from iCloud backup(<rdar://problem/31744826>).
Comment 1 Sihui Liu 2018-05-30 11:49:39 PDT
Created attachment 341588 [details]
Patch
Comment 2 Chris Dumez 2018-05-30 12:19:58 PDT
Comment on attachment 341588 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=341588&action=review

> Source/WebKit/UIProcess/WebStorage/LocalStorageDatabaseTracker.cpp:63
> +    SQLiteFileSystem::deleteDatabaseFile(databasePath("StorageTracker.db"));

Shouldn't we do this in the dispatch call above so that we do I/O on the background thread instead of the main thread?

> Source/WebKit/UIProcess/WebStorage/LocalStorageDatabaseTracker.cpp:87
> +    if (path.isEmpty())

if (!path.isEmpty())
    SQLiteFileSystem::deleteDatabaseFile(path);

since we'd want to notify the clients even if the path was empty I would  assume?

> Source/WebKit/UIProcess/WebStorage/LocalStorageDatabaseTracker.cpp:98
> +    Vector<String> paths = FileSystem::listDirectory(m_localStorageDirectory, "*.localstorage");

auto paths

> Source/WebKit/UIProcess/WebStorage/LocalStorageDatabaseTracker.cpp:111
> +    importOrigins();

Looks like this unconditionally re-imports all origins from disk, even though they may have already been imported. This seems bad.

> Source/WebKit/UIProcess/WebStorage/LocalStorageDatabaseTracker.cpp:131
> +        origins.append(origin);

This should have been uncheckedAppend() since you reserveInitialCapacity() beforehand. However, in this case, we should rely on pre-existing copyToVector() instead of duplicating the logic for copying a container into a vector.

> Source/WebKit/UIProcess/WebStorage/LocalStorageDatabaseTracker.cpp:171
> +    m_origins.clear();

What if origins were already imported?

> Source/WebKit/UIProcess/WebStorage/LocalStorageDatabaseTracker.cpp:174
>          if (!path.endsWith(".localstorage"))

Why is this needed? Maybe this could be an assertion?

> Source/WebKit/UIProcess/WebStorage/LocalStorageDatabaseTracker.h:60
> +        double creationTime;

We try to avoid storing timestamps as double nowadays. This and the one below should probably be WallTime instead of double.
Comment 3 Sihui Liu 2018-05-30 13:24:02 PDT
Created attachment 341597 [details]
Patch
Comment 4 Chris Dumez 2018-05-30 13:31:03 PDT
Comment on attachment 341597 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=341597&action=review

> Source/WebKit/UIProcess/WebStorage/LocalStorageDatabaseTracker.cpp:63
> +    SQLiteFileSystem::deleteDatabaseFile(databasePath("StorageTracker.db"));

Why are we still doing this on the main thread?

> Source/WebKit/UIProcess/WebStorage/LocalStorageDatabaseTracker.cpp:110
> +    importOrigins();

Why is this OK? This will reimport all origins from disk, even though they have likely already been imported.

> Source/WebKit/UIProcess/WebStorage/LocalStorageDatabaseTracker.cpp:166
>          if (!path.endsWith(".localstorage"))

Why is this still here? Can this be made an assertion?

> Source/WebKit/UIProcess/WebStorage/LocalStorageDatabaseTracker.cpp:172
> +        if (origin)

Should we add a RELEASE_LOG_ERROR() for the case where it fails? It seems like we'd want to know if we fail to import origins from the disk.
Comment 5 Sihui Liu 2018-05-30 13:31:30 PDT
Created attachment 341599 [details]
Patch
Comment 6 Chris Dumez 2018-05-30 13:31:43 PDT
When you re-upload a patch please address ALL comments, either in the patch or via a comment explaining why you chose not to.
Comment 7 Sihui Liu 2018-05-30 13:39:19 PDT
(In reply to Chris Dumez from comment #4)
> Comment on attachment 341597 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=341597&action=review
> 
> > Source/WebKit/UIProcess/WebStorage/LocalStorageDatabaseTracker.cpp:63
> > +    SQLiteFileSystem::deleteDatabaseFile(databasePath("StorageTracker.db"));
> 
> Why are we still doing this on the main thread?
> 
Reload and fix.

> > Source/WebKit/UIProcess/WebStorage/LocalStorageDatabaseTracker.cpp:110
> > +    importOrigins();
> 
> Why is this OK? This will reimport all origins from disk, even though they
> have likely already been imported.
> 
See <rdar://problem/22781730>.

> > Source/WebKit/UIProcess/WebStorage/LocalStorageDatabaseTracker.cpp:166
> >          if (!path.endsWith(".localstorage"))
> 
> Why is this still here? Can this be made an assertion?
> 
Database file is accompanied by files "-shm" and "-wal"; e.g. "xxx.localstorage" exists with "xxx.localstorage-shm" and "xxx.localstorage-wal".
Comment 8 Geoffrey Garen 2018-05-30 13:46:50 PDT
Comment on attachment 341599 [details]
Patch

Did you measure the time it takes to load StorageTracker.db vs list the directory? What do those timings look like?

If StorageTracker.db is faster than listing the directory, then we need some other plan for solving the launch-time performance issue cased by LocalStorageDatabaseTracker::LocalStorageDatabaseTracker.
Comment 9 Chris Dumez 2018-05-30 13:52:41 PDT
(In reply to Sihui Liu from comment #7)
> (In reply to Chris Dumez from comment #4)
> > Comment on attachment 341597 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=341597&action=review
> > 
> > > Source/WebKit/UIProcess/WebStorage/LocalStorageDatabaseTracker.cpp:63
> > > +    SQLiteFileSystem::deleteDatabaseFile(databasePath("StorageTracker.db"));
> > 
> > Why are we still doing this on the main thread?
> > 
> Reload and fix.
> 
> > > Source/WebKit/UIProcess/WebStorage/LocalStorageDatabaseTracker.cpp:110
> > > +    importOrigins();
> > 
> > Why is this OK? This will reimport all origins from disk, even though they
> > have likely already been imported.
> > 
> See <rdar://problem/22781730>.

I have looked at this radar and am still not unclear why we'd want to force a reimport. Please clarify.
Either we could add a flag to indicate that we've already done the import, or for some reason we do not trust our in-memory data. If we do not trust our in-memory data for this operation, then why are we trusting it for other operations?

> > > Source/WebKit/UIProcess/WebStorage/LocalStorageDatabaseTracker.cpp:166
> > >          if (!path.endsWith(".localstorage"))
> > 
> > Why is this still here? Can this be made an assertion?
> > 
> Database file is accompanied by files "-shm" and "-wal"; e.g.
> "xxx.localstorage" exists with "xxx.localstorage-shm" and
> "xxx.localstorage-wal".

We are asking for "*.localstorage", not "*.localstorage*" so I would not expect such files to be returned. Are they?
Comment 10 Chris Dumez 2018-05-30 14:48:30 PDT
Comment on attachment 341599 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=341599&action=review

> Source/WebCore/ChangeLog:3
> +        Stop using StorageTracker.db in LocalStroageDatabaseTracker

typo: LocalStroageDatabaseTracker

> Source/WebKit/ChangeLog:3
> +        Stop using StorageTracker.db in LocalStroageDatabaseTracker

typo: LocalStroageDatabaseTracker
Comment 11 Sihui Liu 2018-05-30 16:51:51 PDT
(In reply to Chris Dumez from comment #9)
> (In reply to Sihui Liu from comment #7)
> > (In reply to Chris Dumez from comment #4)
> > > Comment on attachment 341597 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=341597&action=review
> > > 
> > > > Source/WebKit/UIProcess/WebStorage/LocalStorageDatabaseTracker.cpp:63
> > > > +    SQLiteFileSystem::deleteDatabaseFile(databasePath("StorageTracker.db"));
> > > 
> > > Why are we still doing this on the main thread?
> > > 
> > Reload and fix.
> > 
> > > > Source/WebKit/UIProcess/WebStorage/LocalStorageDatabaseTracker.cpp:110
> > > > +    importOrigins();
> > > 
> > > Why is this OK? This will reimport all origins from disk, even though they
> > > have likely already been imported.
> > > 
> > See <rdar://problem/22781730>.
> 
> I have looked at this radar and am still not unclear why we'd want to force
> a reimport. Please clarify.
> Either we could add a flag to indicate that we've already done the import,
> or for some reason we do not trust our in-memory data. If we do not trust
> our in-memory data for this operation, then why are we trusting it for other
> operations?
> 
In this bug two UI processes are modifying the same directory, which actually is not an expected use case of WebKit.

In the new patch I removed m_origins which might resolve the confusion and the bug.

> > > > Source/WebKit/UIProcess/WebStorage/LocalStorageDatabaseTracker.cpp:166
> > > >          if (!path.endsWith(".localstorage"))
> > > 
> > > Why is this still here? Can this be made an assertion?
> > > 
> > Database file is accompanied by files "-shm" and "-wal"; e.g.
> > "xxx.localstorage" exists with "xxx.localstorage-shm" and
> > "xxx.localstorage-wal".
> 
> We are asking for "*.localstorage", not "*.localstorage*" so I would not
> expect such files to be returned. Are they?
Yes! I've tried, removed the redundant code.
Comment 12 Sihui Liu 2018-05-30 16:52:42 PDT
Created attachment 341624 [details]
Patch
Comment 13 Geoffrey Garen 2018-05-30 17:03:58 PDT
Comment on attachment 341624 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=341624&action=review

> Source/WebKit/ChangeLog:8
> +        Stop using StorageTracker.db and stop caching origins in LocalStorageDatabaseTracker for efficiency 

The specific efficiency we're trying to achieve is an improvement in launch time, especially on computers with spinning disks and large local storage collections.
Comment 14 Geoffrey Garen 2018-06-01 12:45:36 PDT
Comment on attachment 341624 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=341624&action=review

r=me

> Source/WebCore/platform/sql/SQLiteFileSystem.h:60
> +    WEBCORE_EXPORT static String appendDatabaseFileNameToPath(const String& path, const String& fileName);

This is something that's nice to explain in your ChangeLog. What you've done here is to call SQLiteFileSystem functions in cases where we used to call FileSystem functions because SQLiteFileSystem is logically more correct.
Comment 15 Chris Dumez 2018-06-01 12:48:03 PDT
Comment on attachment 341624 [details]
Patch

So we're not worried about the performance impact of not having the m_origins cache now and going to disk every time we need the origins?
Comment 16 Chris Dumez 2018-06-01 12:49:04 PDT
(In reply to Chris Dumez from comment #9)
> (In reply to Sihui Liu from comment #7)
> > (In reply to Chris Dumez from comment #4)
> > > Comment on attachment 341597 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=341597&action=review
> > > 
> > > > Source/WebKit/UIProcess/WebStorage/LocalStorageDatabaseTracker.cpp:63
> > > > +    SQLiteFileSystem::deleteDatabaseFile(databasePath("StorageTracker.db"));
> > > 
> > > Why are we still doing this on the main thread?
> > > 
> > Reload and fix.
> > 
> > > > Source/WebKit/UIProcess/WebStorage/LocalStorageDatabaseTracker.cpp:110
> > > > +    importOrigins();
> > > 
> > > Why is this OK? This will reimport all origins from disk, even though they
> > > have likely already been imported.
> > > 
> > See <rdar://problem/22781730>.

This radar is closed, did you point me to the right radar?
Comment 17 Geoffrey Garen 2018-06-01 12:56:21 PDT
> So we're not worried about the performance impact of not having the
> m_origins cache now and going to disk every time we need the origins?

We did some debugging and it looks like we only make use of m_origins when you list website data in the Safari preferences window. Other storage technologies do not appear to cache their lists, so there's little incremental benefit to LocalStorage caching its list. Either way, you have a few seconds of async delay before the list populates.

If we do see more frequent use of this API, I think we'll want to add back the m_origins cache.

(Note that there is an open discussion about what should happen when multiple processes point at the same data store path. If we wanted to revive the m_origins cache, we would need to resolve that discussion and either (a) explain how the cache can be coherent across multiple UI processes or (b) decide that multiple UI processes with concurrent access to the same data store path is not supported.)
Comment 18 Geoffrey Garen 2018-06-01 12:58:35 PDT
> > > > Why is this OK? This will reimport all origins from disk, even though they
> > > > have likely already been imported.
> > > > 
> > > See <rdar://problem/22781730>.
> 
> This radar is closed, did you point me to the right radar?

This Radar explains the original intention behind re-importing all origins, namely: working around a bug that can happen when multiple UI processes use the same data store path.

FWIW, the latest patch removes the extra import in favor of just not caching at all, since we weren't able to demonstrate a benefit to caching.

<rdar://problem/40665000> is the ongoing discussion about whether we should support concurrent access to a data store path or not.
Comment 19 WebKit Commit Bot 2018-06-01 13:13:14 PDT
Comment on attachment 341624 [details]
Patch

Clearing flags on attachment: 341624

Committed r232410: <https://trac.webkit.org/changeset/232410>
Comment 20 WebKit Commit Bot 2018-06-01 13:13:16 PDT
All reviewed patches have been landed.  Closing bug.
Comment 21 Radar WebKit Bug Importer 2018-06-01 13:24:29 PDT
<rdar://problem/40732090>