Issue 2055053003: [BlobAsync] Disk support for blob storage

dmurph

Description was changed from ========== [BlobAsync] Disk support for blob storage This involves moving the ...

4 years, 6 months ago (2016-06-24 20:31:45 UTC) #1

dmurph

dmurph@chromium.org changed reviewers: + kinuko@chromium.org, michaeln@chromium.org

4 years, 6 months ago (2016-06-24 20:33:42 UTC) #2

dmurph

THIS IS IT GUYS. I have blob paging working, and it's passing all of the ...

4 years, 6 months ago (2016-06-24 20:33:43 UTC) #3

dmurph

Description was changed from ========== [Blob] Disk support for blob storage. This is the final ...

4 years, 6 months ago (2016-06-24 20:34:15 UTC) #4

Marijn Kruisselbrink

Just some random comments https://codereview.chromium.org/2055053003/diff/60001/storage/browser/blob/blob_flattener.cc File storage/browser/blob/blob_flattener.cc (right): https://codereview.chromium.org/2055053003/diff/60001/storage/browser/blob/blob_flattener.cc#newcode34 storage/browser/blob/blob_flattener.cc:34: size_t blob_flattening_index_offset = 0; You're ...

4 years, 5 months ago (2016-06-29 22:42:21 UTC) #8

dmurph

Thanks for taking a look! I simplified the memory requesting logic, moved all of that ...

4 years, 5 months ago (2016-07-06 23:44:29 UTC) #9

Thanks for taking a look! I simplified the memory requesting logic, moved all of
that out of the async_builder, and cleaned things up a little based on your
comments.

PTAL everyone! It still has log statements and I need to re-add the metrics back
in, but it's looking not bad!

https://codereview.chromium.org/2055053003/diff/60001/storage/browser/blob/bl...
File storage/browser/blob/blob_flattener.cc (right):

https://codereview.chromium.org/2055053003/diff/60001/storage/browser/blob/bl...
storage/browser/blob/blob_flattener.cc:34: size_t blob_flattening_index_offset =
0;
On 2016/06/29 at 22:42:21, Marijn Kruisselbrink wrote:
> You're not using this for anything?

Removed thanks.

https://codereview.chromium.org/2055053003/diff/60001/storage/browser/blob/bl...
storage/browser/blob/blob_flattener.cc:50: slice.done_building() ?
copies_from_built_ref
On 2016/06/29 at 22:42:21, Marijn Kruisselbrink wrote:
> is done_building() her the right thing to do? Or can you just get rid of
done_building completely and instead decide on the type() of the item being
sliced which list to add to? (TYPE_BYTES added to copies_from_built_ref and
TYPE_BYTES_DESCRIPTION added to copies_from_pending_ref)? Or I might be
misunderstanding how this works.

I can remove this now, especially because I no longer do the map for blob
slices.

https://codereview.chromium.org/2055053003/diff/60001/storage/browser/blob/bl...
File storage/browser/blob/blob_slice.cc (right):

https://codereview.chromium.org/2055053003/diff/60001/storage/browser/blob/bl...
storage/browser/blob/blob_slice.cc:122: size_t mid = 0;
On 2016/06/29 at 22:42:21, Marijn Kruisselbrink wrote:
> Couldn't you write almost this entire method using one of the binary search
methods built in to c++? Something like:
> 
> *item_index = std::upper_bound(offsets.begin(), offsets.end(), blob_offset) -
offsets.begin();
> *offset_in_item = *item_index == 0 ? blob_offset : blob_offset -
offsets[*item_index - 1];

UR A WIZZARD 'AARY! 
https://youtu.be/kIq-xXsM1-4?t=17

https://codereview.chromium.org/2055053003/diff/60001/storage/browser/blob/bl...
File storage/browser/blob/blob_storage_registry.h (right):

https://codereview.chromium.org/2055053003/diff/60001/storage/browser/blob/bl...
storage/browser/blob/blob_storage_registry.h:68: size_t source_item_index = 0;
On 2016/06/29 at 22:42:21, Marijn Kruisselbrink wrote:
> Would it be any cleaner/simpler if instead of uuid + item_indices you stored a
scoped_refptr<ShareableBlobDataItem> directly in ItemCopyEntry?

hmmmmmmmm yeah probably ;)

kinuko

Random-ish minor comments to start with... (have only done very partial review yet) https://codereview.chromium.org/2055053003/diff/90001/content/browser/blob_storage/blob_async_builder_host_unittest.cc File ...

4 years, 5 months ago (2016-07-07 16:11:44 UTC) #10

michaeln

not a complete review, i'm still sorting thru it https://codereview.chromium.org/2055053003/diff/40001/content/browser/blob_storage/blob_dispatcher_host.cc File content/browser/blob_storage/blob_dispatcher_host.cc (right): https://codereview.chromium.org/2055053003/diff/40001/content/browser/blob_storage/blob_dispatcher_host.cc#newcode83 content/browser/blob_storage/blob_dispatcher_host.cc:83: ...

4 years, 5 months ago (2016-07-07 20:05:23 UTC) #11

dmurph

Hello! I didn't have a chance to get to all comments but I did make ...

4 years, 5 months ago (2016-07-08 01:22:12 UTC) #12

Marijn Kruisselbrink

Some more random comments, hopefully not duplicating anybody else's comments. https://codereview.chromium.org/2055053003/diff/110001/content/browser/blob_storage/chrome_blob_storage_context.cc File content/browser/blob_storage/chrome_blob_storage_context.cc (right): https://codereview.chromium.org/2055053003/diff/110001/content/browser/blob_storage/chrome_blob_storage_context.cc#newcode115 ...

4 years, 5 months ago (2016-07-08 23:37:38 UTC) #13

dmurph

Thanks for the reviews everyone! QUESTION: Is this architected enough for me to start writing ...

4 years, 5 months ago (2016-07-11 23:33:29 UTC) #14

Thanks for the reviews everyone! QUESTION: Is this architected enough for me to
start writing tests for everything?

https://codereview.chromium.org/2055053003/diff/90001/storage/browser/blob/bl...
File storage/browser/blob/blob_async_builder_host.cc (right):

https://codereview.chromium.org/2055053003/diff/90001/storage/browser/blob/bl...
storage/browser/blob/blob_async_builder_host.cc:23:
BlobAsyncTransportRequestBuilder::RendererMemoryItemRequest;
On 2016/07/07 20:05:22, michaeln wrote:
> I think less nesting of structs and enum types might help readability,
> MemoryItemRequest is a fine top level struct/class name.

I consolidated all enums. I feel like keeping this one is fine, as it's only
used in this class and always directly ties to the request builder.

https://codereview.chromium.org/2055053003/diff/90001/storage/browser/blob/bl...
storage/browser/blob/blob_async_builder_host.cc:122: base::Callback<void(bool,
IPCBlobCreationCancelCode)>());
On 2016/07/07 20:05:22, michaeln wrote:
> this and line 81/82 should probably look alike

Done.

https://codereview.chromium.org/2055053003/diff/90001/storage/browser/blob/bl...
storage/browser/blob/blob_async_builder_host.cc:261: IPCBlobCreationCancelCode
reason) {
On 2016/07/07 20:05:22, michaeln wrote:
> where are |success| and |reason| used?

Fixed in new patch.

https://codereview.chromium.org/2055053003/diff/90001/storage/browser/blob/bl...
File storage/browser/blob/blob_memory_controller.cc (right):

https://codereview.chromium.org/2055053003/diff/90001/storage/browser/blob/bl...
storage/browser/blob/blob_memory_controller.cc:221: const
base::Callback<void(FileCreationInfo)>& done) {
On 2016/07/07 20:05:22, michaeln wrote:
> the name of this callback reads like a boolean value

Done.

https://codereview.chromium.org/2055053003/diff/90001/storage/browser/blob/bl...
storage/browser/blob/blob_memory_controller.cc:250: const
base::Callback<void(bool)>& can_request) {
On 2016/07/07 20:05:22, michaeln wrote:
> the name of this callback reads like a boolean value

Done.

https://codereview.chromium.org/2055053003/diff/90001/storage/browser/blob/bl...
storage/browser/blob/blob_memory_controller.cc:303:
recent_item_cache_.Put(item->item_id(), item);
On 2016/07/07 20:05:22, michaeln wrote:
> I think MRUCache::Put() has the erase-old, insert-new behavior built in. If
> that's right, no need for this method to call cache->Get() and cache->Erase()
> directly.
> 
> Wait... actually, looks like MRUCache::Get() puts the item at the front of the
> rececency queue. If that's right, all that's needed here is a call to
> mru_cache->Get(id).

Done.

https://codereview.chromium.org/2055053003/diff/90001/storage/browser/blob/bl...
storage/browser/blob/blob_memory_controller.cc:411:
shareable_item->item_.swap(new_item);
On 2016/07/07 20:05:23, michaeln wrote:
> shareable_item->item_ = new_item might be more obvious

Done.

https://codereview.chromium.org/2055053003/diff/90001/storage/browser/blob/bl...
File storage/browser/blob/blob_memory_controller.h (right):

https://codereview.chromium.org/2055053003/diff/90001/storage/browser/blob/bl...
storage/browser/blob/blob_memory_controller.h:70: base::Optional<base::File>
file = base::nullopt;
On 2016/07/07 20:05:23, michaeln wrote:
> The base::File object is inexpensive to construct and supports an empty state,
> see the IsValid() method. I'm not so familiar with the Optional<> template or
> when it's appropriate to use, it doesn't seem necessary in this case.
> 
> What the reason to use Optional<> for this data member?

We don't always need it. Changed back to regular file.

https://codereview.chromium.org/2055053003/diff/90001/storage/browser/blob/bl...
storage/browser/blob/blob_memory_controller.h:77: void SetMemoryLimits(size_t
max_blob_in_memory_size_bytes,
On 2016/07/07 16:11:44, kinuko wrote:
> Do we need this in addition to / separately from SetMemoryContantsForTesting? 
> This also seems to be used only for testing?

Done.

https://codereview.chromium.org/2055053003/diff/90001/storage/browser/blob/bl...
storage/browser/blob/blob_memory_controller.h:89: void EnableDisk(const
base::FilePath storage_directory,
On 2016/07/07 16:11:44, kinuko wrote:
> nit: const ref?

Done.

https://codereview.chromium.org/2055053003/diff/90001/storage/browser/blob/bl...
storage/browser/blob/blob_memory_controller.h:133: void
RemoveBlobItemInRecents(uint64_t id);
On 2016/07/07 20:05:23, michaeln wrote:
> for symmetry, maybe have this method take the |item| ptr as input like the
> Update method does

Done.

https://codereview.chromium.org/2055053003/diff/90001/storage/browser/blob/bl...
storage/browser/blob/blob_memory_controller.h:136: void DisableDisk();
On 2016/07/07 16:11:44, kinuko wrote:
> nit: I feel this and disk_enabled() should be placed next to EnableDisk()

Done.

https://codereview.chromium.org/2055053003/diff/90001/storage/browser/blob/bl...
File storage/browser/blob/blob_slice.cc (right):

https://codereview.chromium.org/2055053003/diff/90001/storage/browser/blob/bl...
storage/browser/blob/blob_slice.cc:17: uint64_t size) {
On 2016/07/07 20:05:23, michaeln wrote:
> some naming suggestions: source, slice_offset, slice_size

Done.

https://codereview.chromium.org/2055053003/diff/90001/storage/browser/blob/bl...
storage/browser/blob/blob_slice.cc:18: const auto& source_items =
source.items();
On 2016/07/07 20:05:23, michaeln wrote:
> what if source_items is empty? can that happen?

That shouldn't happen, as we check to see if we need to slice the blob before we
create the slice. Even so, it should correctly create the blob w/
copying_memory_size = 0, and no items. I'll start adding tests soon, as this
architecture seems pretty good.

https://codereview.chromium.org/2055053003/diff/90001/storage/browser/blob/bl...
storage/browser/blob/blob_slice.cc:22: uint64_t offset_from_mid =
On 2016/07/07 20:05:23, michaeln wrote:
> for these two locals, names like item_index and item_offset would've helped me
> 
> are there any assumptions being made about the inputs that could be DCHECK'd,
> like...
> DCHECK(!items.empty())
> DCHECK_LE(slice_offset + slice_size, source.size())

Done.

https://codereview.chromium.org/2055053003/diff/90001/storage/browser/blob/bl...
storage/browser/blob/blob_slice.cc:33: for (uint64_t memory_read = 0; mid <
num_items && memory_read < size; mid++) {
On 2016/07/07 20:05:23, michaeln wrote:
> it looks like |memory_read| also accumulates the sizes of file backed items?
is
> |memory_read| misnamed? maybe total_sliced?

Done.

https://codereview.chromium.org/2055053003/diff/90001/storage/browser/blob/bl...
storage/browser/blob/blob_slice.cc:36: if (length == 0) {
On 2016/07/07 20:05:23, michaeln wrote:
> do we have zero length items? i think at some point we didn't

We shouldn't. I'll dcheck this.

https://codereview.chromium.org/2055053003/diff/90001/storage/browser/blob/bl...
storage/browser/blob/blob_slice.cc:44: bool can_share_item =
On 2016/07/07 20:05:23, michaeln wrote:
> since this local is only used in the if below, getting rid of the local might
be
> a small improvement. a code comment that says // We can share the entire item
> would satisfy the purpose it looks like your want the local to serve

Done.

https://codereview.chromium.org/2055053003/diff/90001/storage/browser/blob/bl...
storage/browser/blob/blob_slice.cc:73: data_item = new
BlobDataItem(std::move(element), nullptr);
On 2016/07/07 20:05:23, michaeln wrote:
> when do we really copy the data?

When the blob is finished constructing. The flattener records all of these
'copies', and then we store them. When we have all other blob data (& we've
asked the memory manager if we can store this data) we do these copies.

https://codereview.chromium.org/2055053003/diff/110001/content/browser/blob_s...
File content/browser/blob_storage/chrome_blob_storage_context.cc (right):

https://codereview.chromium.org/2055053003/diff/110001/content/browser/blob_s...
content/browser/blob_storage/chrome_blob_storage_context.cc:115:
BrowserThread::GetMessageLoopProxyForThread(BrowserThread::FILE);
On 2016/07/08 23:37:36, Marijn Kruisselbrink wrote:
> Is BrowserThread::FILE the correct TaskRunner to use for all these file API
> calls, or should this use BrowserThread::GetPlockingPool instead?

I switched to blocking pool for all file stuff. Nice suggestion, didn't know
about that feature.

https://codereview.chromium.org/2055053003/diff/110001/storage/browser/blob/b...
File storage/browser/blob/blob_async_builder_host.cc (right):

https://codereview.chromium.org/2055053003/diff/110001/storage/browser/blob/b...
storage/browser/blob/blob_async_builder_host.cc:54: // Validate that our
referenced blobs.
On 2016/07/08 23:37:36, Marijn Kruisselbrink wrote:
> Validate what?

Done.

https://codereview.chromium.org/2055053003/diff/110001/storage/browser/blob/b...
File storage/browser/blob/blob_flattener.cc (right):

https://codereview.chromium.org/2055053003/diff/110001/storage/browser/blob/b...
storage/browser/blob/blob_flattener.cc:35: size_t current_browser_index = 0;
On 2016/07/08 23:37:37, Marijn Kruisselbrink wrote:
> You're never reading from this variable?

Done.

https://codereview.chromium.org/2055053003/diff/110001/storage/browser/blob/b...
storage/browser/blob/blob_flattener.cc:98:
output_blob->AppendSharedBlobItem(uuid, std::move(shareable_item));
On 2016/07/08 23:37:37, Marijn Kruisselbrink wrote:
> This std::move does nothing, as shareable_item is a const scoped_refptr<>&, so
> can't be moved from anyway.

I wish there was a compiler warning for this haha.

https://codereview.chromium.org/2055053003/diff/110001/storage/browser/blob/b...
File storage/browser/blob/blob_memory_controller.cc (right):

https://codereview.chromium.org/2055053003/diff/110001/storage/browser/blob/b...
storage/browser/blob/blob_memory_controller.cc:82: // ShareableFileReference
which will manage the lifetime of the file.
On 2016/07/08 23:37:37, Marijn Kruisselbrink wrote:
> comment doesn't seem to match the implementation? The caller of this method
had
> already created the ShareableFileReference, this just sets fields of that
> reference

Done.

https://codereview.chromium.org/2055053003/diff/110001/storage/browser/blob/b...
storage/browser/blob/blob_memory_controller.cc:88: if (creation_info.error ==
File::FILE_OK) {
On 2016/07/08 23:37:37, Marijn Kruisselbrink wrote:
> nit: Maybe just if (success) ?

Done.

https://codereview.chromium.org/2055053003/diff/110001/storage/browser/blob/b...
storage/browser/blob/blob_memory_controller.cc:119: int length =
base::checked_cast<int, uint64_t>(element.length());
On 2016/07/08 23:37:37, Marijn Kruisselbrink wrote:
> Why are you explicitly specifying the Src type? Can't checked_cast just get
that
> from the type of what you pass to it?
> 
> And what happens when the DataElement has a size that is larger than fits in
an
> int? Is that not possible, or will that result in only part of the element
being
> written to disk?

It's not possible, as we can't have that much memory in memory. I changed this
to a loop, just in case we want to write > int::max.

https://codereview.chromium.org/2055053003/diff/110001/storage/browser/blob/b...
storage/browser/blob/blob_memory_controller.cc:122: if (bytes_written < 0) {
On 2016/07/08 23:37:37, Marijn Kruisselbrink wrote:
> Should this check before the DCHECK_EQ(length, bytes_written)?

Removed dcheck.

https://codereview.chromium.org/2055053003/diff/110001/storage/browser/blob/b...
storage/browser/blob/blob_memory_controller.cc:274: if (blob_memory_used_ +
memory_size > max_blob_in_memory_size_) {
On 2016/07/08 23:37:37, Marijn Kruisselbrink wrote:
> How can this condition ever be true? The previous check already ensured that
> memory_size < (max_blob_memory_space_ - blob_memory_used_ + min(0, ...)),
which
> seems to guarantee that this check will never be true.

That min was incorrect, changed to max.

https://codereview.chromium.org/2055053003/diff/110001/storage/browser/blob/b...
storage/browser/blob/blob_memory_controller.cc:354: size_t size =
base::checked_cast<size_t, uint64_t>(item->item()->length());
On 2016/07/08 23:37:37, Marijn Kruisselbrink wrote:
> why are you explicitly passing the Src template type? That seems like it could
> hide an accidental implicit conversion...

Didn't know I could just do dest.

https://codereview.chromium.org/2055053003/diff/110001/storage/browser/blob/b...
File storage/browser/blob/blob_memory_controller.h (right):

https://codereview.chromium.org/2055053003/diff/110001/storage/browser/blob/b...
storage/browser/blob/blob_memory_controller.h:45: : public
base::SupportsWeakPtr<BlobMemoryController> {
On 2016/07/08 23:37:37, Marijn Kruisselbrink wrote:
> In general using a member base::WeakPtrFactory is preferred over deriving from
> base::SupportsWeakPtr. SupportsWeakPtr has confusing behavior around
destruction
> (if any members of the derived class would hold weak ptrs for example). Also
> nothing else seems to need weak pointers to BlobMemoryController so keeping
weak
> pointers an implementation detail seems nicer anyway.

Done.

https://codereview.chromium.org/2055053003/diff/110001/storage/browser/blob/b...
storage/browser/blob/blob_memory_controller.h:60: typedef
PendingBlobConstructionList::iterator PendingContructionEntry;
On 2016/07/08 23:37:37, Marijn Kruisselbrink wrote:
> Contruction -> Construction

Done.

https://codereview.chromium.org/2055053003/diff/110001/storage/browser/blob/b...
storage/browser/blob/blob_memory_controller.h:137: void
SetMemoryContantsForTesting(size_t max_ipc_memory_size,
On 2016/07/08 23:37:37, Marijn Kruisselbrink wrote:
> Contants -> Constants

Done.

https://codereview.chromium.org/2055053003/diff/110001/storage/browser/blob/b...
storage/browser/blob/blob_memory_controller.h:208: // Lifetime of the
ShareableBlobDataItem objects is handles externally in the
On 2016/07/08 23:37:37, Marijn Kruisselbrink wrote:
> handles -> handled

Done.

https://codereview.chromium.org/2055053003/diff/110001/storage/browser/blob/b...
File storage/browser/blob/blob_storage_context.cc (right):

https://codereview.chromium.org/2055053003/diff/110001/storage/browser/blob/b...
storage/browser/blob/blob_storage_context.cc:190:
entry->dependent_blobs.emplace_back(
On 2016/07/08 23:37:37, Marijn Kruisselbrink wrote:
> nit: Couldn't this just be push_back? Either way you're moving the
> std::unique_ptr CreateHandle returns into the method, and the method then has
to
> move it in place (in the case of push_back), or call the constructor that
> matches the arguments you passed to emplace_back to construct it in place
(which
> in this case would be the move constructor). Emplace is useful to construct
> something in place, but if that constructing is just moving an already
> constructed item it really is more or less equivalent to push_back.

Ah, yes, forgot to not use that.

https://codereview.chromium.org/2055053003/diff/110001/storage/browser/blob/b...
storage/browser/blob/blob_storage_context.cc:255: static uint64_t sNextItemId =
0;
On 2016/07/08 23:37:37, Marijn Kruisselbrink wrote:
> nit: why not move this variable into GetAndIncrementItemId?
> 
> And I wonder if it would maybe make sense to have ShareableBlobDataItem create
> its own ID rather then everybody having to pass the result of
> GetAndIncrementItemId to its constructor?

Sure, changed.

https://codereview.chromium.org/2055053003/diff/110001/storage/browser/blob/b...
storage/browser/blob/blob_storage_context.cc:291: src_data,
static_cast<size_t>(dest_size));
On 2016/07/08 23:37:37, Marijn Kruisselbrink wrote:
> why the cast? dest_size is already a size_t

Done.

https://codereview.chromium.org/2055053003/diff/110001/storage/browser/blob/b...
storage/browser/blob/blob_storage_context.cc:345: if (entry == nullptr) {
On 2016/07/08 23:37:38, Marijn Kruisselbrink wrote:
> nit: if (!entry)?

Done.

Marijn Kruisselbrink

https://codereview.chromium.org/2055053003/diff/110001/storage/browser/blob/blob_flattener.cc File storage/browser/blob/blob_flattener.cc (right): https://codereview.chromium.org/2055053003/diff/110001/storage/browser/blob/blob_flattener.cc#newcode98 storage/browser/blob/blob_flattener.cc:98: output_blob->AppendSharedBlobItem(uuid, std::move(shareable_item)); On 2016/07/11 at 23:33:28, dmurph wrote: > ...

4 years, 5 months ago (2016-07-12 21:33:07 UTC) #15

dmurph

Alrighty! I added a fuzzer style test for BlobStorageContext, which found a bunch of bugs. ...

4 years, 5 months ago (2016-07-14 01:04:31 UTC) #16

michaeln

some more comments i still haven't tackled the BlobMemoryController class which i see as the ...

4 years, 5 months ago (2016-07-14 01:44:44 UTC) #17

dmurph

The CQ bit was checked by dmurph@chromium.org to run a CQ dry run

4 years, 5 months ago (2016-07-14 19:33:24 UTC) #18

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2055053003/190001

4 years, 5 months ago (2016-07-14 19:34:20 UTC) #19

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 5 months ago (2016-07-14 19:58:37 UTC) #20

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_compile_dbg/builds/96572) linux_android_rel_ng on ...

4 years, 5 months ago (2016-07-14 19:58:38 UTC) #21

dmurph

The CQ bit was checked by dmurph@chromium.org to run a CQ dry run

4 years, 5 months ago (2016-07-14 21:43:03 UTC) #22

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2055053003/210001

4 years, 5 months ago (2016-07-14 21:43:34 UTC) #23

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 5 months ago (2016-07-14 22:23:01 UTC) #24

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_android_rel_ng/builds/104309)

4 years, 5 months ago (2016-07-14 22:23:02 UTC) #25

michaeln

I apologize for being so slow in this review. When I first looked, I was ...

4 years, 5 months ago (2016-07-15 02:12:08 UTC) #26

dmurph

Alrighty, I added a ton of tests and I'm replies to half of the comments ...

4 years, 5 months ago (2016-07-15 02:45:27 UTC) #27

Alrighty, I added a ton of tests and I'm replies to half of the comments as
usual, as I need to go home. I'll go through the rest of your comments &
thoughts tomorrow morning. It looks like you're mostly understanding things
correctly.

https://codereview.chromium.org/2055053003/diff/150001/content/browser/blob_s...
File content/browser/blob_storage/blob_dispatcher_host.cc (right):

https://codereview.chromium.org/2055053003/diff/150001/content/browser/blob_s...
content/browser/blob_storage/blob_dispatcher_host.cc:149: // We should only get
these messages for errors.
On 2016/07/14 01:44:42, michaeln wrote:
> I see you're using the same generic status MSG for browser to renderer and
vice
> versa. If we only use this message for renderer->browser to cancel building
> blobs, i'd vote to leave this method name as is, OnCancelBuildingBob even
though
> it's mapped to the more generic STATUS message id.

Done.

https://codereview.chromium.org/2055053003/diff/150001/storage/browser/blob/b...
File storage/browser/blob/blob_memory_controller.cc (right):

https://codereview.chromium.org/2055053003/diff/150001/storage/browser/blob/b...
storage/browser/blob/blob_memory_controller.cc:246: disk_used_ += size_bytes;
On 2016/07/14 01:44:43, michaeln wrote:
> and this "allocates" memory on disk

Added more documentation.

https://codereview.chromium.org/2055053003/diff/150001/storage/browser/blob/b...
storage/browser/blob/blob_memory_controller.cc:278: blob_memory_used_ +=
memory_size;
On 2016/07/14 01:44:43, michaeln wrote:
> ok, this method "allocates" the memory

Added more documentation.

https://codereview.chromium.org/2055053003/diff/150001/storage/browser/blob/b...
File storage/browser/blob/blob_memory_controller.h (right):

https://codereview.chromium.org/2055053003/diff/150001/storage/browser/blob/b...
storage/browser/blob/blob_memory_controller.h:59: typedef
PendingBlobConstructionList::iterator PendingConstructionEntry;
On 2016/07/14 01:44:43, michaeln wrote:
> This typedef is odd?

I need that entry defined as  I return it below.

https://codereview.chromium.org/2055053003/diff/150001/storage/browser/blob/b...
storage/browser/blob/blob_memory_controller.h:77:
scoped_refptr<base::SequencedWorkerPool> file_worker_pool);
On 2016/07/14 01:44:43, michaeln wrote:
> +1 putting these tasks on the worker pool instead of a well known thread.
> 
> Instead of passing the SequencedWorkerPool itself, all thats really needed is
a
> TaskRunner backed by the pool. This makes testing easier too.
> 
> It might be a good idea to have this class use a SequentedTaskRunner to ensure
> that the blob system does not tie up all of the threads of the pool at any
given
> time.
> 

Done.

https://codereview.chromium.org/2055053003/diff/150001/storage/browser/blob/b...
storage/browser/blob/blob_memory_controller.h:94: void
CreateTemporaryFileForRenderer(
On 2016/07/14 01:44:43, michaeln wrote:
> naming nit: "ForRenderer" isn't really needed

Done.

https://codereview.chromium.org/2055053003/diff/150001/storage/browser/blob/b...
storage/browser/blob/blob_memory_controller.h:144: size_t max_blob_memory_space,
On 2016/07/14 01:44:43, michaeln wrote:
> what's the difference between max_blob_in_memory_size and
max_blob_memory_space?

Removed. One was the addition of max blob size and in flight size.

https://codereview.chromium.org/2055053003/diff/150001/storage/browser/blob/b...
storage/browser/blob/blob_memory_controller.h:148: uint64_t max_file_size) {
On 2016/07/14 01:44:43, michaeln wrote:
> is there a reason the "min" value has "page" in the name and the max value
does
> not?

The min size is used for paging, and the max size is used for files for the
renderer.

https://codereview.chromium.org/2055053003/diff/150001/storage/browser/blob/b...
storage/browser/blob/blob_memory_controller.h:200: // Constants.
On 2016/07/14 01:44:43, michaeln wrote:
> A struct for these constant/config/settings would be nice.

put a TODO.

https://codereview.chromium.org/2055053003/diff/150001/storage/browser/blob/b...
storage/browser/blob/blob_memory_controller.h:216: RecentItemsCache
recent_item_cache_;
On 2016/07/14 01:44:43, michaeln wrote:
> The using alias for RecentItemsCache isn't needed, the alias is only used on
> this line. It'd be a little clearer to use the base::MRUCache<> inline, the
> comment referring to the rawptr would make more sense.

Done.

https://codereview.chromium.org/2055053003/diff/150001/storage/browser/blob/b...
File storage/browser/blob/blob_slice.h (right):

https://codereview.chromium.org/2055053003/diff/150001/storage/browser/blob/b...
storage/browser/blob/blob_slice.h:26: size_t copying_memory_size = 0;
On 2016/07/14 01:44:43, michaeln wrote:
> a comment about what this is would be nice
> 
> we can't share a partial data item, so we may have to copy the relevant parts
> from the first and last source items
> 
> alternatively, we might not need the data member, a method could compute it
> based by summing the size of the first and last dest_items (when first_source
> and last_source are non-null)

Done.

https://codereview.chromium.org/2055053003/diff/150001/storage/browser/blob/b...
storage/browser/blob/blob_slice.h:29: // in the original blob. See
first_item_slice_offset() and first_item_index().
On 2016/07/14 01:44:43, michaeln wrote:
> comment is stale

Done.

https://codereview.chromium.org/2055053003/diff/150001/storage/browser/blob/b...
storage/browser/blob/blob_slice.h:33: bool has_sliced_last_memory_item = false;
On 2016/07/14 01:44:43, michaeln wrote:
> Since we have the first_source_item and last_source_item data members, i think
> the bools are redundant. If i'm reading it right, the refptrs are set only
when
> we have a respective "sliced" memory item.

Done.

dmurph

Description was changed from ========== [Blob] Disk support for blob storage. This is the final ...

4 years, 5 months ago (2016-07-15 18:01:17 UTC) #28

Description was changed from

==========
[Blob] Disk support for blob storage.

This is the final patch to enable disk support for Blob storage. This includes:
 * Saving large blobs to disk right away
 * Paging old blobs to disk

STATUS:
Debugging logging & lots of DCHECKs are present. Tests are commented out.


BUG=375297
==========

to

==========
[Blob] Disk support for blob storage.

This is the final patch to enable disk support for Blob storage. This
includes:
 * Saving large blobs to disk right away
 * Paging old blobs to disk

The new system operates like the following:
* Blobs are registered with metadata & descriptions of items in one
  message/call.
* The async transport host asks the memory manager how it should
  transfer the data, and from that creates a BlobDataBuilder.
* This builder is handed to the BlobStorageContext in BuildBlob, along
  with a callback that the transport host will wait for before kicking
  off transport.
* The BuildBlob call immediately constructs the internal blob data
  with all ShareableBlobDataItems that will eventually be populated.
  This work includes:
  * Decomposing all blobs we depend upon and either copying over or
    creating new shareable items for partial data items.
  * The new shareable item data will be copied over after the
    dependent blob is complete, and we have enough memory for it.
  * This involves using the new BlobFlattener and BlobSlice classes.
* This results in us knowing the total amount of memory we need to
  build this blob (new memory + copies from other blobs). We ask the
  memory controller for this memory, and it will call us back when it
  is available. When this occurs we can tell the transport layer to
  start sending content if any was pending.
* We now wait for the following before completing our blob:
  1. We have enough room to fit the blob in memory.
  2. The transport layer has finished transporting any pending
     content.
  3. Blobs that we are referencing have finished populating.
* When all of these conditions have been satisfied, we:
  1. Perform any copies that we need to do from dependent blobs.
  2. Change our state to DONE.
  3. Report to listeners that we've finished constructing.

Here are some other slightly special cases we need to handle:
* If our blob size is larger than in-memory size, then we give file
  handles to the renderer to populate. Before these files are created,
  our InternalBlobData's representation of these items is a temporary
  file name constant. If any new blob references this blob, we can't
  treat that slice like a regular file, and instead treat it like a
  'copy', where we copy over the file reference during step 1 of blob
  completion.
* Similarily, during step 1 of completion the referenced blob's data
  could have been paged to disk while we were constructing. We do the
  same thing as above but we make sure to also call FreeMemory on our
  BlobMemoryController, as we had previously allocated memory for that
  item.

STATUS:
Debugging logging & lots of DCHECKs are present.

BUG=375297
==========

dmurph

Michael's analysis was slightly off, so I updated the description to outline how blobs are ...

4 years, 5 months ago (2016-07-15 18:11:05 UTC) #29

dmurph

Alright, so I made the enum change to include the two pending states. This simplifies ...

4 years, 5 months ago (2016-07-15 20:18:16 UTC) #30

kinuko

Still haven't gotten half way through, but I have more comments now with possibly a ...

4 years, 5 months ago (2016-07-17 16:15:47 UTC) #31

dmurph

I still need to do the de-friendify of tests, but replied to all other comments. ...

4 years, 5 months ago (2016-07-19 02:26:28 UTC) #32

dmurph

Description was changed from ========== [Blob] Disk support for blob storage. This is the final ...

4 years, 5 months ago (2016-07-19 18:40:34 UTC) #33

Description was changed from

==========
[Blob] Disk support for blob storage.

This is the final patch to enable disk support for Blob storage. This
includes:
 * Saving large blobs to disk right away
 * Paging old blobs to disk

The new system operates like the following:
* Blobs are registered with metadata & descriptions of items in one
  message/call.
* The async transport host asks the memory manager how it should
  transfer the data, and from that creates a BlobDataBuilder.
* This builder is handed to the BlobStorageContext in BuildBlob, along
  with a callback that the transport host will wait for before kicking
  off transport.
* The BuildBlob call immediately constructs the internal blob data
  with all ShareableBlobDataItems that will eventually be populated.
  This work includes:
  * Decomposing all blobs we depend upon and either copying over or
    creating new shareable items for partial data items.
  * The new shareable item data will be copied over after the
    dependent blob is complete, and we have enough memory for it.
  * This involves using the new BlobFlattener and BlobSlice classes.
* This results in us knowing the total amount of memory we need to
  build this blob (new memory + copies from other blobs). We ask the
  memory controller for this memory, and it will call us back when it
  is available. When this occurs we can tell the transport layer to
  start sending content if any was pending.
* We now wait for the following before completing our blob:
  1. We have enough room to fit the blob in memory.
  2. The transport layer has finished transporting any pending
     content.
  3. Blobs that we are referencing have finished populating.
* When all of these conditions have been satisfied, we:
  1. Perform any copies that we need to do from dependent blobs.
  2. Change our state to DONE.
  3. Report to listeners that we've finished constructing.

Here are some other slightly special cases we need to handle:
* If our blob size is larger than in-memory size, then we give file
  handles to the renderer to populate. Before these files are created,
  our InternalBlobData's representation of these items is a temporary
  file name constant. If any new blob references this blob, we can't
  treat that slice like a regular file, and instead treat it like a
  'copy', where we copy over the file reference during step 1 of blob
  completion.
* Similarily, during step 1 of completion the referenced blob's data
  could have been paged to disk while we were constructing. We do the
  same thing as above but we make sure to also call FreeMemory on our
  BlobMemoryController, as we had previously allocated memory for that
  item.

STATUS:
Debugging logging & lots of DCHECKs are present.

BUG=375297
==========

to

==========
[Blob] Disk support for blob storage.

This is the final patch to enable disk support for Blob storage. This
includes:
 * Saving large blobs to disk right away
 * Paging old blobs to disk

The new system operates like the following:
* Blobs are registered with metadata & descriptions of items in one
  message/call.
* The async transport host asks the memory manager how it should
  transfer the data, and from that creates a BlobDataBuilder.
* This builder is handed to the BlobStorageContext in BuildBlob, along
  with a callback that the transport host will wait for before kicking
  off transport.
* The BuildBlob call immediately constructs the internal blob data
  with all ShareableBlobDataItems that will eventually be populated.
  This work includes:
  * Decomposing all blobs we depend upon and either copying over or
    creating new shareable items for partial data items.
  * The new shareable item data will be copied over after the
    dependent blob is complete, and we have enough memory for it.
  * This involves using the new BlobFlattener and BlobSlice classes.
* This results in us knowing the total amount of memory we need to
  build this blob (new memory + copies from other blobs). We ask the
  memory controller for this memory, and it will call us back when it
  is available. When this occurs we can tell the transport layer to
  start sending content if any was pending.
* We now wait for the following before completing our blob:
  1. We have enough room to fit the blob in memory.
  2. The transport layer has finished transporting any pending
     content.
  3. Blobs that we are referencing have finished populating.
* When all of these conditions have been satisfied, we:
  1. Perform any copies that we need to do from dependent blobs.
  2. Change our state to DONE.
  3. Report to listeners that we've finished constructing.

Here are some other slightly special cases we need to handle:
* If our blob size is larger than in-memory size, then we give file
  handles to the renderer to populate. Before these files are created,
  our InternalBlobData's representation of these items is a temporary
  file name constant. If any new blob references this blob, we can't
  treat that slice like a regular file, and instead treat it like a
  'copy', where we copy over the file reference during step 1 of blob
  completion.
* Similarily, during step 1 of completion the referenced blob's data
  could have been paged to disk while we were constructing. We do the
  same thing as above but we make sure to also call FreeMemory on our
  BlobMemoryController, as we had previously allocated memory for that
  item.

STATUS:
Debugging logging & lots of DCHECKs are present.

BUG=375297,612261
==========

dmurph

The CQ bit was checked by dmurph@chromium.org to run a CQ dry run

4 years, 5 months ago (2016-07-19 18:51:36 UTC) #34

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2055053003/290001

4 years, 5 months ago (2016-07-19 18:52:10 UTC) #35

dmurph

dmurph@chromium.org changed reviewers: + falken@chromium.org

4 years, 5 months ago (2016-07-19 18:53:10 UTC) #36

dmurph

I added a necessary change to ServiceURLRequestJob (and removed some unneeded code) that fetches file ...

4 years, 5 months ago (2016-07-19 18:53:11 UTC) #37

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 5 months ago (2016-07-19 19:38:21 UTC) #38

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/55192)

4 years, 5 months ago (2016-07-19 19:38:23 UTC) #39

dmurph

Kinuko: Michael & I chatted a lot today, and he had some good feedback on ...

4 years, 5 months ago (2016-07-19 23:56:36 UTC) #40

kinuko

On 2016/07/19 23:56:36, dmurph wrote: > Kinuko: > > Michael & I chatted a lot ...

4 years, 5 months ago (2016-07-20 08:07:57 UTC) #41

falken

service worker lgtm https://codereview.chromium.org/2055053003/diff/290001/content/browser/service_worker/service_worker_url_request_job.cc File content/browser/service_worker/service_worker_url_request_job.cc (left): https://codereview.chromium.org/2055053003/diff/290001/content/browser/service_worker/service_worker_url_request_job.cc#oldcode621 content/browser/service_worker/service_worker_url_request_job.cc:621: total_size = std::numeric_limits<uint64_t>::max(); Seems we can ...

4 years, 5 months ago (2016-07-22 04:22:19 UTC) #42

dmurph

The CQ bit was checked by dmurph@chromium.org to run a CQ dry run

4 years, 4 months ago (2016-08-02 00:51:29 UTC) #43

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2055053003/330001

4 years, 4 months ago (2016-08-02 00:51:47 UTC) #44

dmurph

Description was changed from ========== [Blob] Disk support for blob storage. This is the final ...

4 years, 4 months ago (2016-08-02 00:55:13 UTC) #45

Description was changed from

==========
[Blob] Disk support for blob storage.

This is the final patch to enable disk support for Blob storage. This
includes:
 * Saving large blobs to disk right away
 * Paging old blobs to disk

The new system operates like the following:
* Blobs are registered with metadata & descriptions of items in one
  message/call.
* The async transport host asks the memory manager how it should
  transfer the data, and from that creates a BlobDataBuilder.
* This builder is handed to the BlobStorageContext in BuildBlob, along
  with a callback that the transport host will wait for before kicking
  off transport.
* The BuildBlob call immediately constructs the internal blob data
  with all ShareableBlobDataItems that will eventually be populated.
  This work includes:
  * Decomposing all blobs we depend upon and either copying over or
    creating new shareable items for partial data items.
  * The new shareable item data will be copied over after the
    dependent blob is complete, and we have enough memory for it.
  * This involves using the new BlobFlattener and BlobSlice classes.
* This results in us knowing the total amount of memory we need to
  build this blob (new memory + copies from other blobs). We ask the
  memory controller for this memory, and it will call us back when it
  is available. When this occurs we can tell the transport layer to
  start sending content if any was pending.
* We now wait for the following before completing our blob:
  1. We have enough room to fit the blob in memory.
  2. The transport layer has finished transporting any pending
     content.
  3. Blobs that we are referencing have finished populating.
* When all of these conditions have been satisfied, we:
  1. Perform any copies that we need to do from dependent blobs.
  2. Change our state to DONE.
  3. Report to listeners that we've finished constructing.

Here are some other slightly special cases we need to handle:
* If our blob size is larger than in-memory size, then we give file
  handles to the renderer to populate. Before these files are created,
  our InternalBlobData's representation of these items is a temporary
  file name constant. If any new blob references this blob, we can't
  treat that slice like a regular file, and instead treat it like a
  'copy', where we copy over the file reference during step 1 of blob
  completion.
* Similarily, during step 1 of completion the referenced blob's data
  could have been paged to disk while we were constructing. We do the
  same thing as above but we make sure to also call FreeMemory on our
  BlobMemoryController, as we had previously allocated memory for that
  item.

STATUS:
Debugging logging & lots of DCHECKs are present.

BUG=375297,612261
==========

to

==========
[Blob] Disk support for blob storage.

This is the final patch to enable disk support for Blob storage. This
includes:
 * Saving large blobs to disk right away
 * Paging old blobs to disk

The new system operates like the following:
* Blobs are registered with metadata & descriptions of items in one
  message/call.
* The async transport host asks the memory manager how it should
  transfer the data, and from that creates a BlobDataBuilder.
* This builder is handed to the BlobStorageContext in BuildBlob, along
  with a callback that the transport host will wait for before kicking
  off transport.
* The BuildBlob call immediately constructs the internal blob data
  with all ShareableBlobDataItems that will eventually be populated.
  This work includes:
  * Decomposing all blobs we depend upon and either copying over or
    creating new shareable items for partial data items.
  * The new shareable item data will be copied over after the
    dependent blob is complete, and we have enough memory for it.
  * This involves using the new BlobFlattener and BlobSlice stucts.
* This results in us knowing the total amount of memory we need to
  build this blob (new memory + copies from other blobs). We ask the
  memory controller to reserve quota for these items in separate
  calls.
* We now wait for the following before completing our blob:
  1. Blobs that we referenced are finished building,
  2. We have been granted memory quota for any copies from referenced
     blobs, and
  3. We have been granted memory or disk quota for blob memory coming
     from the renderer.
* When all of these conditions have been satisfied we finish the blob:
  1. Perform any copies that we need to do from referenced blobs.
  2. Change our state to DONE.
  3. Report to listeners that we've finished constructing.

New data stored:
* We've combined error and states into one BlobStatus.
* Building state has been consolidated into a BuildingState struct in
  InternalBlobData.
* ShareableBlobDataItems now have state so the memory controller can
  keep track of which items to grant/release quota.

STATUS:
Debugging logging & lots of DCHECKs are present.

BUG=375297,612261
==========

dmurph

The CQ bit was checked by dmurph@chromium.org to run a CQ dry run

4 years, 4 months ago (2016-08-02 00:55:55 UTC) #46

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2055053003/350001

4 years, 4 months ago (2016-08-02 00:56:14 UTC) #47

dmurph

Hello everyone! I finished the refactor that Michael discussed with me, and added a ton ...

4 years, 4 months ago (2016-08-02 00:58:02 UTC) #48

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 4 months ago (2016-08-02 01:08:35 UTC) #49

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-generic_chromium_compile_only_ng/builds/176386)

4 years, 4 months ago (2016-08-02 01:08:38 UTC) #50

dmurph

dmurph@chromium.org changed reviewers: + costan@google.com

4 years, 4 months ago (2016-08-05 20:46:37 UTC) #51

pwnall

pwnall@chromium.org changed reviewers: + pwnall@chromium.org - costan@google.com

4 years, 4 months ago (2016-08-05 22:19:43 UTC) #53

Marijn Kruisselbrink

Didn't look at everything yet, but some comments anyway: https://codereview.chromium.org/2055053003/diff/350001/content/browser/service_worker/service_worker_url_request_job.cc File content/browser/service_worker/service_worker_url_request_job.cc (right): https://codereview.chromium.org/2055053003/diff/350001/content/browser/service_worker/service_worker_url_request_job.cc#newcode176 content/browser/service_worker/service_worker_url_request_job.cc:176: ...

4 years, 4 months ago (2016-08-05 23:23:55 UTC) #54

michaeln

here's a bunch of comments, the cl is very large and unwieldy, breaking it up ...

4 years, 4 months ago (2016-08-15 22:44:45 UTC) #55

pwnall

https://codereview.chromium.org/2055053003/diff/350001/storage/common/blob_storage/blob_storage_constants.h File storage/common/blob_storage/blob_storage_constants.h (right): https://codereview.chromium.org/2055053003/diff/350001/storage/common/blob_storage/blob_storage_constants.h#newcode22 storage/common/blob_storage/blob_storage_constants.h:22: const uint64_t kBlobStorageMinFileSizeBytes = 5 * 1024 * 1024; ...

4 years, 4 months ago (2016-08-17 22:01:55 UTC) #56

michaeln

Here's a rough sketch of what an independent cut at 'controller' api might look like? ...

4 years, 4 months ago (2016-08-18 21:54:01 UTC) #57

michaeln

and in a subsequent cl, ShareableBlobDataItems would own block a ReservedSpace if needed On 2016/08/18 ...

4 years, 4 months ago (2016-08-18 22:05:30 UTC) #58

dmurph

I got through most of the comments, end of the day so I wanted to ...

4 years, 4 months ago (2016-08-19 00:18:34 UTC) #59

I got through most of the comments, end of the day so I wanted to send this out.

I think it's looking MUCH better, especially since the file/memory split in the
memory controller.

I think renaming the methods in BlobStorageContext is a good idea Michael, just
didn't get around to that today.  The biggest issue there is that
CreatePendingBlob isn't quite accurate, as the blob can complete right there
immediately.

Status of the patch: I didn't update the tests yet (they dont' all build) but
the layout tests are all working. I wanted to finalize the architecture a little
more before updating those tests.

I updated the BlobStorageContext BuildBlob code a bit to be more clear, as well
as the BuildingState. Let me know what you think please!!!!

Thanks,
Daniel

https://codereview.chromium.org/2055053003/diff/350001/content/browser/blob_s...
File content/browser/blob_storage/blob_dispatcher_host.cc (right):

https://codereview.chromium.org/2055053003/diff/350001/content/browser/blob_s...
content/browser/blob_storage/blob_dispatcher_host.cc:86:
base::Bind(&BlobDispatcherHost::MaybeSendFinalBlobStatus,
On 2016/08/15 22:44:42, michaeln wrote:
> Seems odd that the builder is expected to call this closure upon completion in
> some cases, yet not in others, such that this class also has to call it in
> various places. It's hard to know whos supposed to do what?
> 
> I'm not sure if this is possible/practical, but if the builder were always
> responsible for calling the completion callback or never responsible for it so
> the DispatcherHost were always repsonsible for sending the 'done' message,
that
> could make a cleaner division of labor.

Done.

https://codereview.chromium.org/2055053003/diff/350001/content/browser/blob_s...
content/browser/blob_storage/blob_dispatcher_host.cc:203: uuid,
BlobStatus::ERR_BLOB_DEREFERENCED_WHILE_BUILDING);
On 2016/08/15 22:44:42, michaeln wrote:
> at callsites where there's no maybe about it, maybe...
> 
>    Send(new BlobStorageMsg_SendBlobStatus(
>        uuid, ERR_BLOB_DEREFERENCED_WHILE_BUILDING));

No more maybe.

https://codereview.chromium.org/2055053003/diff/350001/content/browser/servic...
File content/browser/service_worker/service_worker_url_request_job.cc (right):

https://codereview.chromium.org/2055053003/diff/350001/content/browser/servic...
content/browser/service_worker/service_worker_url_request_job.cc:119: if
(file.error_details() != base::File::FILE_OK)
On 2016/08/15 22:44:43, michaeln wrote:
> base:: has a nice utility to GetFileInfo given just a path. so i don't think
we
> need a base::File instance here.

Done.

https://codereview.chromium.org/2055053003/diff/350001/content/browser/servic...
content/browser/service_worker/service_worker_url_request_job.cc:136: class
ServiceWorkerURLRequestJob::BlobFilesWaiter {
On 2016/08/15 22:44:43, michaeln wrote:
> Oh, the purpose of this class is now completely different.
> 
> We used to have to wait for constituent blob elements in the RequestBody to
> complete prior to "resolving" it into a RequestBlob.
> 
> But now we have to get the size of any constituent file elements in the
> RequestBody because we have to know the size in order to create the
RequestBlob.
> 
> A larger name change might be good since it doesn't really 'wait' for an
> external agency to complete something.
> 
> class FileSizeResolver?
> 
> The impl might be easier to compose with a helper function like...
> 
> int64_t GetFileSizeOnBlockingPool(const base::FilePath& file_path) {
>   int64_t size = 0;
>   bool success = base::GetFileSize(file_path, &size);
>   return success ? size : -1;
> }
> 
> ++num_pending_file_size_tasks_;
> PostTaskAndReplyWithResult(
>     runner,
>     base::Bind(&GetFileSizeOnBlockingPool, path),
>     base::Bind(&OnGotFileSize, GetWeakPtr(), element_index));

Done.

https://codereview.chromium.org/2055053003/diff/350001/content/browser/servic...
content/browser/service_worker/service_worker_url_request_job.cc:162: for (const
ResourceRequestBodyImpl::Element& element : *body_->elements()) {
On 2016/08/15 22:44:42, michaeln wrote:
> might be good place for const auto&

Done.

https://codereview.chromium.org/2055053003/diff/350001/content/browser/servic...
content/browser/service_worker/service_worker_url_request_job.cc:176:
base::Bind(&PopulateUnknownFileLengths, body_->elements_mutable()),
On 2016/08/15 22:44:43, michaeln wrote:
> On 2016/08/05 23:23:54, Marijn Kruisselbrink wrote:
> > Is this safe? body_->elements_mutable() is just a raw pointer, so you're
> > seemingly relying on something else to keep body_ alive. But on the next
> > callback you're using a weak pointer to |this|, so it can't be the reference
> > this holds to body_ that keeps body_ alive.
> 
> what he said

Now we protect this by accessing the elements only in this class.

https://codereview.chromium.org/2055053003/diff/350001/content/browser/servic...
content/browser/service_worker/service_worker_url_request_job.cc:595:
storage::BlobDataBuilder blob_builder(uuid);
On 2016/08/15 22:44:43, michaeln wrote:
> you could get rid of the uuid local and a line of code

Done.

https://codereview.chromium.org/2055053003/diff/350001/content/browser/servic...
File content/browser/service_worker/service_worker_url_request_job.h (right):

https://codereview.chromium.org/2055053003/diff/350001/content/browser/servic...
content/browser/service_worker/service_worker_url_request_job.h:304:
std::unique_ptr<BlobFilesWaiter> blob_construction_waiter_;
On 2016/08/15 22:44:43, michaeln wrote:
> maybe chance the name of the member too

Done.

https://codereview.chromium.org/2055053003/diff/350001/content/child/blob_sto...
File content/child/blob_storage/blob_message_filter.cc (right):

https://codereview.chromium.org/2055053003/diff/350001/content/child/blob_sto...
content/child/blob_storage/blob_message_filter.cc:38:
IPC_MESSAGE_HANDLER(BlobStorageMsg_SendBlobStatus, OnBlobStatus)
On 2016/08/15 22:44:43, michaeln wrote:
> maybe call this OnBlobFinalStatus here and throughout on the client side to
make
> it clear that we won't see one of the pending states in here

Done.

https://codereview.chromium.org/2055053003/diff/350001/content/child/blob_sto...
File content/child/blob_storage/blob_transport_controller.cc (right):

https://codereview.chromium.org/2055053003/diff/350001/content/child/blob_sto...
content/child/blob_storage/blob_transport_controller.cc:118:
std::pair<BlobStatus, std::vector<BlobItemBytesResponse>> WriteDiskRequests(
On 2016/08/15 22:44:43, michaeln wrote:
> oh, this might be a good place for a base::Optional<>, a missing value means
> writing failed

Done.

https://codereview.chromium.org/2055053003/diff/350001/storage/browser/blob/b...
File storage/browser/blob/blob_async_builder_host.cc (right):

https://codereview.chromium.org/2055053003/diff/350001/storage/browser/blob/b...
storage/browser/blob/blob_async_builder_host.cc:69: const BlobStatusCallback&
status_callback) {
On 2016/08/15 22:44:43, michaeln wrote:
> does this get called more than once? if not, calling it 'completion_callback'
> would definitely help
> 
> actually that might help in other places too

Done.

https://codereview.chromium.org/2055053003/diff/350001/storage/browser/blob/b...
storage/browser/blob/blob_async_builder_host.cc:144: state_ptr->status_callback
= status_callback;
On 2016/08/15 22:44:43, michaeln wrote:
> comletion callback?

Done.

https://codereview.chromium.org/2055053003/diff/350001/storage/browser/blob/b...
storage/browser/blob/blob_async_builder_host.cc:147: // memory, and we create
the temporary files right away.
On 2016/08/15 22:44:43, michaeln wrote:
> confusing comment since the block of code under this comment does not do what
> the comment says? maybe this comment should go elsewhere?

Done.

https://codereview.chromium.org/2055053003/diff/350001/storage/browser/blob/b...
File storage/browser/blob/blob_async_builder_host.h (right):

https://codereview.chromium.org/2055053003/diff/350001/storage/browser/blob/b...
storage/browser/blob/blob_async_builder_host.h:43: // * hold all blobs that are
currently being built asynchronously for a child
On 2016/08/15 22:44:43, michaeln wrote:
> I'm not sure the detailed bullets add value. They try to describe inner
details
> but they don't quite do it. It adds up to being less clear really because the
> reader may think the understand based on the comment, but the details are
> actually different.
> 
> For example "hold all blobs". Could easily thing that the class references to
> blobs being built, but that's not right.

Done.

https://codereview.chromium.org/2055053003/diff/350001/storage/browser/blob/b...
storage/browser/blob/blob_async_builder_host.h:46: //   and uses the BlobStatus
return value to signify other results,
On 2016/08/15 22:44:43, michaeln wrote:
> i don't understand the comment on this line?

Done.

https://codereview.chromium.org/2055053003/diff/350001/storage/browser/blob/b...
storage/browser/blob/blob_async_builder_host.h:51: class STORAGE_EXPORT
BlobAsyncBuilderHost {
On 2016/08/15 22:44:43, michaeln wrote:
> Food for thought, this talks to the BlobTransportController on the renderer
> side. A real common naming convention would make this class the
> BlobTransportControllerHost.

Hmmmmmmmmmm

https://codereview.chromium.org/2055053003/diff/350001/storage/browser/blob/b...
storage/browser/blob/blob_async_builder_host.h:72: // * PENDING_MEMORY_QUOTA if
we're waiting for memory quota,
On 2016/08/05 23:23:54, Marijn Kruisselbrink wrote:
> there's only one PENDING status now, right?

Done.

https://codereview.chromium.org/2055053003/diff/350001/storage/browser/blob/b...
File storage/browser/blob/blob_data_builder.cc (right):

https://codereview.chromium.org/2055053003/diff/350001/storage/browser/blob/b...
storage/browser/blob/blob_data_builder.cc:28:
"kFakeFilenameToBeChangedByPopulateFutureFile";
On 2016/08/15 22:44:43, michaeln wrote:
> would be safer to use a shorter filename to avoid path length problems
> "_future_name_"

Done.

https://codereview.chromium.org/2055053003/diff/350001/storage/browser/blob/b...
storage/browser/blob/blob_data_builder.cc:30: bool
BlobDataBuilder::IsTemporaryFileItem(const DataElement& element) {
On 2016/08/15 22:44:43, michaeln wrote:
> i think this can be simplified, element.path().BaseName() gives what precedes
> the '.' in the last component of the path
> 
> return element.path().BaseName() == kFutureFilename;

Done.

https://codereview.chromium.org/2055053003/diff/350001/storage/browser/blob/b...
storage/browser/blob/blob_data_builder.cc:34:
element.path().MaybeAsASCII().substr(0, prefix.size()) == prefix;
On 2016/08/05 23:23:54, Marijn Kruisselbrink wrote:
> Why call element.path().MaybeAsASCII() if you already have saved that as path
> above? And isn't this just base::StartsWith(path, prefix,
> base::CompareCase::SENSITIVE) (which would avoid the substring copy, and the
> std::string copy of prefix since it just takes a StringPiece).

Done.

https://codereview.chromium.org/2055053003/diff/350001/storage/browser/blob/b...
storage/browser/blob/blob_data_builder.cc:37: std::string
BlobDataBuilder::GetTemporaryFileID(const DataElement& element) {
On 2016/08/15 22:44:43, michaeln wrote:
> maybe DCHECK(IsFutureFileItem(element))

Done.

https://codereview.chromium.org/2055053003/diff/350001/storage/browser/blob/b...
storage/browser/blob/blob_data_builder.cc:131:
std::string(kAppendFutureFileTemporaryFileName))
On 2016/08/05 23:23:54, Marijn Kruisselbrink wrote:
> Why the conversion to a std::string? FromUTF8Unsafe takes a StringPiece, so
> passing the const char* directly should work fine.

Done.

https://codereview.chromium.org/2055053003/diff/350001/storage/browser/blob/b...
storage/browser/blob/blob_data_builder.cc:135: return items_.size() - 1;
On 2016/08/15 22:44:43, michaeln wrote:
> Why does file_id need to be passed in? the class comment says it has to be
> unique within the builder and the builder can most easily maintain that
> constraint.
> 
> FilePath name = FormFutureFilename(items_.size());
> elem->SetXYZ(name, ...);
> 
> The 'id' can be the index of the element

The id can be reused between elements. I'm putting elements 1, 2, 3 in file 0,
and 7, 9, 10 in file 1.

https://codereview.chromium.org/2055053003/diff/350001/storage/browser/blob/b...
File storage/browser/blob/blob_data_builder.h (right):

https://codereview.chromium.org/2055053003/diff/350001/storage/browser/blob/b...
storage/browser/blob/blob_data_builder.h:39: static const char
kAppendFutureFileTemporaryFileName[];
On 2016/08/15 22:44:44, michaeln wrote:
> please use shorter symbols, kFutureFileName[]

Done.

https://codereview.chromium.org/2055053003/diff/350001/storage/browser/blob/b...
storage/browser/blob/blob_data_builder.h:42: static bool
IsTemporaryFileItem(const DataElement& element);
On 2016/08/15 22:44:44, michaeln wrote:
> please use consistent terminology, "future" or "temporary", pick one, i like
> "future"

Done.

https://codereview.chromium.org/2055053003/diff/350001/storage/browser/blob/b...
storage/browser/blob/blob_data_builder.h:43: // Generated from |file_id| given
in AppendFutureFile.
On 2016/08/15 22:44:44, michaeln wrote:
> what does this function do? oh?

Done.

https://codereview.chromium.org/2055053003/diff/350001/storage/browser/blob/b...
storage/browser/blob/blob_data_builder.h:44: static std::string
GetTemporaryFileID(const DataElement& element);
On 2016/08/15 22:44:44, michaeln wrote:
> there's mysteriousness around file_ids and future files that could be
clarified
> some?
> 
> FilePath FormFutureFilename(id);
> string ExtractFutureFileId(path);
> 
> can the file_id types be an int?

Done.

https://codereview.chromium.org/2055053003/diff/350001/storage/browser/blob/b...
File storage/browser/blob/blob_flattener.cc (right):

https://codereview.chromium.org/2055053003/diff/350001/storage/browser/blob/b...
storage/browser/blob/blob_flattener.cc:104: input_element.length());
On 2016/08/05 23:23:54, Marijn Kruisselbrink wrote:
> Should this use input_element.length() (which can be kUnknownSize) or length
> (which is guaranteed at this point to have a known value, and usually is the
> same as input_element.length())?

Good idea. Thanks.

https://codereview.chromium.org/2055053003/diff/350001/storage/browser/blob/b...
storage/browser/blob/blob_flattener.cc:143: total_size += length;
On 2016/08/05 23:23:54, Marijn Kruisselbrink wrote:
> It seems a bit odd to sometimes be adding kUnknownSize to total_size (and
above
> to transport_quote_needed). I suppose since we'll error out later if that
> happened with more than one total input item it all works out fine...

Yeah, since total_size is a safe math variable anyways, I think this should be
fine. We'll error out, or have an invalid total size, both of which are caught.

https://codereview.chromium.org/2055053003/diff/350001/storage/browser/blob/b...
storage/browser/blob/blob_flattener.cc:152: status = contains_pending_content ?
BlobStatus::PENDING : BlobStatus::DONE;
On 2016/08/05 23:23:54, Marijn Kruisselbrink wrote:
> Currently you have whoever called BlobFlattener check for IsValid on the
various
> checked numbers. Would it make sense to move those checks to here and set
status
> appropriately, thereby making calling code simpler?

Done.

https://codereview.chromium.org/2055053003/diff/350001/storage/browser/blob/b...
File storage/browser/blob/blob_memory_controller.cc (right):

https://codereview.chromium.org/2055053003/diff/350001/storage/browser/blob/b...
storage/browser/blob/blob_memory_controller.cc:187: // We check both
individually, as when something is stored straight to disk
On 2016/08/05 23:23:54, Marijn Kruisselbrink wrote:
> Not sure what "both" refers to here? You only check one thing in this if...
> (edit: ah, CanFitInSystem does two checks. And you have a similar comment
there,
> although this comment is a bit clearer in why you need two separate checks. it
> just is really confusing in this place).

Done.

https://codereview.chromium.org/2055053003/diff/350001/storage/browser/blob/b...
storage/browser/blob/blob_memory_controller.cc:223: bool files = false;
On 2016/08/05 23:23:55, Marijn Kruisselbrink wrote:
> What is this |files| thing, and what is it used for? You seemed to just check
> total_files_size_needed != 0 when you're checking for presence of files.

Removed.

https://codereview.chromium.org/2055053003/diff/350001/storage/browser/blob/b...
storage/browser/blob/blob_memory_controller.cc:365: if (entry.memory_quota_entry
== blobs_waiting_for_paging_.end())
On 2016/08/05 23:23:54, Marijn Kruisselbrink wrote:
> Under what circumstances would this comparison be true? I'm not entirely sure
> what conditions you're trying to detect here. I don't think you ever create an
> entry with end() as value, so this should never compare as true...

Simplified this by separating out memory and file quota requests. Now it's end()
if the request wasn't async.

https://codereview.chromium.org/2055053003/diff/350001/storage/browser/blob/b...
storage/browser/blob/blob_memory_controller.cc:367: const std::pair<size_t,
QuotaRequestCallback> pair =
On 2016/08/05 23:23:54, Marijn Kruisselbrink wrote:
> Not sure if this two-line variable declaration gains you anything over just
> using entry.memory_quota_entry->first on the next line...

Done.

https://codereview.chromium.org/2055053003/diff/350001/storage/browser/blob/b...
storage/browser/blob/blob_memory_controller.cc:379: base::hash_set<uint64_t>
visited_items;
On 2016/08/05 23:23:54, Marijn Kruisselbrink wrote:
> base::hash_set is deprecated (and is just an alias for std::unordered_set
> anyway)

Done.

https://codereview.chromium.org/2055053003/diff/350001/storage/browser/blob/b...
storage/browser/blob/blob_memory_controller.cc:381: switch (item->state()) {
On 2016/08/05 23:23:54, Marijn Kruisselbrink wrote:
> It might help make it more immediately obvious what this switch statement does
> (skip over all items whose state isn't QUOTA_GRANTED or POPULATED_WITH_QUOTA)
if
> you add a comment. Or maybe it's just me that was confused by the
continue/break
> thing at first read of this.

Simplified by requiring that the items are all QUOTA_NEEDED and temporary files.

https://codereview.chromium.org/2055053003/diff/350001/storage/browser/blob/b...
storage/browser/blob/blob_memory_controller.cc:396:
visited_items.find(item->item_id()) != visited_items.end())
On 2016/08/05 23:23:54, Marijn Kruisselbrink wrote:
> nit: there is a ContainsKey method in base/stl_util.h that you could use here
> and in some other places. Although to be honest I'm not entirely sure what the
> value of that method is over just doing the find != end comparison.

Done.

https://codereview.chromium.org/2055053003/diff/350001/storage/browser/blob/b...
storage/browser/blob/blob_memory_controller.cc:476: if
(pending_file_opens_.find(disk_quota_entry) == pending_file_opens_.end()) {
On 2016/08/05 23:23:54, Marijn Kruisselbrink wrote:
> nit: rather than first checking if the value exists and then erasing it if it
> does, you could just use the return value of .erase() to check if the entry
> existed. Although I doubt the performance difference of one vs two lookups is
> going to matter here.

Done.

https://codereview.chromium.org/2055053003/diff/350001/storage/browser/blob/b...
File storage/browser/blob/blob_memory_controller.h (right):

https://codereview.chromium.org/2055053003/diff/350001/storage/browser/blob/b...
storage/browser/blob/blob_memory_controller.h:52: // memory to disk if we are
running low on quota.
On 2016/08/15 22:44:44, michaeln wrote:
> Please be less verbose in this comment (and in general where possible). I'd
> suggest dropping the method specific verbiage.

Done.

https://codereview.chromium.org/2055053003/diff/350001/storage/browser/blob/b...
storage/browser/blob/blob_memory_controller.h:55: enum class
MemoryStrategyResult {
On 2016/08/15 22:44:44, michaeln wrote:
> Shorter symbols, maybe MemoryStrategy or even just Strategy. Since the name is
> scoped to BlobMemoryController, Strategy is probably sufficient.

Done.

https://codereview.chromium.org/2055053003/diff/350001/storage/browser/blob/b...
storage/browser/blob/blob_memory_controller.h:87: // after |success_callback| is
called from the ReserveQuotaForItems call.
On 2016/08/15 22:44:44, michaeln wrote:
> Does this need to be public? would be nice if the details of this were opaque
to
> the caller to simplify the interface.

Simplified

https://codereview.chromium.org/2055053003/diff/350001/storage/browser/blob/b...
storage/browser/blob/blob_memory_controller.h:95: // We're either saving
On 2016/08/15 22:44:44, michaeln wrote:
> i don't understand the comment?

Removed.

https://codereview.chromium.org/2055053003/diff/350001/storage/browser/blob/b...
storage/browser/blob/blob_memory_controller.h:116: MemoryStrategyResult
DecideBlobTransportationMemoryStrategy(
On 2016/08/15 22:44:44, michaeln wrote:
> Please use shorter symbols: DetermineStrategy() DecideStrategy()
PickStrategy()
> or ChooseStrategy().

Done.

https://codereview.chromium.org/2055053003/diff/350001/storage/browser/blob/b...
storage/browser/blob/blob_memory_controller.h:121: bool CanFitInSystem(uint64_t
size) const;
On 2016/08/15 22:44:44, michaeln wrote:
> Does this need to be public? It's checked when the strategy is chosen and
again
> when the pending blob is created. Why does it need to be public?

When we create a blob by using BlobStorageContext::AddFinishedBlob, we don't
check a transportation strategy, and so we still need to check this.

https://codereview.chromium.org/2055053003/diff/350001/storage/browser/blob/b...
storage/browser/blob/blob_memory_controller.h:123: // 'Reserves' space for the
given items, and calls |success_callback| when
On 2016/08/15 22:44:44, michaeln wrote:
> why the quotes?

Simplified.

https://codereview.chromium.org/2055053003/diff/350001/storage/browser/blob/b...
storage/browser/blob/blob_memory_controller.h:133: // One can only reserve quota
for bytes and temporary file items (see
On 2016/08/15 22:44:44, michaeln wrote:
> Maybe make two methods them, one to ReserveMemoryQuota and the other to
> ResourceDiskQuota.

Great idea!

https://codereview.chromium.org/2055053003/diff/350001/storage/browser/blob/b...
storage/browser/blob/blob_memory_controller.h:138:
base::Optional<PendingQuotaEntry> ReserveQuotaForItems(
On 2016/08/15 22:44:44, michaeln wrote:
> Can this return an integer id type to represent the reservation? I think the
use
> of optional and exposing of the struct with internal details adds complexity
to
> this already complicated public interface.

Yes, done.

https://codereview.chromium.org/2055053003/diff/350001/storage/browser/blob/b...
storage/browser/blob/blob_memory_controller.h:264: base::hash_set<size_t>
pending_file_opens_;
On 2016/08/05 23:23:55, Marijn Kruisselbrink wrote:
> base::hash_set is deprecated, use std::unordered_set instead

Done.

https://codereview.chromium.org/2055053003/diff/350001/storage/browser/blob/b...
storage/browser/blob/blob_memory_controller.h:269: // TODO(dmurph): consolidate
these into a struct.
On 2016/08/15 22:44:44, michaeln wrote:
> can this be done in this cl?

Done.

https://codereview.chromium.org/2055053003/diff/350001/storage/browser/blob/b...
File storage/browser/blob/blob_storage_context.h (right):

https://codereview.chromium.org/2055053003/diff/350001/storage/browser/blob/b...
storage/browser/blob/blob_storage_context.h:67: // you must use the BuildBlob
method below.
On 2016/08/15 22:44:44, michaeln wrote:
> Might not need the comments about a finished blob not containing future items.
> Less is more. I think the impl could DCHECK that condition.

Done.

michaeln

I continue to think we should split this CL up. Some ideas on that split? ...

4 years, 4 months ago (2016-08-23 01:31:55 UTC) #60

I continue to think we should split this CL up. Some ideas on that split?

* factor out the storage space controller and do it independently to start with

* hoist maintaining the details of the buildingstate out of the asyncbuilderhost
and into the central context (since in the new system, completion may not occur
until after the instigating renderer is gone)

* maybe massage storage context api to reflect blobs that change states asyncly,
let consumers observe state changes?

In general, I think there's some leakage of complexity regarding the building
state that i suspect could be better encapsulated inside the
'context/spacecontroller'. Here's a idea?

Internally there are several states with the transitions not necessarily coming
in order:

PENDING_DEPENDENT_BLOBS
PENDING_QUOTA_FOR_NEW_ITEMS
PENDING_QUOTA_FOR_COPIED_ITEMS
PENDING_TRANSPORT
DONE | BROKEN

But from the outside, i think we might be able to dumb that down? This is from
the point of view of the context/spacecontroller presenting a programming
interface that easy to consume for other parts of the blob system. Some states
may be skipped, but higher states won't back up to a lower state.

PENDING_QUOTA
only applies to "new items" since the copying of items is wholly an internal
detail

PENDING_TRANSPORT
if/when it hits this state, its ready to accept file/memory for the new items

PENDING_INTERNALS
if/when it hits this state, some internals are still being waiting on, either
dependent blobs or quota for copied items, or the copying of items, or some
other internal detail

FINAL_DONE | FINAL_BROKEN
terminal states

If the context supported observing the progression of state changes, i think
that might help in terms of grokability? 

void AddObserver(uuid, blob_state_change_observer)
void RemoveObserver(uuid, blob_state_change_observer)

https://codereview.chromium.org/2055053003/diff/350001/content/browser/blob_s...
File content/browser/blob_storage/blob_dispatcher_host.cc (right):

https://codereview.chromium.org/2055053003/diff/350001/content/browser/blob_s...
content/browser/blob_storage/blob_dispatcher_host.cc:86:
base::Bind(&BlobDispatcherHost::MaybeSendFinalBlobStatus,
On 2016/08/19 00:18:31, dmurph wrote:
> On 2016/08/15 22:44:42, michaeln wrote:
> > Seems odd that the builder is expected to call this closure upon completion
in
> > some cases, yet not in others, such that this class also has to call it in
> > various places. It's hard to know whos supposed to do what?
> > 
> > I'm not sure if this is possible/practical, but if the builder were always
> > responsible for calling the completion callback or never responsible for it
so
> > the DispatcherHost were always repsonsible for sending the 'done' message,
> that
> > could make a cleaner division of labor.
> 
> Done.

Looks like this was done for OnStart handling specifically but not in general.

https://codereview.chromium.org/2055053003/diff/350001/storage/browser/blob/b...
File storage/browser/blob/blob_memory_controller.cc (right):

https://codereview.chromium.org/2055053003/diff/350001/storage/browser/blob/b...
storage/browser/blob/blob_memory_controller.cc:73:
scoped_refptr<ShareableFileReference> file_reference) {
ShareableFileRefs are not thread-safe so this has to be done a somewhat
differently

https://codereview.chromium.org/2055053003/diff/350001/storage/browser/blob/b...
storage/browser/blob/blob_memory_controller.cc:182: // Step 1: Handle case where
we have no memory to transport.
seems like the the step n comments in this method might not be needed, the code
looks fairly self explanatory

https://codereview.chromium.org/2055053003/diff/350001/storage/browser/blob/b...
storage/browser/blob/blob_memory_controller.cc:201: (total_transportation_bytes
> max_blob_in_memory_size_)) {
should this take shortcut_bytes into account? total - shortcut?

or when the FILE strategy is chosen, do we ignore the shortcut bytes delivered
in the ipc and ask the renderer to write those bytes to a file?

https://codereview.chromium.org/2055053003/diff/390001/content/browser/blob_s...
File content/browser/blob_storage/blob_dispatcher_host.cc (right):

https://codereview.chromium.org/2055053003/diff/390001/content/browser/blob_s...
content/browser/blob_storage/blob_dispatcher_host.cc:154:
SendFinalBlobStatus(uuid, result);
I thought that in some cases the result can still be PENDING? Ditto previous
comment about the builder knows when it completes and how it completes and is
probably in best position to signal completion.

https://codereview.chromium.org/2055053003/diff/390001/content/browser/blob_s...
content/browser/blob_storage/blob_dispatcher_host.cc:229:
!context->registry().HasEntry(uuid)) {
If the final ref is dropped from a different renderer (not the one that is
building the blob), this cancelling and status sending doesn't happen. It'd be
nice if this cancellation code was agnostic about the renderer responsible for
the final dec ref.

If the builder observed status changes (via the context) for blobs it's
building, it could do this internally and it wouldn't matter which process was
the last to drop the ref.

https://codereview.chromium.org/2055053003/diff/390001/storage/browser/blob/b...
File storage/browser/blob/blob_async_builder_host.h (right):

https://codereview.chromium.org/2055053003/diff/390001/storage/browser/blob/b...
storage/browser/blob/blob_async_builder_host.h:101: struct BlobBuildingState {
Since overall building state is being moved to the context, i'd vote to call
this class something else now.

Also, since it's a nested class, the 'Blob' prefix seems redundant (ditto for
any other private nested classes prefixed with Blob).

Maybe TransportState? (with this class being the BlobTransportControllerHost
which interacts with the BlobTransportController in the renderer).

https://codereview.chromium.org/2055053003/diff/390001/storage/common/blob_st...
File storage/common/blob_storage/blob_storage_constants.h (right):

https://codereview.chromium.org/2055053003/diff/390001/storage/common/blob_st...
storage/common/blob_storage/blob_storage_constants.h:19: struct
BlobStorageQuotas {
not sure about calling this Quotas since that term is loaded? Also, seems like
this could be a nested struct since it directly applies to the

https://codereview.chromium.org/2055053003/diff/390001/storage/common/blob_st...
storage/common/blob_storage/blob_storage_constants.h:20: size_t
GetTotalMemorySpace() const {
nit: total_memory_space()

dmurph

per request this has been split up, first patch is the BlobMemoryController located here: https://codereview.chromium.org/2339933004/ ...

4 years, 3 months ago (2016-09-20 19:53:26 UTC) #61

dmurph

The CQ bit was checked by dmurph@chromium.org to run a CQ dry run

4 years, 2 months ago (2016-10-10 22:27:03 UTC) #62

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2055053003/410001

4 years, 2 months ago (2016-10-10 22:27:29 UTC) #63

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 2 months ago (2016-10-10 22:47:51 UTC) #64

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_chromium_compile_only_ng/builds/214146)

4 years, 2 months ago (2016-10-10 22:47:52 UTC) #65

dmurph

Hey Kinuko, Michael, Marijn, and Victor, I have this patch updated to the recently committed ...

4 years, 2 months ago (2016-10-21 22:21:32 UTC) #66

dmurph

4 years, 2 months ago (2016-10-21 22:27:14 UTC) #67

Michael and I had an idea about doing all the non-memory controller
changes, where we don't need file state stuff, before integrating that
memory controller. My main concern is that removing the file quota
completely serializes our state, so it's a WAY simpler problem. So we may
oversimplify the solution to just recomplicate it again. But maybe we'll
then be able to focus on getting the other bits well? Idk.

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

Issue 2055053003: [BlobAsync] Disk support for blob storage (Closed)

Description

Patch Set 1 #

Patch Set 2 : PAging working! #

Patch Set 3 : fixes, working w/ Layout tests #

Patch Set 4 : Added back transport controller test, small cleanups #

Patch Set 5 : WIP don't review this one #

Patch Set 6 : comments, simplifications #

Patch Set 7 : comments, simplification of enums into ONE #

Patch Set 8 : halfway done with comments #

Patch Set 9 : comments #

Patch Set 10 : comments, new tests, bug fixes #

Patch Set 11 : rebase #

Patch Set 12 : build and browsertest fixes #

Patch Set 13 : More tests, got through half of the comments #

Patch Set 14 : Finished comments, added new pending enum state #

Patch Set 15 : comments, moved Entry to InternalBlobData #

Patch Set 16 : Fixed layout tests, cleaned up test visibility #

Patch Set 17 : BlobMemoryController refactor #

Patch Set 18 : Combined BlobSlice & BlobFlattener files, more comments, a little cleanup. #

Patch Set 19 : Combined BlobSlice & BlobFlattener files, more comments, a little cleanup. #

Patch Set 20 : memory controller pass #

Patch Set 21 : half of comments #

Patch Set 22 : Rebase w/ BlobMemoryController patch #

Patch Set 23 : rebase #

Messages