New issue
Advanced search Search tips

Issue 713409 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: May 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux
Pri: 1
Type: Bug

Blocked on:
issue 724109

Blocking:
issue 710726
issue 719007
issue 721516


Participants' hotlists:
dmurph-future-tasks
OWP-Storage-WASM

Show other hotlists

Other hotlists containing this issue:
IDB-Scaling


Sign in to add a comment

Wrap large IndexedDB values into Blobs

Project Member Reported by dmu...@chromium.org, Apr 19 2017

Issue description

LevelDB was not designed to handle large values -- in general, values should be smaller than the block size, which we currently set to 32KB.

One way to gracefully handle large values and not stress LevelDB is to wrap the large values in Blobs in Blink. We already support attaching Blobs to values, so the code outside Blink doesn't need to know anything has changed.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Apr 28 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ca74aee3773bb77867cf02d5f81725d10b33e4ab

commit ca74aee3773bb77867cf02d5f81725d10b33e4ab
Author: dmurph <dmurph@chromium.org>
Date: Fri Apr 28 02:09:39 2017

[IndexedDB] Hold referenced to IDB::put blobs in the IDBRequest

Since blobs can die during transit (pending mojo change), we can prevent
this by holding onto the blob handles in the IDBRequest until we get the
response back from the browser.

R=pwnall,jsbell
BUG= 351753 , 710736 , 713409 

Review-Url: https://codereview.chromium.org/2828953002
Cr-Commit-Position: refs/heads/master@{#467836}

[modify] https://crrev.com/ca74aee3773bb77867cf02d5f81725d10b33e4ab/third_party/WebKit/Source/modules/indexeddb/IDBObjectStore.cpp
[modify] https://crrev.com/ca74aee3773bb77867cf02d5f81725d10b33e4ab/third_party/WebKit/Source/modules/indexeddb/IDBRequest.cpp
[modify] https://crrev.com/ca74aee3773bb77867cf02d5f81725d10b33e4ab/third_party/WebKit/Source/modules/indexeddb/IDBRequest.h

Components: Blink>Storage>IndexedDB
Labels: Postmortem-Followup
Owner: pwnall@chromium.org
Blocking: 719007
Project Member

Comment 5 by bugdroid1@chromium.org, May 11 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/55ad6749e01eb6bb63b6fb2f64655a82cf4e7d2c

commit 55ad6749e01eb6bb63b6fb2f64655a82cf4e7d2c
Author: pwnall <pwnall@chromium.org>
Date: Thu May 11 06:34:03 2017

Rename IDBRequest::On* callbacks to IDBRequest::Enqueue*.

The current OnSuccess() / OnError() naming scheme is a bit misleading,
because it suggests that the success and error events are dispatched to
script synchronously. The events are actually queued up, via
ExecutionContext::EnqueueEvent(), in order to support having the
application be suspended in Dev Tools.

This CL fixes up the naming scheme and de-virtualizes some IDBRequest
methods that are not overridden by its only subclass, IDBOpenDBRequest.
This leads the way to more complex changes in event processing needed by
the referenced bug.

BUG= 713409 

Review-Url: https://codereview.chromium.org/2870803002
Cr-Commit-Position: refs/heads/master@{#470844}

[modify] https://crrev.com/55ad6749e01eb6bb63b6fb2f64655a82cf4e7d2c/third_party/WebKit/Source/modules/indexeddb/IDBFactory.cpp
[modify] https://crrev.com/55ad6749e01eb6bb63b6fb2f64655a82cf4e7d2c/third_party/WebKit/Source/modules/indexeddb/IDBOpenDBRequest.cpp
[modify] https://crrev.com/55ad6749e01eb6bb63b6fb2f64655a82cf4e7d2c/third_party/WebKit/Source/modules/indexeddb/IDBOpenDBRequest.h
[modify] https://crrev.com/55ad6749e01eb6bb63b6fb2f64655a82cf4e7d2c/third_party/WebKit/Source/modules/indexeddb/IDBRequest.cpp
[modify] https://crrev.com/55ad6749e01eb6bb63b6fb2f64655a82cf4e7d2c/third_party/WebKit/Source/modules/indexeddb/IDBRequest.h
[modify] https://crrev.com/55ad6749e01eb6bb63b6fb2f64655a82cf4e7d2c/third_party/WebKit/Source/modules/indexeddb/IDBRequestTest.cpp
[modify] https://crrev.com/55ad6749e01eb6bb63b6fb2f64655a82cf4e7d2c/third_party/WebKit/Source/modules/indexeddb/WebIDBCallbacksImpl.cpp

Comment 6 by pwnall@chromium.org, May 11 2017

Description: Show this description

Comment 7 by pwnall@chromium.org, May 11 2017

Labels: -Pri-3 Pri-2
Summary: Wrap large IndexedDB values into Blobs (was: IndexedDB: Auto-blob large values.)

Comment 8 by pwnall@chromium.org, May 11 2017

Blocking: 721516

Comment 9 by pwnall@chromium.org, May 19 2017

Blockedon: 724109
Marking  issue 724109  as a blocker, because the bug in there causes my large CL's tests to fail. I have a CL out for that bug, so it shouldn't hold this work back any longer.
Blocking: 710726
Project Member

Comment 11 by bugdroid1@chromium.org, May 19 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/02934390c13cbefa44cffd91dde26ec1b96fbe3d

commit 02934390c13cbefa44cffd91dde26ec1b96fbe3d
Author: Victor Costan <pwnall@chromium.org>
Date: Fri May 19 19:36:04 2017

Don't null out IDBRequest::transaction_ on early kills.

This makes the internal state easier to reason about for future changes.

BUG= 713409 

Change-Id: I3faac5c0a043c307c9eb7987cf4dfea2f2ef48c6
Reviewed-on: https://chromium-review.googlesource.com/508479
Reviewed-by: Daniel Murphy <dmurph@chromium.org>
Reviewed-by: Joshua Bell <jsbell@chromium.org>
Commit-Queue: Victor Costan <pwnall@chromium.org>
Cr-Commit-Position: refs/heads/master@{#473277}
[modify] https://crrev.com/02934390c13cbefa44cffd91dde26ec1b96fbe3d/third_party/WebKit/Source/modules/indexeddb/IDBRequest.cpp
[modify] https://crrev.com/02934390c13cbefa44cffd91dde26ec1b96fbe3d/third_party/WebKit/Source/modules/indexeddb/IDBTransactionTest.cpp

Project Member

Comment 12 by bugdroid1@chromium.org, May 26 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/9f25693ac827e8ea87699023e640760fd09ef9bb

commit 9f25693ac827e8ea87699023e640760fd09ef9bb
Author: pwnall <pwnall@chromium.org>
Date: Fri May 26 01:59:28 2017

Wrap large IndexedDB values into Blobs before writing to LevelDB.

Gist: IndexedDB values are currently stored in LevelDB according to the
serialization logic in SerializedScriptValue (SSV). After this CL, an
SSV output that exceeds a threshold gets wrapped in a Blob, and LevelDB
stores a reference to the Blob.
"small" v8::Value -> SSV -> IDBValue (SSV output) -> LevelDB
"large" v8::Value -> SSV -> IDBValue (SSV output) -> Blob w/ SSV output
                  -> IDBValue (Blob reference) -> LevelDB

On the write side, the IndexedDB (IDB) value serialization logic is
extracted from IDBObjectStore::put() into a separate class,
IDBValueWrapper. This is mostly to avoid further swelling
IDBObjectStore::put(), which is already rather large. IDBValueWrapper
knows how to turn an IDBValue into a SerializedScriptValue (SSV), and
how to wrap the SSV into a Blob, if necessary.

The read-side changes are more complex due to the following:
1) IDB request results are laziliy deserialized from IDBValue, because
   the SSV deserialization logic is expensive. The deserialization is done
   synchronously, but Blob data can only be fetched asynchronously. So,
   IDB values must be unwrapped from Blobs before the SSV logic is
   invoked.
2) The events for IDB request results must be dispatched in the order in
   which the requests were issued. If an IDB result's SSV requires
   Blob-unwrapping, all the following results must be queued until the
   Blob is unwrapped and the result's event can be delivered.

The queueing is handled by (sigh) a layer of indirection. Previously,
WebIDBCallbacksImpl called straight into IDBRequest::EnqueueResult().
This CL routes all callbacks that can be queued into
IDBRequest::HandleResult() overloads that match the
IDBRequest::EnqueueResult() signatures. When a result needs to be queued
(either due to Blob-unwrapping, or because it arrives after another
queued result), its data is saved in a IDBRequestQueueItem. These queued
results are tracked by a per-transaction IDBRequestQueue.

When a IDBValue contains a Blob reference, the Blob's content is
asynchronously fetched by IDBRequestLoader. Afterwards,
IDBValueUnwrapper turns the Blob data into an IDBValue that can be
de-serialized by the SerializedScriptValue logic.

BUG= 713409 
TBR=haraken

Review-Url: https://codereview.chromium.org/2822453003
Cr-Commit-Position: refs/heads/master@{#474874}

[add] https://crrev.com/9f25693ac827e8ea87699023e640760fd09ef9bb/third_party/WebKit/LayoutTests/external/wpt/IndexedDB/large-nested-cloning.html
[add] https://crrev.com/9f25693ac827e8ea87699023e640760fd09ef9bb/third_party/WebKit/LayoutTests/external/wpt/IndexedDB/large-requests-abort.html
[add] https://crrev.com/9f25693ac827e8ea87699023e640760fd09ef9bb/third_party/WebKit/LayoutTests/external/wpt/IndexedDB/request-event-ordering.html
[modify] https://crrev.com/9f25693ac827e8ea87699023e640760fd09ef9bb/third_party/WebKit/LayoutTests/external/wpt/IndexedDB/support-promises.js
[modify] https://crrev.com/9f25693ac827e8ea87699023e640760fd09ef9bb/third_party/WebKit/Source/modules/BUILD.gn
[modify] https://crrev.com/9f25693ac827e8ea87699023e640760fd09ef9bb/third_party/WebKit/Source/modules/indexeddb/BUILD.gn
[modify] https://crrev.com/9f25693ac827e8ea87699023e640760fd09ef9bb/third_party/WebKit/Source/modules/indexeddb/IDBFactory.cpp
[modify] https://crrev.com/9f25693ac827e8ea87699023e640760fd09ef9bb/third_party/WebKit/Source/modules/indexeddb/IDBObjectStore.cpp
[modify] https://crrev.com/9f25693ac827e8ea87699023e640760fd09ef9bb/third_party/WebKit/Source/modules/indexeddb/IDBOpenDBRequest.cpp
[modify] https://crrev.com/9f25693ac827e8ea87699023e640760fd09ef9bb/third_party/WebKit/Source/modules/indexeddb/IDBOpenDBRequest.h
[modify] https://crrev.com/9f25693ac827e8ea87699023e640760fd09ef9bb/third_party/WebKit/Source/modules/indexeddb/IDBRequest.cpp
[modify] https://crrev.com/9f25693ac827e8ea87699023e640760fd09ef9bb/third_party/WebKit/Source/modules/indexeddb/IDBRequest.h
[add] https://crrev.com/9f25693ac827e8ea87699023e640760fd09ef9bb/third_party/WebKit/Source/modules/indexeddb/IDBRequestLoader.cpp
[add] https://crrev.com/9f25693ac827e8ea87699023e640760fd09ef9bb/third_party/WebKit/Source/modules/indexeddb/IDBRequestLoader.h
[add] https://crrev.com/9f25693ac827e8ea87699023e640760fd09ef9bb/third_party/WebKit/Source/modules/indexeddb/IDBRequestQueueItem.cpp
[add] https://crrev.com/9f25693ac827e8ea87699023e640760fd09ef9bb/third_party/WebKit/Source/modules/indexeddb/IDBRequestQueueItem.h
[modify] https://crrev.com/9f25693ac827e8ea87699023e640760fd09ef9bb/third_party/WebKit/Source/modules/indexeddb/IDBRequestTest.cpp
[modify] https://crrev.com/9f25693ac827e8ea87699023e640760fd09ef9bb/third_party/WebKit/Source/modules/indexeddb/IDBTransaction.cpp
[modify] https://crrev.com/9f25693ac827e8ea87699023e640760fd09ef9bb/third_party/WebKit/Source/modules/indexeddb/IDBTransaction.h
[modify] https://crrev.com/9f25693ac827e8ea87699023e640760fd09ef9bb/third_party/WebKit/Source/modules/indexeddb/IDBTransactionTest.cpp
[modify] https://crrev.com/9f25693ac827e8ea87699023e640760fd09ef9bb/third_party/WebKit/Source/modules/indexeddb/IDBValue.cpp
[modify] https://crrev.com/9f25693ac827e8ea87699023e640760fd09ef9bb/third_party/WebKit/Source/modules/indexeddb/IDBValue.h
[add] https://crrev.com/9f25693ac827e8ea87699023e640760fd09ef9bb/third_party/WebKit/Source/modules/indexeddb/IDBValueWrapping.cpp
[add] https://crrev.com/9f25693ac827e8ea87699023e640760fd09ef9bb/third_party/WebKit/Source/modules/indexeddb/IDBValueWrapping.h
[add] https://crrev.com/9f25693ac827e8ea87699023e640760fd09ef9bb/third_party/WebKit/Source/modules/indexeddb/IDBValueWrappingTest.cpp
[modify] https://crrev.com/9f25693ac827e8ea87699023e640760fd09ef9bb/third_party/WebKit/Source/modules/indexeddb/WebIDBCallbacksImpl.cpp
[modify] https://crrev.com/9f25693ac827e8ea87699023e640760fd09ef9bb/third_party/WebKit/Source/platform/testing/weburl_loader_mock_factory_impl.cc
[modify] https://crrev.com/9f25693ac827e8ea87699023e640760fd09ef9bb/third_party/WebKit/Source/platform/testing/weburl_loader_mock_factory_impl.h
[modify] https://crrev.com/9f25693ac827e8ea87699023e640760fd09ef9bb/third_party/WebKit/public/platform/WebURLLoaderMockFactory.h

Labels: -Pri-2 M-60 Pri-1
I'll keep this bug open until the M60 branch point cutoff is announced, and I'll make sure the CL above is included in M60.
Status: Started (was: Available)
Status: Fixed (was: Started)
Seems this change can have made IndexedDB stop inserting the primary key into value after putting large values with autoIncremented primary keys. https://github.com/dfahlander/Dexie.js/issues/551

I think you're referring to  http://crbug.com/735317  which should have been fixed. The fix landed in 60.0.3112.50, and is not in 60.0.3112.40, so you might need to go to chrome://settings/help to force an update.

Sign in to add a comment