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

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 months, 1 week ago by pwnall
Modified:
2 months, 3 weeks ago
CC:
Mads Ager (chromium), blink-reviews, chromium-reviews, cmumford, DmitrySkiba, jsbell+idb_chromium.org, kinuko+fileapi, kinuko+watch, kouhei+heap_chromium.org, Mircea Trofin, nhiroki, 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 TBR=haraken Review-Url: https://codereview.chromium.org/2822453003 Cr-Commit-Position: refs/heads/master@{#474874} Committed: https://chromium.googlesource.com/chromium/src/+/9f25693ac827e8ea87699023e640760fd09ef9bb

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: 41

Patch Set 10 : Test fixes and most of round 3 of feedback. #

Patch Set 11 : WTF Bind #

Patch Set 12 : Finished up round 3 of feedback. #

Patch Set 13 : Fixed Windows compilation. #

Total comments: 14

Patch Set 14 : Addressed last round of feedback. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+2909 lines, -148 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 8 9 10 11 1 chunk +244 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 8 9 10 11 1 chunk +369 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/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 1 chunk +1 line, -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 9 7 chunks +94 lines, -19 lines 0 comments Download
M third_party/WebKit/Source/modules/indexeddb/IDBRequest.cpp View 1 2 3 4 5 6 7 8 9 10 12 chunks +125 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 8 9 10 11 12 13 1 chunk +185 lines, -0 lines 0 comments Download
A third_party/WebKit/Source/modules/indexeddb/IDBRequestQueueItem.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +263 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/modules/indexeddb/IDBRequestTest.cpp View 1 2 3 4 5 6 7 8 9 4 chunks +173 lines, -18 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 9 2 chunks +28 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/modules/indexeddb/IDBTransactionTest.cpp View 1 2 3 4 5 6 7 8 9 4 chunks +199 lines, -36 lines 0 comments Download
M third_party/WebKit/Source/modules/indexeddb/IDBValue.h View 1 2 3 4 5 6 7 8 9 4 chunks +12 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/modules/indexeddb/IDBValue.cpp View 1 2 3 4 5 6 7 8 9 2 chunks +15 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 10 11 12 1 chunk +214 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 10 11 1 chunk +256 lines, -0 lines 0 comments Download
A third_party/WebKit/Source/modules/indexeddb/IDBValueWrappingTest.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +72 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
M third_party/WebKit/Source/platform/testing/weburl_loader_mock_factory_impl.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 3 chunks +16 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/platform/testing/weburl_loader_mock_factory_impl.cc View 1 2 3 4 5 6 7 8 9 3 chunks +59 lines, -11 lines 0 comments Download
M third_party/WebKit/public/platform/WebURLLoaderMockFactory.h View 1 2 3 4 5 6 7 8 9 2 chunks +13 lines, -2 lines 0 comments Download
Commit queue not available (can’t edit this change).

Messages

Total messages: 130 (100 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 ...
4 months, 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 > ...
4 months, 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 ...
3 months, 2 weeks 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 -> ...
3 months, 2 weeks 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 ...
3 months, 2 weeks 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 ...
3 months, 2 weeks 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 ...
3 months, 1 week 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 ...
3 months, 1 week 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 ...
3 months, 1 week 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: > ...
3 months, 1 week 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 ...
3 months, 1 week 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/ ...
3 months 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?
3 months 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: > ...
3 months 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 ...
3 months ago (2017-05-23 18:16:40 UTC) #73
jbroman
https://codereview.chromium.org/2822453003/diff/480001/third_party/WebKit/Source/modules/indexeddb/IDBValueWrapping.cpp File third_party/WebKit/Source/modules/indexeddb/IDBValueWrapping.cpp (right): https://codereview.chromium.org/2822453003/diff/480001/third_party/WebKit/Source/modules/indexeddb/IDBValueWrapping.cpp#newcode39 third_party/WebKit/Source/modules/indexeddb/IDBValueWrapping.cpp:39: // will have to be bumped. Yes, I believe ...
3 months ago (2017-05-23 20:13:11 UTC) #74
pwnall
foolip@chromium.org: Can you please review the changes in third_party/WebKit/public/platform/WebURLLoaderMockFactory.h? 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 third_party/WebKit/LayoutTests/external/wpt/IndexedDB/large-nested-cloning.html:15: ...
2 months, 4 weeks ago (2017-05-25 13:27:12 UTC) #104
pwnall
dcheng: Can you please review the changes in third_party/WebKit/public/platform/WebURLLoaderMockFactory.h? jsbell, dmurph: PTAL?
2 months, 4 weeks ago (2017-05-25 13:51:51 UTC) #108
jbroman
https://codereview.chromium.org/2822453003/diff/480001/third_party/WebKit/Source/modules/indexeddb/IDBValueWrapping.cpp File third_party/WebKit/Source/modules/indexeddb/IDBValueWrapping.cpp (right): https://codereview.chromium.org/2822453003/diff/480001/third_party/WebKit/Source/modules/indexeddb/IDBValueWrapping.cpp#newcode39 third_party/WebKit/Source/modules/indexeddb/IDBValueWrapping.cpp:39: // will have to be bumped. On 2017/05/25 at ...
2 months, 4 weeks ago (2017-05-25 14:50:59 UTC) #109
dmurph
lgtm with nit. Context for other reviewers: We're trying really hard to get this in ...
2 months, 4 weeks ago (2017-05-25 17:28:07 UTC) #112
dcheng
LGTM. I think the comments are fine to address in followups. https://codereview.chromium.org/2822453003/diff/720001/third_party/WebKit/Source/modules/indexeddb/IDBRequest.cpp File third_party/WebKit/Source/modules/indexeddb/IDBRequest.cpp (right): ...
2 months, 4 weeks ago (2017-05-25 18:34:05 UTC) #113
jsbell
lgtm with a couple nits https://codereview.chromium.org/2822453003/diff/720001/third_party/WebKit/Source/modules/indexeddb/IDBRequestQueueItem.cpp File third_party/WebKit/Source/modules/indexeddb/IDBRequestQueueItem.cpp (right): https://codereview.chromium.org/2822453003/diff/720001/third_party/WebKit/Source/modules/indexeddb/IDBRequestQueueItem.cpp#newcode1 third_party/WebKit/Source/modules/indexeddb/IDBRequestQueueItem.cpp:1: Nit: remove blank line ...
2 months, 4 weeks ago (2017-05-25 18:49:42 UTC) #114
pwnall
dmurph, dcheng, jsbell: Thank you very much for the super-timely reviews! https://codereview.chromium.org/2822453003/diff/720001/third_party/WebKit/Source/modules/indexeddb/IDBRequest.cpp File third_party/WebKit/Source/modules/indexeddb/IDBRequest.cpp (right): ...
2 months, 4 weeks ago (2017-05-25 20:01:11 UTC) #117
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2822453003/740001
2 months, 4 weeks ago (2017-05-25 20:01:11 UTC) #118
commit-bot: I haz the power
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/447828)
2 months, 4 weeks ago (2017-05-25 20:11:32 UTC) #120
pwnall
TBR=haraken for the changes to the files below: * third_party/WebKit/Source/modules/BUILD.gn -- add IDBWrappingTest.cpp to list ...
2 months, 4 weeks ago (2017-05-25 20:20:34 UTC) #123
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2822453003/740001
2 months, 4 weeks ago (2017-05-25 20:21:27 UTC) #125
haraken
rs LGTM
2 months, 4 weeks ago (2017-05-25 23:23:56 UTC) #126
commit-bot: I haz the power
Committed patchset #14 (id:740001) as https://chromium.googlesource.com/chromium/src/+/9f25693ac827e8ea87699023e640760fd09ef9bb
2 months, 4 weeks ago (2017-05-26 01:59:47 UTC) #129
pwnall
2 months, 3 weeks ago (2017-05-26 22:31:44 UTC) #130
Message was sent while issue was closed.
On 2017/05/25 23:23:56, haraken wrote:
> rs LGTM

Thank you very much for the quick review!
Sign in to reply to this message.

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