Bug 225065 - Don't keep local storage data in memory in the NetworkProcess
Summary: Don't keep local storage data in memory in the NetworkProcess
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-04-26 11:41 PDT by Chris Dumez
Modified: 2021-06-11 12:08 PDT (History)
8 users (show)

See Also:


Attachments
WIP Patch (32.86 KB, patch)
2021-04-26 11:41 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (40.41 KB, patch)
2021-04-26 16:31 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (39.82 KB, patch)
2021-04-27 07:55 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (40.48 KB, patch)
2021-04-27 10:39 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (40.66 KB, patch)
2021-04-27 11:33 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (40.75 KB, patch)
2021-04-27 11:48 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2021-04-26 11:41:05 PDT
Don't keep local storage data in memory in the NetworkProcess. Instead, just rely on the existing SQLiteDatabase.
Comment 1 Chris Dumez 2021-04-26 11:41:59 PDT
Created attachment 427070 [details]
WIP Patch
Comment 2 Chris Dumez 2021-04-26 16:31:25 PDT
Created attachment 427102 [details]
Patch
Comment 3 Kimmo Kinnunen 2021-04-27 00:23:20 PDT
Comment on attachment 427102 [details]
Patch

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

> Source/WebCore/storage/StorageMap.cpp:187
> +    return nullptr;

would it make sense to return "this" here, so you wouldn't need the conditional assignment in the call site?
Personally "clear()" is a bit confusing wrt the purpose. Maybe would call this "duplicateEmpty()" or "createEmptyDuplicate()"

> Source/WebKit/NetworkProcess/WebStorage/LocalStorageDatabase.cpp:240
> +        return query->getColumnBlobAsString(0);

I think the error will be logged in this case.
If I understand correctly, this is not an error case?

> Source/WebKit/NetworkProcess/WebStorage/LocalStorageDatabase.cpp:331
> +    auto countStatement = makeUnique<SQLiteStatement>(m_database, "SELECT COUNT(*) FROM ItemTable"_s);

since this does not use scopedStatement, I don't think it needs to be allocated from heap? (Seeing it makes me think it escapes and then leads to investigation about why doesn't it escape)

> Source/WebKit/NetworkProcess/WebStorage/LocalStorageDatabase.cpp:361
> +        statement = makeUnique<SQLiteStatement>(m_database, query);

just some thoughts about the general code, not related to this patch.
I'd hope:
 ASCIILiteral query
 Optional<SQLitePreparedStatement>& statement
 statement = m_database->prepare(query);
E.g. using SQLite statements wouldn't create nor store Strings since sqlite anyway operates on utf-8 cstrings. The queries are often literals)
the statement could be allocated inline in LocalStorageDatabase via Optional if it was not so big (storing the query and the database )
Comment 4 Chris Dumez 2021-04-27 07:48:27 PDT
Comment on attachment 427102 [details]
Patch

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

>> Source/WebCore/storage/StorageMap.cpp:187
>> +    return nullptr;
> 
> would it make sense to return "this" here, so you wouldn't need the conditional assignment in the call site?
> Personally "clear()" is a bit confusing wrt the purpose. Maybe would call this "duplicateEmpty()" or "createEmptyDuplicate()"

This is the whole pattern in this file. Every function on StorageMap behaves this way. While I don't mind refactoring, I don't think this should be done in this patch. I like the idea of return |this|. I don't like the new naming proposal. This is a clear() function, that just happens to copy on write if there is more than one user.

>> Source/WebKit/NetworkProcess/WebStorage/LocalStorageDatabase.cpp:240
>> +        return query->getColumnBlobAsString(0);
> 
> I think the error will be logged in this case.
> If I understand correctly, this is not an error case?

Yes, looks like I messed up my error checking. I'll fix.

>> Source/WebKit/NetworkProcess/WebStorage/LocalStorageDatabase.cpp:331
>> +    auto countStatement = makeUnique<SQLiteStatement>(m_database, "SELECT COUNT(*) FROM ItemTable"_s);
> 
> since this does not use scopedStatement, I don't think it needs to be allocated from heap? (Seeing it makes me think it escapes and then leads to investigation about why doesn't it escape)

My bad. I initially used ScopedStatement then changed my mind and failed to restore the original code properly.
Comment 5 Chris Dumez 2021-04-27 07:55:48 PDT
Created attachment 427151 [details]
Patch
Comment 6 Chris Dumez 2021-04-27 07:59:43 PDT
(In reply to Chris Dumez from comment #4)
> Comment on attachment 427102 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=427102&action=review
> 
> >> Source/WebCore/storage/StorageMap.cpp:187
> >> +    return nullptr;
> > 
> > would it make sense to return "this" here, so you wouldn't need the conditional assignment in the call site?
> > Personally "clear()" is a bit confusing wrt the purpose. Maybe would call this "duplicateEmpty()" or "createEmptyDuplicate()"
> 
> This is the whole pattern in this file. Every function on StorageMap behaves
> this way. While I don't mind refactoring, I don't think this should be done
> in this patch. I like the idea of return |this|. I don't like the new naming
> proposal. This is a clear() function, that just happens to copy on write if
> there is more than one user.

I will look into refactoring this in a separate patch. I think ideally, these functions wouldn't have to return anything and the copy-on-write behavior would be 100% internal to the StorageMap class.
Comment 7 Alex Christensen 2021-04-27 10:24:01 PDT
Comment on attachment 427151 [details]
Patch

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

> Source/WebKit/ChangeLog:12
> +        Both the NetworkProcess would keep the entries in memory, in a

NetworkProcess and WebProcess

> Source/WebKit/ChangeLog:53
> +        LocalStorageDatabase no longer has a queue on pending operations.

of

> Source/WebKit/NetworkProcess/WebStorage/LocalStorageDatabase.cpp:53
> +    entrySize += key.length() * sizeof(UChar);

Do you think it would be worth checking if it's an 8 bit string and using sizeof(LChar) here?  It's also O(1) and gets a better estimate.
I guess this is used for estimating the size of writing to the database.  Does the database always store UChars?

> Source/WebKit/NetworkProcess/WebStorage/LocalStorageDatabase.cpp:166
> +HashMap<String, String> LocalStorageDatabase::items() const

Do we want to assert that we're not on the main run loop in these functions, since they do disk operations?

> Source/WebKit/NetworkProcess/WebStorage/LocalStorageDatabase.cpp:244
> +void LocalStorageDatabase::setItem(const String& key, const String& value, String& oldValue, bool& quotaException)

It would be nice if we could do this without out params.

> Source/WebKit/NetworkProcess/WebStorage/StorageArea.cpp:152
> -const HashMap<String, String>& StorageArea::items() const
> +HashMap<String, String> StorageArea::items() const

This copies the whole map.  Does it need to?

> Source/WebKit/NetworkProcess/WebStorage/StorageManagerSet.cpp:150
> +    m_queue->dispatch([&semaphore] {

dispatchSync?
Comment 8 Chris Dumez 2021-04-27 10:32:15 PDT
Comment on attachment 427151 [details]
Patch

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

>> Source/WebKit/NetworkProcess/WebStorage/LocalStorageDatabase.cpp:53
>> +    entrySize += key.length() * sizeof(UChar);
> 
> Do you think it would be worth checking if it's an 8 bit string and using sizeof(LChar) here?  It's also O(1) and gets a better estimate.
> I guess this is used for estimating the size of writing to the database.  Does the database always store UChars?

I matched the logic in StorageMap for now for size estimation. It may be worth improving but maybe I can do this separately since I'd need to touch StorageMap too.

>> Source/WebKit/NetworkProcess/WebStorage/LocalStorageDatabase.cpp:166
>> +HashMap<String, String> LocalStorageDatabase::items() const
> 
> Do we want to assert that we're not on the main run loop in these functions, since they do disk operations?

Sure, I will add more assertions.

>> Source/WebKit/NetworkProcess/WebStorage/LocalStorageDatabase.cpp:244
>> +void LocalStorageDatabase::setItem(const String& key, const String& value, String& oldValue, bool& quotaException)
> 
> It would be nice if we could do this without out params.

Yes. Again, I am trying to match the StorageMap API though since one is used for session storage and the other for local storage and it makes the call sites nicer if we're consistent.
I'd be happy to return a boolean instead but I'd want to do the same for StorageMap for consistency. This means I likely want Bug 225108 to land first.

>> Source/WebKit/NetworkProcess/WebStorage/StorageArea.cpp:152
>> +HashMap<String, String> StorageArea::items() const
> 
> This copies the whole map.  Does it need to?

In the ephemeral case, yes, it does a copy.

In the non-ephemeral / database case, we construct the HashMap on the fly and don't store it so I cannot return a const ref. I'll give it some thought but I am not sure how to avoid that.

>> Source/WebKit/NetworkProcess/WebStorage/StorageManagerSet.cpp:150
>> +    m_queue->dispatch([&semaphore] {
> 
> dispatchSync?

See r274363. The issue is that dispatchSync() on cocoa uses GCD's dispatch_sync() which may not run the code on the destination thread. While dispatch_sync() does this in a thread-safe manner, it trips our internal threading assertions here.
Comment 9 Chris Dumez 2021-04-27 10:39:46 PDT
Created attachment 427164 [details]
Patch
Comment 10 Chris Dumez 2021-04-27 10:39:57 PDT
(In reply to Chris Dumez from comment #9)
> Created attachment 427164 [details]
> Patch

Added more threading assertions.
Comment 11 EWS 2021-04-27 11:27:26 PDT
Tools/Scripts/svn-apply failed to apply attachment 427164 [details] to trunk.
Please resolve the conflicts and upload a new patch.
Comment 12 Chris Dumez 2021-04-27 11:33:42 PDT
Created attachment 427177 [details]
Patch
Comment 13 Chris Dumez 2021-04-27 11:48:07 PDT
Created attachment 427178 [details]
Patch
Comment 14 Chris Dumez 2021-04-27 12:03:05 PDT
Comment on attachment 427178 [details]
Patch

Clearing flags on attachment: 427178

Committed r276653 (237080@main): <https://commits.webkit.org/237080@main>
Comment 15 Chris Dumez 2021-04-27 12:03:09 PDT
All reviewed patches have been landed.  Closing bug.
Comment 16 Radar WebKit Bug Importer 2021-04-27 12:04:24 PDT
<rdar://problem/77222667>