Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
821913004:
[Storage] Consoliation of BlobItems with small size (Closed)
Description[Storage] Consolidation of BlobItems with small size
https://bit.ly/AutoBlobToDisk
R=michaeln, piman
BUG=375297
Committed: https://crrev.com/4e45f41babddb10985c8c0afb9b0b6b9e8356094
Cr-Commit-Position: refs/heads/master@{#312770}
Committed: https://crrev.com/a986a8db3f226762d312601a3cf6b770fe9df6ee
Cr-Commit-Position: refs/heads/master@{#312907}
Patch Set 1 #Patch Set 2 : switching threshold checks to keep old behavior #Patch Set 3 : fix for clearing buffer #
Total comments: 11
Patch Set 4 : Style restructure #Patch Set 5 : Removed C++11 vector data() call #
Total comments: 4
Patch Set 6 : More memory efficient #Patch Set 7 : cleanup #
Total comments: 12
Patch Set 8 : Fixes #Patch Set 9 : Size fix #Patch Set 10 : Merge and length calc fix #
Messages
Total messages: 29 (7 generated)
Hello michaeln, Please take a look at this patch, it consolidates blob items with small size to fit into the 250 KB window. Thanks, Daniel
https://codereview.chromium.org/821913004/diff/40001/content/child/webblobreg... File content/child/webblobregistry_impl.cc (left): https://codereview.chromium.org/821913004/diff/40001/content/child/webblobreg... content/child/webblobregistry_impl.cc:54: SendDataForBlob(uuid_str, data_item.data); might be nice to bury the new buffering logic inside of the BufferDataForBlob() helper so the registerBlobData() remains more easily read https://codereview.chromium.org/821913004/diff/40001/content/child/webblobreg... File content/child/webblobregistry_impl.cc (right): https://codereview.chromium.org/821913004/diff/40001/content/child/webblobreg... content/child/webblobregistry_impl.cc:46: std::vector<char> buf_; nit: nix trailing underbar https://codereview.chromium.org/821913004/diff/40001/content/child/webblobreg... content/child/webblobregistry_impl.cc:48: size_t consolidating_items_size_bytes; do we need this local? what about buf.size()? https://codereview.chromium.org/821913004/diff/40001/content/child/webblobreg... content/child/webblobregistry_impl.cc:53: while (data.itemAt(i++, data_item)) { in each case below, you could avoid flushing the buffer if length is zero. maybe have a test like that up top here to continue the loop for empty items https://codereview.chromium.org/821913004/diff/40001/content/child/webblobreg... content/child/webblobregistry_impl.cc:65: SendAndClearConsolidatingBuffer(uuid_str, &buf_, i think this can get called even if consolidating_items_size_bytes is zero https://codereview.chromium.org/821913004/diff/40001/content/child/webblobreg... content/child/webblobregistry_impl.cc:142: void WebBlobRegistryImpl::SendAndClearConsolidatingBuffer( having a test for if (buffer->size()) return in here might help readabilty elsewhere and avoid sending 0 len items naming nit: this is a FlushDataBuffer kind of operation
Question: Currently I'm allocating a vector, and then the relevant data is copied again into a DataElement when I copy the vector. Do you think it would be worth it to make the buffer shared memory, where we don't do the second copy to DataElement? This depends on the overhead of just copying the amount of memory, and allocating shared memory. Keep in mind this is for every blob (we could be smart and only allocated when there is data to be sent). Daniel https://codereview.chromium.org/821913004/diff/40001/content/child/webblobreg... File content/child/webblobregistry_impl.cc (right): https://codereview.chromium.org/821913004/diff/40001/content/child/webblobreg... content/child/webblobregistry_impl.cc:46: std::vector<char> buf_; On 2015/01/21 02:16:48, michaeln wrote: > nit: nix trailing underbar Done. https://codereview.chromium.org/821913004/diff/40001/content/child/webblobreg... content/child/webblobregistry_impl.cc:48: size_t consolidating_items_size_bytes; On 2015/01/21 02:16:48, michaeln wrote: > do we need this local? what about buf.size()? Ah, I was getting confused with arrays and vectors here. https://codereview.chromium.org/821913004/diff/40001/content/child/webblobreg... content/child/webblobregistry_impl.cc:53: while (data.itemAt(i++, data_item)) { On 2015/01/21 02:16:48, michaeln wrote: > in each case below, you could avoid flushing the buffer if length is zero. maybe > have a test like that up top here to continue the loop for empty items Done. https://codereview.chromium.org/821913004/diff/40001/content/child/webblobreg... content/child/webblobregistry_impl.cc:65: SendAndClearConsolidatingBuffer(uuid_str, &buf_, On 2015/01/21 02:16:48, michaeln wrote: > i think this can get called even if consolidating_items_size_bytes is zero Done. https://codereview.chromium.org/821913004/diff/40001/content/child/webblobreg... content/child/webblobregistry_impl.cc:142: void WebBlobRegistryImpl::SendAndClearConsolidatingBuffer( On 2015/01/21 02:16:47, michaeln wrote: > having a test for if (buffer->size()) return in here might help readabilty > elsewhere and avoid sending 0 len items > > naming nit: this is a FlushDataBuffer kind of operation Done.
> Currently I'm allocating a vector, and then the relevant data is copied again > into a DataElement when I copy the vector. Do you think it would be worth it to > make the buffer shared memory, where we don't do the second copy to DataElement? > > This depends on the overhead of just copying the amount of memory, and > allocating shared memory. Keep in mind this is for every blob (we could be > smart and only allocated when there is data to be sent). We don't want to always use shared mem because on some platforms it involves sync ipcs to allocate the block because the privileged browser process has to do it. To avoid a copy, instead of keeping a naked vector on the stack, you could keep a DataElement 'consolidated_data_item' and append to it's buf_.
https://codereview.chromium.org/821913004/diff/80001/content/child/webblobreg... File content/child/webblobregistry_impl.cc (right): https://codereview.chromium.org/821913004/diff/80001/content/child/webblobreg... content/child/webblobregistry_impl.cc:47: consolidating_buffer.reserve(kLargeThresholdBytes); should we care about preallocating this much mem? if so, maybe this large reservation can be done inside of BufferBlobData if data.len > buf.capacity? do you know what the default capacity typically is? https://codereview.chromium.org/821913004/diff/80001/content/child/webblobreg... content/child/webblobregistry_impl.cc:70: if (data_item.length) { these tests in the case blocks could come out now
Done, I switched to just using a DataElement and adding functionality to that class. Hopefully that's ok. https://codereview.chromium.org/821913004/diff/80001/content/child/webblobreg... File content/child/webblobregistry_impl.cc (right): https://codereview.chromium.org/821913004/diff/80001/content/child/webblobreg... content/child/webblobregistry_impl.cc:47: consolidating_buffer.reserve(kLargeThresholdBytes); On 2015/01/22 00:46:42, michaeln wrote: > should we care about preallocating this much mem? if so, maybe this large > reservation can be done inside of BufferBlobData if data.len > buf.capacity? do > you know what the default capacity typically is? I can just use the internal vector in DataElement. The resizing behavior is pretty smart std::vector, so I don't think we have to worry about crazy append allocation. https://codereview.chromium.org/821913004/diff/80001/content/child/webblobreg... content/child/webblobregistry_impl.cc:70: if (data_item.length) { On 2015/01/22 00:46:42, michaeln wrote: > these tests in the case blocks could come out now Done.
https://codereview.chromium.org/821913004/diff/120001/content/child/webblobre... File content/child/webblobregistry_impl.cc (right): https://codereview.chromium.org/821913004/diff/120001/content/child/webblobre... content/child/webblobregistry_impl.cc:54: // This skips empty files and blobs. We skip empty data below. this comment seems unnecessary https://codereview.chromium.org/821913004/diff/120001/content/child/webblobre... content/child/webblobregistry_impl.cc:66: if (data_item.data.size() == 0) { is WebBlobData::Item.data.size() ever != to WebBlobData::Item.length? if so, a comment here noting that might make sense. but more importantly, if so, could the check on line 55 be filtering out items with data but with a .length field of 0? https://codereview.chromium.org/821913004/diff/120001/content/child/webblobre... content/child/webblobregistry_impl.cc:73: storage::BlobData::Item item; there are compile errors about 'item' being defined multiple times https://codereview.chromium.org/821913004/diff/120001/content/child/webblobre... content/child/webblobregistry_impl.cc:136: storage::BlobData::Item item; local no longer needed https://codereview.chromium.org/821913004/diff/120001/storage/common/data_ele... File storage/common/data_element.h (right): https://codereview.chromium.org/821913004/diff/120001/storage/common/data_ele... storage/common/data_element.h:58: length_ = 0; maybe also set bytes_ to nullptr https://codereview.chromium.org/821913004/diff/120001/storage/common/data_ele... storage/common/data_element.h:65: DCHECK_NE(length_, kuint64max); i think you can DCHECK(!bytes_);
https://codereview.chromium.org/821913004/diff/120001/content/child/webblobre... File content/child/webblobregistry_impl.cc (right): https://codereview.chromium.org/821913004/diff/120001/content/child/webblobre... content/child/webblobregistry_impl.cc:54: // This skips empty files and blobs. We skip empty data below. On 2015/01/22 21:57:46, michaeln wrote: > this comment seems unnecessary Done. https://codereview.chromium.org/821913004/diff/120001/content/child/webblobre... content/child/webblobregistry_impl.cc:66: if (data_item.data.size() == 0) { On 2015/01/22 21:57:46, michaeln wrote: > is WebBlobData::Item.data.size() ever != to WebBlobData::Item.length? if so, a > comment here noting that might make sense. > > but more importantly, if so, could the check on line 55 be filtering out items > with data but with a .length field of 0? It never equals the size. See the statement above, we DCHECK that data_item.length is -1. https://codereview.chromium.org/821913004/diff/120001/content/child/webblobre... content/child/webblobregistry_impl.cc:73: storage::BlobData::Item item; On 2015/01/22 21:57:46, michaeln wrote: > there are compile errors about 'item' being defined multiple times Done. https://codereview.chromium.org/821913004/diff/120001/content/child/webblobre... content/child/webblobregistry_impl.cc:136: storage::BlobData::Item item; On 2015/01/22 21:57:46, michaeln wrote: > local no longer needed Done. https://codereview.chromium.org/821913004/diff/120001/storage/common/data_ele... File storage/common/data_element.h (right): https://codereview.chromium.org/821913004/diff/120001/storage/common/data_ele... storage/common/data_element.h:58: length_ = 0; On 2015/01/22 21:57:46, michaeln wrote: > maybe also set bytes_ to nullptr Done. https://codereview.chromium.org/821913004/diff/120001/storage/common/data_ele... storage/common/data_element.h:65: DCHECK_NE(length_, kuint64max); On 2015/01/22 21:57:46, michaeln wrote: > i think you can DCHECK(!bytes_); Done.
lgtm!
The CQ bit was checked by dmurph@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/821913004/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
dmurph@chromium.org changed reviewers: + piman@chromium.org
+piman for owners
lgtm
The CQ bit was checked by dmurph@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/821913004/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by dmurph@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/821913004/140001
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/4e45f41babddb10985c8c0afb9b0b6b9e8356094 Cr-Commit-Position: refs/heads/master@{#312770}
Message was sent while issue was closed.
A revert of this CL (patchset #8 id:140001) has been created in https://codereview.chromium.org/869763003/ by kochi@chromium.org. The reason for reverting is: This caused regression on some layout tests. e.g. http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Mac10.6%20%28de... fast/files/read-blob-async.html fast/files/workers/worker-read-blob-async.html fast/files/workers/worker-read-blob-sync.html fast/files/workers/worker-read-file-constructor-async.html http/tests/local/blob/send-data-blob.html http/tests/navigation/beacon-same-origin.html http/tests/websocket/bufferedAmount-after-send.html .
The CQ bit was checked by dmurph@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/821913004/180001
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/a986a8db3f226762d312601a3cf6b770fe9df6ee Cr-Commit-Position: refs/heads/master@{#312907} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
