Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(5)

Issue 2822453003: Wrap large IndexedDB values into Blobs before writing to LevelDB.

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 month, 1 week ago by pwnall
Modified:
3 hours, 9 minutes ago
Reviewers:
dmurph, jsbell, jbroman
CC:
Mads Ager (chromium), blink-reviews, chromium-reviews, cmumford, DmitrySkiba, haraken, jsbell+idb_chromium.org, kinuko+fileapi, kinuko+watch, kouhei+heap_chromium.org, Mircea Trofin, nhiroki (slow), oilpan-reviews, tzik
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

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

Patch Set 1 #

Total comments: 2

Patch Set 2 : Fixed compilation errors on Windows and no-DCHECKs. #

Total comments: 50

Patch Set 3 : Attempt to fix compile error. #

Patch Set 4 : Rebase + start addressing feedback. #

Total comments: 20

Patch Set 5 : Addressed feedback. #

Patch Set 6 : WIP: Getting IDBRequestTest.EventsAfterStopping to pass. #

Total comments: 81

Patch Set 7 : Rebased. #

Patch Set 8 : Addressed round 2 of feedback. #

Patch Set 9 : Rebased. #

Total comments: 19

Patch Set 10 : Addressing feedback round 3. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+2219 lines, -92 lines) Patch
A third_party/WebKit/LayoutTests/external/wpt/IndexedDB/large-nested-cloning.html View 1 2 3 4 5 6 7 1 chunk +288 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/external/wpt/IndexedDB/large-requests-abort.html View 1 2 3 4 5 6 7 1 chunk +228 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/external/wpt/IndexedDB/request-event-ordering.html View 1 2 3 4 5 6 7 1 chunk +351 lines, -0 lines 0 comments Download
M third_party/WebKit/LayoutTests/external/wpt/IndexedDB/support-promises.js View 1 2 3 4 5 6 7 1 chunk +20 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/modules/indexeddb/BUILD.gn View 1 2 3 4 1 chunk +6 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/modules/indexeddb/IDBFactory.cpp View 1 2 3 4 5 6 7 8 9 3 chunks +3 lines, -3 lines 0 comments Download
M third_party/WebKit/Source/modules/indexeddb/IDBObjectStore.cpp View 1 2 3 4 5 6 6 chunks +19 lines, -28 lines 0 comments Download
M third_party/WebKit/Source/modules/indexeddb/IDBOpenDBRequest.h View 1 2 3 4 3 chunks +2 lines, -3 lines 0 comments Download
M third_party/WebKit/Source/modules/indexeddb/IDBOpenDBRequest.cpp View 1 2 3 4 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/Source/modules/indexeddb/IDBRequest.h View 1 2 3 4 5 6 7 8 7 chunks +92 lines, -19 lines 0 comments Download
M third_party/WebKit/Source/modules/indexeddb/IDBRequest.cpp View 1 2 3 4 5 6 7 8 10 chunks +95 lines, -16 lines 0 comments Download
A third_party/WebKit/Source/modules/indexeddb/IDBRequestLoader.h View 1 2 3 4 5 6 7 8 9 1 chunk +91 lines, -0 lines 0 comments Download
A third_party/WebKit/Source/modules/indexeddb/IDBRequestLoader.cpp View 1 2 3 4 5 6 7 8 9 1 chunk +97 lines, -0 lines 0 comments Download
A third_party/WebKit/Source/modules/indexeddb/IDBRequestQueueItem.h View 1 2 3 4 5 6 7 1 chunk +151 lines, -0 lines 0 comments Download
A third_party/WebKit/Source/modules/indexeddb/IDBRequestQueueItem.cpp View 1 2 3 4 5 6 7 1 chunk +216 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/modules/indexeddb/IDBRequestTest.cpp View 1 2 3 4 5 6 7 3 chunks +31 lines, -11 lines 0 comments Download
M third_party/WebKit/Source/modules/indexeddb/IDBTransaction.h View 1 2 3 4 5 6 7 8 9 5 chunks +22 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/modules/indexeddb/IDBTransaction.cpp View 1 2 3 4 5 6 7 8 2 chunks +31 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/modules/indexeddb/IDBValue.h View 3 chunks +8 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/modules/indexeddb/IDBValue.cpp View 1 2 3 1 chunk +7 lines, -0 lines 0 comments Download
A third_party/WebKit/Source/modules/indexeddb/IDBValueWrapping.h View 1 2 3 4 5 6 7 8 9 1 chunk +187 lines, -0 lines 0 comments Download
A third_party/WebKit/Source/modules/indexeddb/IDBValueWrapping.cpp View 1 2 3 4 5 6 7 8 9 1 chunk +251 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/modules/indexeddb/WebIDBCallbacksImpl.cpp View 1 2 3 4 5 6 7 8 12 chunks +22 lines, -11 lines 0 comments Download
Commit queue not available (can’t edit this change).

Messages

Total messages: 80 (64 generated)
dmurph
https://codereview.chromium.org/2822453003/diff/1/third_party/WebKit/Source/modules/indexeddb/IDBValueWrapper.cpp File third_party/WebKit/Source/modules/indexeddb/IDBValueWrapper.cpp (right): https://codereview.chromium.org/2822453003/diff/1/third_party/WebKit/Source/modules/indexeddb/IDBValueWrapper.cpp#newcode77 third_party/WebKit/Source/modules/indexeddb/IDBValueWrapper.cpp:77: Blob* wrapper = Ok so this is how we'll ...
1 month, 1 week ago (2017-04-13 18:05:00 UTC) #2
pwnall
On 2017/04/13 18:05:00, dmurph wrote: > https://codereview.chromium.org/2822453003/diff/1/third_party/WebKit/Source/modules/indexeddb/IDBValueWrapper.cpp > File third_party/WebKit/Source/modules/indexeddb/IDBValueWrapper.cpp (right): > > https://codereview.chromium.org/2822453003/diff/1/third_party/WebKit/Source/modules/indexeddb/IDBValueWrapper.cpp#newcode77 > ...
1 month, 1 week ago (2017-04-13 19:37:06 UTC) #3
pwnall
jsbell: Can you please review the CL (including architecture / general approach)? dmurph: Can you ...
2 weeks, 6 days ago (2017-05-04 00:45:15 UTC) #30
dmurph
Some initial comments. https://codereview.chromium.org/2822453003/diff/200001/third_party/WebKit/LayoutTests/external/wpt/IndexedDB/large-nested-cloning.html File third_party/WebKit/LayoutTests/external/wpt/IndexedDB/large-nested-cloning.html (right): https://codereview.chromium.org/2822453003/diff/200001/third_party/WebKit/LayoutTests/external/wpt/IndexedDB/large-nested-cloning.html#newcode127 third_party/WebKit/LayoutTests/external/wpt/IndexedDB/large-nested-cloning.html:127: function cloningTest(label, descriptors) { descriptors -> ...
2 weeks, 5 days ago (2017-05-04 22:27:08 UTC) #35
jsbell
Sorry for an incomplete/scattered review. Initial notes, mostly nits. I'll continue the review tomorrow. https://codereview.chromium.org/2822453003/diff/200001/third_party/WebKit/Source/modules/indexeddb/IDBRequest.h ...
2 weeks, 5 days ago (2017-05-05 00:29:37 UTC) #36
jsbell
Next batch of comments https://codereview.chromium.org/2822453003/diff/240001/third_party/WebKit/Source/modules/indexeddb/IDBObjectStore.cpp File third_party/WebKit/Source/modules/indexeddb/IDBObjectStore.cpp (right): https://codereview.chromium.org/2822453003/diff/240001/third_party/WebKit/Source/modules/indexeddb/IDBObjectStore.cpp#newcode383 third_party/WebKit/Source/modules/indexeddb/IDBObjectStore.cpp:383: IDBValueWrapper value_wrapper(isolate, value.V8Value(), write_wasm_to_stream, Is ...
2 weeks, 1 day ago (2017-05-08 22:08:02 UTC) #39
pwnall
dmurph, jsbell: Thank you very much for the quick and thorough feedback! Unfortunately, I wasn't ...
1 week, 5 days ago (2017-05-11 23:54:26 UTC) #54
dmurph
This looks really good. I'm going to take another pass this evening, I didn't really ...
1 week, 1 day ago (2017-05-15 19:32:23 UTC) #57
pwnall
On 2017/05/15 19:32:23, dmurph wrote: > What do you think about moving logic out of ...
1 week, 1 day ago (2017-05-15 23:14:43 UTC) #58
pwnall
https://codereview.chromium.org/2822453003/diff/360001/third_party/WebKit/Source/modules/indexeddb/IDBRequestQueueItem.cpp File third_party/WebKit/Source/modules/indexeddb/IDBRequestQueueItem.cpp (right): https://codereview.chromium.org/2822453003/diff/360001/third_party/WebKit/Source/modules/indexeddb/IDBRequestQueueItem.cpp#newcode126 third_party/WebKit/Source/modules/indexeddb/IDBRequestQueueItem.cpp:126: void IDBRequestQueueItem::MarkReady() { On 2017/05/15 19:32:23, dmurph wrote: > ...
1 week, 1 day ago (2017-05-15 23:15:00 UTC) #59
jsbell
https://codereview.chromium.org/2822453003/diff/360001/third_party/WebKit/LayoutTests/external/wpt/IndexedDB/large-nested-cloning.html File third_party/WebKit/LayoutTests/external/wpt/IndexedDB/large-nested-cloning.html (right): https://codereview.chromium.org/2822453003/diff/360001/third_party/WebKit/LayoutTests/external/wpt/IndexedDB/large-nested-cloning.html#newcode3 third_party/WebKit/LayoutTests/external/wpt/IndexedDB/large-nested-cloning.html:3: <title>IndexedDB: large nested objects are cloned correctly</title> aside: I ...
1 week, 1 day ago (2017-05-15 23:37:39 UTC) #60
pwnall
jsbell, dmurph: PTAL? I'll make sure all tests pass after the following CLs land: https://chromium-review.googlesource.com/c/509150/ ...
4 days, 11 hours ago (2017-05-19 18:27:35 UTC) #65
pwnall
jbroman: Can you please review the accuracy of the comment on lines 22-39 of third_party/WebKit/Source/modules/indexeddb/IDBValueWrapping.cpp?
4 days, 11 hours ago (2017-05-19 18:30:35 UTC) #67
jsbell
overall l*g*t*m https://codereview.chromium.org/2822453003/diff/360001/third_party/WebKit/Source/modules/indexeddb/IDBRequest.cpp File third_party/WebKit/Source/modules/indexeddb/IDBRequest.cpp (right): https://codereview.chromium.org/2822453003/diff/360001/third_party/WebKit/Source/modules/indexeddb/IDBRequest.cpp#newcode256 third_party/WebKit/Source/modules/indexeddb/IDBRequest.cpp:256: transit_blob_handles_.clear(); On 2017/05/19 18:27:33, pwnall wrote: > ...
1 day, 8 hours ago (2017-05-22 21:54:20 UTC) #72
dmurph
Some arch suggestions around the queue item, and test suggestion. https://codereview.chromium.org/2822453003/diff/480001/third_party/WebKit/LayoutTests/external/wpt/IndexedDB/large-nested-cloning.html File third_party/WebKit/LayoutTests/external/wpt/IndexedDB/large-nested-cloning.html (right): https://codereview.chromium.org/2822453003/diff/480001/third_party/WebKit/LayoutTests/external/wpt/IndexedDB/large-nested-cloning.html#newcode15 ...
11 hours, 39 minutes ago (2017-05-23 18:16:40 UTC) #73
jbroman
9 hours, 42 minutes ago (2017-05-23 20:13:11 UTC) #74
https://codereview.chromium.org/2822453003/diff/480001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/indexeddb/IDBValueWrapping.cpp (right):

https://codereview.chromium.org/2822453003/diff/480001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/indexeddb/IDBValueWrapping.cpp:39: // will
have to be bumped.
Yes, I believe this is true. It saddens me somewhat to have so many clever
tricks used here, basically arising from the fact that we didn't originally have
a magic number.

It might be simpler to just grab and reserve a tag that was never used (0xFE or
something), and just use that to mark the value. Anything current should begin
with 0xFF, and it seems to me that this requires slightly less of a history
lesson. Am I forgetting something?

https://codereview.chromium.org/2822453003/diff/480001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/indexeddb/IDBValueWrapping.h (right):

https://codereview.chromium.org/2822453003/diff/480001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/indexeddb/IDBValueWrapping.h:97:
"application/x-blink-idb-value-wrapper";
IIUC, IANA no longer recommends deploying unregistered MIME types with the x-
prefix.
https://tools.ietf.org/html/rfc6838

Dunno if it's worth registering this, using x. instead of x-, or just deciding
this is never going to be visible and thus we don't care. (Or we could use
application/octet-stream, I suppose.)
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 650457f06