Improve accuracy of IndexedDB estimated write size computation

[Ben Nham]

Improve accuracy of IndexedDB estimated write size computation

[Ben Nham]

Created attachment 398332 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/62816407>

[Ben Nham]

Created attachment 398334 [details]
Patch

[Sihui Liu]

Comment on attachment 398334 [details]
Patch

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

The patch looks sane to me.

> Source/WebCore/Modules/indexeddb/server/UniqueIDBDatabase.cpp:82
> +        size += (12 + primaryKeySize + estimateSize(indexKey.asOneKey()));

If index is multientry, we can have multiple indexrecords for one record. (https://www.w3.org/TR/IndexedDB-2/#index-construct). We can use that for more accurate estimate.

> Tools/ChangeLog:25
> +        I added a test to make sure the quota check produces a sane size when using a large number
> +        of indices.

This changelog is for the test, so propbably these two lines are enough.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/IDBQuotaCheck.mm:127
> +#if PLATFORM(MAC)
> +        [webView.get().window setIsVisible:YES];
> +#else
> +        webView.get().window.hidden = NO;
> +#endif

Layout test seems enough for this patch, like LayoutTests/storage/indexeddb/storage-limit.html. 
Layout test storage limit is low by default, you can create objectstore with multiple indexes and verify that add operation would fail.

[youenn fablet]

Comment on attachment 398334 [details]
Patch

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

> Source/WebCore/Modules/indexeddb/server/MemoryIDBBackingStore.cpp:618
> +    return m_serializationContext.get();

This seems somehow redundant with MemoryObjectStore::m_serializationContext, not sure if there is a better way though.

> Source/WebCore/Modules/indexeddb/server/MemoryObjectStore.cpp:255
> +    IndexIDToIndexKeyMap indexKeys = generateIndexKeyMapForValue(m_serializationContext->execState(), m_info, keyData, value);

auto

> Source/WebCore/Modules/indexeddb/server/MemoryObjectStore.cpp:316
> +            error = IDBError { };

This assignment probably does nothing.
Do we want to return early or enter the error path below?
Should we add ASSERT(index)?

> Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.cpp:1858
> +        auto indexIt = indexMap.find(indexID);

s/indexIt/indexIterator

> Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.cpp:1860
> +            error = IDBError { };

Ditto here, I think error is still null so we could just change it to return { }.
Do we want to make it as if the index record failed and do the clean-up step below?
Ditto for ASSERT?

> Source/WebCore/Modules/indexeddb/server/UniqueIDBDatabase.cpp:764
>      if (overwriteMode == IndexedDB::ObjectStoreOverwriteMode::NoOverwrite) {

Can we move the quota check after this if 'check'?

[Ben Nham]

Created attachment 398434 [details]
Patch

[Ben Nham]

Created attachment 398454 [details]
Patch

[Ben Nham]

Comment on attachment 398454 [details]
Patch

Addressed patch feedback:
 - Fixed estimates for multi-entry indices
 - Added layout test (should I delete the API test?)
 - Added bailout if indices can't be found (this shouldn't happen so it is also protected by an assert)
 - Moved check farther down

[Sihui Liu]

(In reply to Ben Nham from comment #8)
> Comment on attachment 398454 [details]
> Patch
> 
> Addressed patch feedback:
>  - Fixed estimates for multi-entry indices
>  - Added layout test (should I delete the API test?)
Yeah, I think layout test is enough.

>  - Added bailout if indices can't be found (this shouldn't happen so it is
> also protected by an assert)
>  - Moved check farther down

[Ben Nham]

Created attachment 398766 [details]
Patch

[Ben Nham]

Comment on attachment 398766 [details]
Patch

- Rebase
 - Remove API test

[EWS]

Committed r261533: <https://trac.webkit.org/changeset/261533>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 398766 [details].

[Jim Mason]

For me, r261533 is breaking parallel builds with -j4.  I have opened bug 211943 to track.