Bug 225065 – Don't keep local storage data in memory in the NetworkProcess

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Chris Dumez]

Don't keep local storage data in memory in the NetworkProcess. Instead, just rely on the existing SQLiteDatabase.

[Chris Dumez]

Created attachment 427070 [details]
WIP Patch

[Chris Dumez]

Created attachment 427102 [details]
Patch

[Kimmo Kinnunen]

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 )

[Chris Dumez]

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.

[Chris Dumez]

Created attachment 427151 [details]
Patch

[Chris Dumez]

(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.

[Alex Christensen]

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?

[Chris Dumez]

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.

[Chris Dumez]

Created attachment 427164 [details]
Patch

[Chris Dumez]

(In reply to Chris Dumez from comment #9)
> Created attachment 427164 [details]
> Patch

Added more threading assertions.

[EWS]

Tools/Scripts/svn-apply failed to apply attachment 427164 [details] to trunk.
Please resolve the conflicts and upload a new patch.

[Chris Dumez]

Created attachment 427177 [details]
Patch

[Chris Dumez]

Created attachment 427178 [details]
Patch

[Chris Dumez]

Comment on attachment 427178 [details]
Patch

Clearing flags on attachment: 427178

Committed r276653 (237080@main): <https://commits.webkit.org/237080@main>

[Chris Dumez]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/77222667>