Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2339933004:
[BlobStorage] BlobMemoryController & tests (Closed)
Description[BlobStorage] BlobMemoryController & tests
This is a split off of https://codereview.chromium.org/2055053003/.
This adds the BlobMemoryController & tests, as well as any other small
required changes. I avoided doing too much refactoring (like the move
to BlobStatus everywhere, and removing BlobStorageRegistry::Entry for
InternalBlobData) to just focus on the BlobMemoryController API.
BUG=375297
R=michaeln@chromium.org,kinuko@chromium.org,mek@chromium.org
Committed: https://crrev.com/0b0e36d9a68b386ab2e8e6c921122bb0f6e7e0e9
Cr-Commit-Position: refs/heads/master@{#426901}
Patch Set 1 #
Total comments: 20
Patch Set 2 : comments #Patch Set 3 : rebase #
Total comments: 40
Patch Set 4 : round 1 comments #
Total comments: 6
Patch Set 5 : forgot new test file #Patch Set 6 : refactor w/ tasks #Patch Set 7 : Comments, and made task base class for hopefully more simplicity #
Total comments: 117
Patch Set 8 : comments, more tests are next #
Total comments: 28
Patch Set 9 : new tests, added catch for file deletion on IO thread #Patch Set 10 : Simplified file object deletion #Patch Set 11 : Fix android & windows build errors #
Total comments: 76
Patch Set 12 : comments, added MemoryAllocation, and changed memory quota bookkeeping #
Total comments: 2
Patch Set 13 : switched back from file literal for windows compile #
Total comments: 11
Patch Set 14 : Comments, MemoryAllocation struct, and new test #Patch Set 15 : windows visibility fix #Patch Set 16 : format and hopefully windows fix #
Total comments: 25
Patch Set 17 : memset fix, windows fix (hopefully) #Patch Set 18 : histograms and windows fix #
Total comments: 30
Patch Set 19 : comments and in_flight limit simplification (using min_page_file_size) #
Total comments: 25
Patch Set 20 : comments, and hopefully windows fix #Patch Set 21 : debugging logging for windows #Patch Set 22 : Removed logging #Patch Set 23 : rebase #Patch Set 24 : Fixed windows bug! #Dependent Patchsets: Messages
Total messages: 129 (88 generated)
Hey all, This is my first split-off patch from the monster patch, where I just focus on the BlobMemoryController. This is the most complex part, so I'm taking Michaels advice and putting this patch first. PTAL! Thanks, Daniel
Description was changed from ========== [BlobStorage] BlobMemoryController & tests This is a split off of https://codereview.chromium.org/2055053003/. This adds the BlobMemoryController & tests, as well as any other small required changes. I avoided doing too much refactoring (like the move to BlobStatus everywhere, and removing BlobStorageRegistry::Entry for InternalBlobData) to just focus on the BlobMemoryController API. BUG=375297 R=michaeln@chromium.org,kinuko@chromium.org,mek@chromium.org ========== to ========== [BlobStorage] BlobMemoryController & tests This is a split off of https://codereview.chromium.org/2055053003/. This adds the BlobMemoryController & tests, as well as any other small required changes. I avoided doing too much refactoring (like the move to BlobStatus everywhere, and removing BlobStorageRegistry::Entry for InternalBlobData) to just focus on the BlobMemoryController API. BUG=375297 R=michaeln@chromium.org,kinuko@chromium.org,mek@chromium.org ==========
dmurph@chromium.org changed reviewers: + pwnall@chromium.org - michaeln@chromium.org
dmurph@chromium.org changed reviewers: + michaeln@chromium.org
+michaeln, who somehow I didn't include here (weird)
Here are some quick things I noticed. I'll resume reading later today. https://codereview.chromium.org/2339933004/diff/1/storage/browser/blob/blob_m... File storage/browser/blob/blob_memory_controller.cc (right): https://codereview.chromium.org/2339933004/diff/1/storage/browser/blob/blob_m... storage/browser/blob/blob_memory_controller.cc:79: LOG(ERROR) << "writing to file!"; The ERROR level doesn't seem right. Also, is this supposed to be a DVLOG? https://codereview.chromium.org/2339933004/diff/1/storage/browser/blob/blob_m... storage/browser/blob/blob_memory_controller.cc:140: BlobMemoryController::FileCreationInfo::FileCreationInfo() {} Can you do `= default` instead? Also, I think you can get rid of the BlobMemoryController namespace, since you aliased FileCreationInfo here. https://codereview.chromium.org/2339933004/diff/1/storage/browser/blob/blob_m... storage/browser/blob/blob_memory_controller.cc:142: BlobMemoryController::FileCreationInfo::~FileCreationInfo() {} same as above https://codereview.chromium.org/2339933004/diff/1/storage/browser/blob/blob_m... storage/browser/blob/blob_memory_controller.cc:161: LOG(ERROR) << "enabling disk"; Is ERROR correct here? https://codereview.chromium.org/2339933004/diff/1/storage/browser/blob/blob_m... storage/browser/blob/blob_memory_controller.cc:196: // Step 3: Decide if we're using the shortcut method. The step comments seem obvious (meaning that the code is very expressive), except for this one. If the shortcut method is documented somewhere else, might be worth replacing this step comment with a pointer to that file, and removing all the other step docs. https://codereview.chromium.org/2339933004/diff/1/storage/browser/blob/blob_m... storage/browser/blob/blob_memory_controller.cc:622: // If disk is enabled, then we include |in_flight_memory_used_|. Otherwise we This comment doesn't seem to agree with the implementation. If I'm reading the code correctly, both branches of `if (disk_enabled_)` do the same thing. https://codereview.chromium.org/2339933004/diff/1/storage/browser/blob/blob_m... storage/browser/blob/blob_memory_controller.cc:635: return disk_enabled_ This may be easier to read if you have a separate `if (disk_enabled_) return 0;` Just an opinion. I don't know enough about the style guide and surrounding code to figure out what's the preferred way to express this. https://codereview.chromium.org/2339933004/diff/1/storage/browser/blob/blob_m... File storage/browser/blob/blob_memory_controller.h (right): https://codereview.chromium.org/2339933004/diff/1/storage/browser/blob/blob_m... storage/browser/blob/blob_memory_controller.h:90: void EnableDisk(const base::FilePath& storage_directory, Using both disk and file(s) means the reader has to mentally resolve one name to the other, which is a (minor) drain on processing power. How about using names like EnableFiles() / DisableFiles() / files_enabled(), and referring to the concept as "file storage" in the comments? https://codereview.chromium.org/2339933004/diff/1/storage/browser/blob/blob_m... storage/browser/blob/blob_memory_controller.h:109: // This reserves quota for the given |unreserved_memory_items|. We expect "We expect" seems a bit soft here. Given the DCHECK in the method, I'd expect something along the lines of "The items must be in the QUOTA_NEEDED state." The rest of the text could sound like "They will be moved to the QUOTA_REQUESTED state immediately, and will eventually be placed into the QUOTA_GRANTED state when |success_callback| is called." https://codereview.chromium.org/2339933004/diff/1/storage/browser/blob/blob_m... storage/browser/blob/blob_memory_controller.h:169: // items are pending file items. We handle error cases, the case where the the caller?
The CQ bit was checked by dmurph@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks for the comments Victor! https://codereview.chromium.org/2339933004/diff/1/storage/browser/blob/blob_m... File storage/browser/blob/blob_memory_controller.cc (right): https://codereview.chromium.org/2339933004/diff/1/storage/browser/blob/blob_m... storage/browser/blob/blob_memory_controller.cc:79: LOG(ERROR) << "writing to file!"; On 2016/09/16 22:45:24, pwnall wrote: > The ERROR level doesn't seem right. Also, is this supposed to be a DVLOG? Yeah I'll get rid of all these now. https://codereview.chromium.org/2339933004/diff/1/storage/browser/blob/blob_m... storage/browser/blob/blob_memory_controller.cc:140: BlobMemoryController::FileCreationInfo::FileCreationInfo() {} On 2016/09/16 22:45:24, pwnall wrote: > Can you do `= default` instead? > > Also, I think you can get rid of the BlobMemoryController namespace, since you > aliased FileCreationInfo here. Done. https://codereview.chromium.org/2339933004/diff/1/storage/browser/blob/blob_m... storage/browser/blob/blob_memory_controller.cc:142: BlobMemoryController::FileCreationInfo::~FileCreationInfo() {} On 2016/09/16 22:45:24, pwnall wrote: > same as above Done. https://codereview.chromium.org/2339933004/diff/1/storage/browser/blob/blob_m... storage/browser/blob/blob_memory_controller.cc:161: LOG(ERROR) << "enabling disk"; On 2016/09/16 22:45:24, pwnall wrote: > Is ERROR correct here? Done. https://codereview.chromium.org/2339933004/diff/1/storage/browser/blob/blob_m... storage/browser/blob/blob_memory_controller.cc:196: // Step 3: Decide if we're using the shortcut method. On 2016/09/16 22:45:24, pwnall wrote: > The step comments seem obvious (meaning that the code is very expressive), > except for this one. If the shortcut method is documented somewhere else, might > be worth replacing this step comment with a pointer to that file, and removing > all the other step docs. Done. https://codereview.chromium.org/2339933004/diff/1/storage/browser/blob/blob_m... storage/browser/blob/blob_memory_controller.cc:622: // If disk is enabled, then we include |in_flight_memory_used_|. Otherwise we On 2016/09/16 22:45:24, pwnall wrote: > This comment doesn't seem to agree with the implementation. If I'm reading the > code correctly, both branches of `if (disk_enabled_)` do the same thing. Merged. https://codereview.chromium.org/2339933004/diff/1/storage/browser/blob/blob_m... storage/browser/blob/blob_memory_controller.cc:635: return disk_enabled_ On 2016/09/16 22:45:24, pwnall wrote: > This may be easier to read if you have a separate `if (disk_enabled_) return 0;` > > Just an opinion. I don't know enough about the style guide and surrounding code > to figure out what's the preferred way to express this. Done. https://codereview.chromium.org/2339933004/diff/1/storage/browser/blob/blob_m... File storage/browser/blob/blob_memory_controller.h (right): https://codereview.chromium.org/2339933004/diff/1/storage/browser/blob/blob_m... storage/browser/blob/blob_memory_controller.h:90: void EnableDisk(const base::FilePath& storage_directory, On 2016/09/16 22:45:24, pwnall wrote: > Using both disk and file(s) means the reader has to mentally resolve one name to > the other, which is a (minor) drain on processing power. How about using names > like EnableFiles() / DisableFiles() / files_enabled(), and referring to the > concept as "file storage" in the comments? Good idea, done. https://codereview.chromium.org/2339933004/diff/1/storage/browser/blob/blob_m... storage/browser/blob/blob_memory_controller.h:109: // This reserves quota for the given |unreserved_memory_items|. We expect On 2016/09/16 22:45:24, pwnall wrote: > "We expect" seems a bit soft here. Given the DCHECK in the method, I'd expect > something along the lines of "The items must be in the QUOTA_NEEDED state." The > rest of the text could sound like "They will be moved to the QUOTA_REQUESTED > state immediately, and will eventually be placed into the QUOTA_GRANTED state > when |success_callback| is called." Done. https://codereview.chromium.org/2339933004/diff/1/storage/browser/blob/blob_m... storage/browser/blob/blob_memory_controller.h:169: // items are pending file items. We handle error cases, the case where the On 2016/09/16 22:45:24, pwnall wrote: > the caller? Fixed this documentation.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Michael, can you PTAL? :) On Mon, Sep 19, 2016, 5:00 PM <dmurph@chromium.org> wrote: > Thanks for the comments Victor! > > > > > https://codereview.chromium.org/2339933004/diff/1/storage/browser/blob/blob_m... > File storage/browser/blob/blob_memory_controller.cc (right): > > > https://codereview.chromium.org/2339933004/diff/1/storage/browser/blob/blob_m... > storage/browser/blob/blob_memory_controller.cc:79: LOG(ERROR) << > "writing to file!"; > On 2016/09/16 22:45:24, pwnall wrote: > > The ERROR level doesn't seem right. Also, is this supposed to be a > DVLOG? > > Yeah I'll get rid of all these now. > > > > https://codereview.chromium.org/2339933004/diff/1/storage/browser/blob/blob_m... > storage/browser/blob/blob_memory_controller.cc:140: > BlobMemoryController::FileCreationInfo::FileCreationInfo() {} > On 2016/09/16 22:45:24, pwnall wrote: > > Can you do `= default` instead? > > > > Also, I think you can get rid of the BlobMemoryController namespace, > since you > > aliased FileCreationInfo here. > > Done. > > > > https://codereview.chromium.org/2339933004/diff/1/storage/browser/blob/blob_m... > storage/browser/blob/blob_memory_controller.cc:142: > BlobMemoryController::FileCreationInfo::~FileCreationInfo() {} > On 2016/09/16 22:45:24, pwnall wrote: > > same as above > > Done. > > > > https://codereview.chromium.org/2339933004/diff/1/storage/browser/blob/blob_m... > storage/browser/blob/blob_memory_controller.cc:161: LOG(ERROR) << > "enabling disk"; > On 2016/09/16 22:45:24, pwnall wrote: > > Is ERROR correct here? > > Done. > > > > https://codereview.chromium.org/2339933004/diff/1/storage/browser/blob/blob_m... > storage/browser/blob/blob_memory_controller.cc:196: // Step 3: Decide if > we're using the shortcut method. > On 2016/09/16 22:45:24, pwnall wrote: > > The step comments seem obvious (meaning that the code is very > expressive), > > except for this one. If the shortcut method is documented somewhere > else, might > > be worth replacing this step comment with a pointer to that file, and > removing > > all the other step docs. > > Done. > > > > https://codereview.chromium.org/2339933004/diff/1/storage/browser/blob/blob_m... > storage/browser/blob/blob_memory_controller.cc:622: // If disk is > enabled, then we include |in_flight_memory_used_|. Otherwise we > On 2016/09/16 22:45:24, pwnall wrote: > > This comment doesn't seem to agree with the implementation. If I'm > reading the > > code correctly, both branches of `if (disk_enabled_)` do the same > thing. > > Merged. > > > > https://codereview.chromium.org/2339933004/diff/1/storage/browser/blob/blob_m... > storage/browser/blob/blob_memory_controller.cc:635: return disk_enabled_ > On 2016/09/16 22:45:24, pwnall wrote: > > This may be easier to read if you have a separate `if (disk_enabled_) > return 0;` > > > > Just an opinion. I don't know enough about the style guide and > surrounding code > > to figure out what's the preferred way to express this. > > Done. > > > > https://codereview.chromium.org/2339933004/diff/1/storage/browser/blob/blob_m... > File storage/browser/blob/blob_memory_controller.h (right): > > > https://codereview.chromium.org/2339933004/diff/1/storage/browser/blob/blob_m... > storage/browser/blob/blob_memory_controller.h:90: void EnableDisk(const > base::FilePath& storage_directory, > On 2016/09/16 22:45:24, pwnall wrote: > > Using both disk and file(s) means the reader has to mentally resolve > one name to > > the other, which is a (minor) drain on processing power. How about > using names > > like EnableFiles() / DisableFiles() / files_enabled(), and referring > to the > > concept as "file storage" in the comments? > > Good idea, done. > > > > https://codereview.chromium.org/2339933004/diff/1/storage/browser/blob/blob_m... > storage/browser/blob/blob_memory_controller.h:109: // This reserves > quota for the given |unreserved_memory_items|. We expect > On 2016/09/16 22:45:24, pwnall wrote: > > "We expect" seems a bit soft here. Given the DCHECK in the method, I'd > expect > > something along the lines of "The items must be in the QUOTA_NEEDED > state." The > > rest of the text could sound like "They will be moved to the > QUOTA_REQUESTED > > state immediately, and will eventually be placed into the > QUOTA_GRANTED state > > when |success_callback| is called." > > Done. > > > > https://codereview.chromium.org/2339933004/diff/1/storage/browser/blob/blob_m... > storage/browser/blob/blob_memory_controller.h:169: // items are pending > file items. We handle error cases, the case where the > On 2016/09/16 22:45:24, pwnall wrote: > > the caller? > > Fixed this documentation. > > https://codereview.chromium.org/2339933004/ > -- 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.
The CQ bit was checked by dmurph@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
I still haven't taken a good look at some of the bits, but here are some comments to move this along. https://codereview.chromium.org/2339933004/diff/40001/content/browser/blob_st... File content/browser/blob_storage/blob_memory_controller_unittest.cc (right): https://codereview.chromium.org/2339933004/diff/40001/content/browser/blob_st... content/browser/blob_storage/blob_memory_controller_unittest.cc:225: // TODO(dmurph): Can you crbug this please? https://codereview.chromium.org/2339933004/diff/40001/content/test/BUILD.gn File content/test/BUILD.gn (right): https://codereview.chromium.org/2339933004/diff/40001/content/test/BUILD.gn#n... content/test/BUILD.gn:955: "../browser/blob_storage/blob_memory_controller_unittest.cc", It's a bit odd (and against the style guide, I think) to cover files in storage/browser with a test in content/ -- does this have to be here? https://codereview.chromium.org/2339933004/diff/40001/storage/browser/blob/bl... File storage/browser/blob/blob_data_builder.cc (right): https://codereview.chromium.org/2339933004/diff/40001/storage/browser/blob/bl... storage/browser/blob/blob_data_builder.cc:43: base::StringToUint64(element.path().BaseName().MaybeAsASCII(), &id); Won't BaseName be something like "_future_name.42", which wouldn't parse? Did you mean to create a directory and use FilePath::Append in AppendFutureFile()? https://codereview.chromium.org/2339933004/diff/40001/storage/browser/blob/bl... storage/browser/blob/blob_data_builder.cc:136: element->SetToFilePathRange(base::FilePath::FromUTF8Unsafe(kFutureFileName) How about pulling out base::FilePath::FromUTF8Unsafe(kFutureFileName).AddExtension(base::Uint64ToString(file_id) into its own static method (e.g., GetFutureFilePath) and exporting that instead of kFutureFileName[]? This way, you could test GetFutureFilePath in conjunction with IsFutureFileItem / GetFutureFileID, and you could have the BlobAsyncTransportRequestBuilderTest test use GetFutureFilePath for the expectation. https://codereview.chromium.org/2339933004/diff/40001/storage/browser/blob/bl... storage/browser/blob/blob_data_builder.cc:159: items_.at(index)->data_handle_ = std::move(file_reference); I think you can use [] instead of at(), because you DCHECKed the index already. But I saw that at() was already used before (line 148). Do you happen to know why at() is better than []? I thought it's worse because it can throw an exception, so it triggers worse code generation. https://codereview.chromium.org/2339933004/diff/40001/storage/browser/blob/bl... File storage/browser/blob/blob_data_builder.h (right): https://codereview.chromium.org/2339933004/diff/40001/storage/browser/blob/bl... storage/browser/blob/blob_data_builder.h:141: friend class BlobAsyncBuilderHostTest; This isn't a fault introduced in this CL... but the large list of friend classes makes me think that some parts of the "private" section (operator==, it seems) have de facto leaked into the public API. https://codereview.chromium.org/2339933004/diff/40001/storage/browser/blob/bl... File storage/browser/blob/blob_data_item.h (right): https://codereview.chromium.org/2339933004/diff/40001/storage/browser/blob/bl... storage/browser/blob/blob_data_item.h:25: class DataElement; Does this forward declaration mean you don't need to include data_element.h? If not, why is the declaration necessary? https://codereview.chromium.org/2339933004/diff/40001/storage/browser/blob/bl... storage/browser/blob/blob_data_item.h:80: // friend class BlobMemoryController; Is this on purpose? Did you mean to leave this friend in, and not make the constructors public? https://codereview.chromium.org/2339933004/diff/40001/storage/browser/blob/sh... File storage/browser/blob/shareable_blob_data_item.cc (right): https://codereview.chromium.org/2339933004/diff/40001/storage/browser/blob/sh... storage/browser/blob/shareable_blob_data_item.cc:39: for (const std::string& uuid : x.referencing_blobs_) { Why do you need non-const access (referencing_blobs_ instead of referencing_blobs()) here? https://codereview.chromium.org/2339933004/diff/40001/storage/browser/blob/sh... File storage/browser/blob/shareable_blob_data_item.h (right): https://codereview.chromium.org/2339933004/diff/40001/storage/browser/blob/sh... storage/browser/blob/shareable_blob_data_item.h:26: // We need to be RefCountedThreadSafe as references of this item is passed to a references are passed https://codereview.chromium.org/2339933004/diff/40001/storage/browser/blob/sh... storage/browser/blob/shareable_blob_data_item.h:68: friend struct BlobFlattener; These are a lot of friends. Would it make more sense to add inline public methods for adding and removing referencing blob UUIDs?
Here are a few more thoughts. Most of them target the comments in the memory controller's header. https://codereview.chromium.org/2339933004/diff/40001/storage/browser/blob/bl... File storage/browser/blob/blob_memory_controller.cc (right): https://codereview.chromium.org/2339933004/diff/40001/storage/browser/blob/bl... storage/browser/blob/blob_memory_controller.cc:373: visited_items.insert(item->item_id()); Do these items need to be released somehow? Granted I still haven't put all the pieces together, I'm having a bit of trouble understanding why it's OK to simply count the memory as freed here. https://codereview.chromium.org/2339933004/diff/40001/storage/browser/blob/bl... storage/browser/blob/blob_memory_controller.cc:374: total_memory_freeing += item->item()->length(); memory_to_be_freed? https://codereview.chromium.org/2339933004/diff/40001/storage/browser/blob/bl... File storage/browser/blob/blob_memory_controller.h (right): https://codereview.chromium.org/2339933004/diff/40001/storage/browser/blob/bl... storage/browser/blob/blob_memory_controller.h:40: // This class is responsible for file & memory quota bookkeeping, creating files It might be easier to reason about what's in this class if you can come up with one phrase that describes its responsibility. This paragraph could be a bullet list, which makes me wonder whether the class violates SRP. It seems to me that the class' main responsibility is deciding where blob contents is stored. If this is true, it follows that the memory controller would decide how a blob's items are transferred from the renderer to the browser, and when/which items move between disk and RAM. So, the bullet list gets to become a 2nd paragraph. WDYT? https://codereview.chromium.org/2339933004/diff/40001/storage/browser/blob/bl... storage/browser/blob/blob_memory_controller.h:74: // The bool argument is if we were able to successfuly receive quota. is if -> is true if? Please feel free to discard this feedback (and let me know) if this shorthand is common in the codebase. https://codereview.chromium.org/2339933004/diff/40001/storage/browser/blob/bl... storage/browser/blob/blob_memory_controller.h:109: // This reserves quota for the given |unreserved_memory_items|. The items must "This" seems unnecessary here, and diverges from the cadence established above. Also, in method comments, I'd expect "this" to refer to the C++ this, e.g. "this memory controller". If you agree, please also update the comments below. https://codereview.chromium.org/2339933004/diff/40001/storage/browser/blob/bl... storage/browser/blob/blob_memory_controller.h:114: // CanReserveQuota before calling this. Would it make sense to have a stronger requirement here, like "DetermineStrategy must have indicated that the shared memory strategy should be used for this blob"? I'm specifically troubled by the fact that the CanReserveQuota() implementation returns true if the blob fits into memory _or_ into file storage. So It seems like it's possible to have that memory return true, but still not have enough quota here. https://codereview.chromium.org/2339933004/diff/40001/storage/browser/blob/bl... storage/browser/blob/blob_memory_controller.h:127: // NOTE: We don't inspect quota limits and assume the user checked nit: not sure if "user" is common, but I think "caller" is a bit more precise here https://codereview.chromium.org/2339933004/diff/40001/storage/browser/blob/bl... storage/browser/blob/blob_memory_controller.h:148: // This adds or updates a bytes item in our LRU table used for paging items Who is responsible for updating the LRU cache/list? https://codereview.chromium.org/2339933004/diff/40001/storage/browser/blob/bl... storage/browser/blob/blob_memory_controller.h:168: // Called when we've finished creating files for ReserveFileQuota. We make Everything after the first sentence seems like an implementation detail, which makes me think it belongs to the body of the function. AFAIK, the comments in the header should indicate how one would use the function. https://codereview.chromium.org/2339933004/diff/40001/storage/browser/blob/bl... storage/browser/blob/blob_memory_controller.h:169: // sure to handle the case where the user had cancelled the operation, and if I find "user" mildly confusing here. I'm guessing it doesn't mean the browser's user. "the case where the quota request has been canceled" seems a tiny bit easier to understand. https://codereview.chromium.org/2339933004/diff/40001/storage/browser/blob/bl... storage/browser/blob/blob_memory_controller.h:182: // We schedule paging until our memory usage is below our memory limit. We use "We" seems redundant. The first sentence seems to re-state the method name. The following sentences appear to describe the implementation (and thus belong in the definition).
Here are replies from first round of comments (and patch) It seems like the API generally is OK, so I'll go ahead and start making all of the tests. WDYT Vqictor? https://codereview.chromium.org/2339933004/diff/40001/content/browser/blob_st... File content/browser/blob_storage/blob_memory_controller_unittest.cc (right): https://codereview.chromium.org/2339933004/diff/40001/content/browser/blob_st... content/browser/blob_storage/blob_memory_controller_unittest.cc:225: // TODO(dmurph): On 2016/09/21 09:03:35, pwnall wrote: > Can you crbug this please? This will be in this patch, no need for crbug. Just waiting until API seems mostly agreed upon in this review. https://codereview.chromium.org/2339933004/diff/40001/content/test/BUILD.gn File content/test/BUILD.gn (right): https://codereview.chromium.org/2339933004/diff/40001/content/test/BUILD.gn#n... content/test/BUILD.gn:955: "../browser/blob_storage/blob_memory_controller_unittest.cc", On 2016/09/21 09:03:35, pwnall wrote: > It's a bit odd (and against the style guide, I think) to cover files in > storage/browser with a test in content/ -- does this have to be here? We don't have a tests target for storage, so we put them all here. It's weird. We should probably change that at some point, but not in this patch. https://codereview.chromium.org/2339933004/diff/40001/storage/browser/blob/bl... File storage/browser/blob/blob_data_builder.cc (right): https://codereview.chromium.org/2339933004/diff/40001/storage/browser/blob/bl... storage/browser/blob/blob_data_builder.cc:43: base::StringToUint64(element.path().BaseName().MaybeAsASCII(), &id); On 2016/09/21 09:03:35, pwnall wrote: > Won't BaseName be something like "_future_name.42", which wouldn't parse? > > Did you mean to create a directory and use FilePath::Append in > AppendFutureFile()? Yeah, this was a suggestion that didn't end up working. Fixed. https://codereview.chromium.org/2339933004/diff/40001/storage/browser/blob/bl... storage/browser/blob/blob_data_builder.cc:136: element->SetToFilePathRange(base::FilePath::FromUTF8Unsafe(kFutureFileName) On 2016/09/21 09:03:35, pwnall wrote: > How about pulling out > base::FilePath::FromUTF8Unsafe(kFutureFileName).AddExtension(base::Uint64ToString(file_id) > into its own static method (e.g., GetFutureFilePath) and exporting that instead > of kFutureFileName[]? > > This way, you could test GetFutureFilePath in conjunction with IsFutureFileItem > / GetFutureFileID, and you could have the BlobAsyncTransportRequestBuilderTest > test use GetFutureFilePath for the expectation. Done. https://codereview.chromium.org/2339933004/diff/40001/storage/browser/blob/bl... storage/browser/blob/blob_data_builder.cc:159: items_.at(index)->data_handle_ = std::move(file_reference); On 2016/09/21 09:03:35, pwnall wrote: > I think you can use [] instead of at(), because you DCHECKed the index already. > But I saw that at() was already used before (line 148). Do you happen to know > why at() is better than []? I thought it's worse because it can throw an > exception, so it triggers worse code generation. Done. https://codereview.chromium.org/2339933004/diff/40001/storage/browser/blob/bl... File storage/browser/blob/blob_data_builder.h (right): https://codereview.chromium.org/2339933004/diff/40001/storage/browser/blob/bl... storage/browser/blob/blob_data_builder.h:141: friend class BlobAsyncBuilderHostTest; On 2016/09/21 09:03:35, pwnall wrote: > This isn't a fault introduced in this CL... but the large list of friend classes > makes me think that some parts of the "private" section (operator==, it seems) > have de facto leaked into the public API. That operator is supposed to be public. Let me know what suggestions you have here... I removed the unnecessary friend declarations (carried over from other patch). https://codereview.chromium.org/2339933004/diff/40001/storage/browser/blob/bl... File storage/browser/blob/blob_data_item.h (right): https://codereview.chromium.org/2339933004/diff/40001/storage/browser/blob/bl... storage/browser/blob/blob_data_item.h:25: class DataElement; On 2016/09/21 09:03:35, pwnall wrote: > Does this forward declaration mean you don't need to include data_element.h? If > not, why is the declaration necessary? Done. https://codereview.chromium.org/2339933004/diff/40001/storage/browser/blob/bl... storage/browser/blob/blob_data_item.h:80: // friend class BlobMemoryController; On 2016/09/21 09:03:35, pwnall wrote: > Is this on purpose? Did you mean to leave this friend in, and not make the > constructors public? removed. https://codereview.chromium.org/2339933004/diff/40001/storage/browser/blob/sh... File storage/browser/blob/shareable_blob_data_item.cc (right): https://codereview.chromium.org/2339933004/diff/40001/storage/browser/blob/sh... storage/browser/blob/shareable_blob_data_item.cc:39: for (const std::string& uuid : x.referencing_blobs_) { On 2016/09/21 09:03:35, pwnall wrote: > Why do you need non-const access (referencing_blobs_ instead of > referencing_blobs()) here? Done. https://codereview.chromium.org/2339933004/diff/40001/storage/browser/blob/sh... File storage/browser/blob/shareable_blob_data_item.h (right): https://codereview.chromium.org/2339933004/diff/40001/storage/browser/blob/sh... storage/browser/blob/shareable_blob_data_item.h:26: // We need to be RefCountedThreadSafe as references of this item is passed to a On 2016/09/21 09:03:35, pwnall wrote: > references are passed Done. https://codereview.chromium.org/2339933004/diff/40001/storage/browser/blob/sh... storage/browser/blob/shareable_blob_data_item.h:68: friend struct BlobFlattener; On 2016/09/21 09:03:35, pwnall wrote: > These are a lot of friends. Would it make more sense to add inline public > methods for adding and removing referencing blob UUIDs? Mmmm it's actually for state and item modification. I could make those public.... especially because this class is only used in the blob storage internals. WDYT?
TBH I don't know enough about the Chromium side yet to be able to come up with alternative designs, so I assumed that you OKed the design a while ago with jsbell, and my job is to help you turn into readable code :) Here are a few more comments. I'm sorry they're coming in batches like this. https://codereview.chromium.org/2339933004/diff/60001/storage/browser/blob/bl... File storage/browser/blob/blob_memory_controller.cc (right): https://codereview.chromium.org/2339933004/diff/60001/storage/browser/blob/bl... storage/browser/blob/blob_memory_controller.cc:46: std::vector<BlobMemoryController::FileCreationInfo> result; Do you need to DCHECK that you're in the File thread (not the IO thread) here, or are you relying on the File::GetInfo implementation to do that for you? After a (very) cursory reading, I can't figure out if the IO thread will trip ThreadRestrictions::AssertIOAllowed or not. https://codereview.chromium.org/2339933004/diff/60001/storage/browser/blob/bl... storage/browser/blob/blob_memory_controller.cc:162: void BlobMemoryController::DisableFilePaging() { Should this method also clear file_runner_ (to avoid using it accidentally afterwards)? Also, why do you not need to cancel pending requests here? https://codereview.chromium.org/2339933004/diff/60001/storage/browser/blob/bl... File storage/browser/blob/blob_memory_controller.h (right): https://codereview.chromium.org/2339933004/diff/60001/storage/browser/blob/bl... storage/browser/blob/blob_memory_controller.h:118: const MemoryQuotaRequestCallback& success_callback); Is it appropriate to call this param success_callback? It seems like it's called with a boolean that indicates success/failure.
Victor and I had an offline discussion about changing the architecture, specifically around moving the quota requests into 'task' objects. I just uploaded the result of this refactor. It cleans up a lot actually, which is nice. I didn't document the refactored changes much yet, this is still TODO. But I wanted to get this patch up. Victor, I didn't look at your other comments yet. You're welcome to take a look at the new architecture and let me know any glarring arch issues, and I'll be working on addressing all of your comments.
The CQ bit was checked by dmurph@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Ok, so I replied to comments and made the tasks a little more simple by pulling out the list iterator and size into a base class. Next step is testing. https://codereview.chromium.org/2339933004/diff/40001/storage/browser/blob/bl... File storage/browser/blob/blob_memory_controller.cc (right): https://codereview.chromium.org/2339933004/diff/40001/storage/browser/blob/bl... storage/browser/blob/blob_memory_controller.cc:373: visited_items.insert(item->item_id()); On 2016/09/21 22:56:58, pwnall wrote: > Do these items need to be released somehow? > > Granted I still haven't put all the pieces together, I'm having a bit of trouble > understanding why it's OK to simply count the memory as freed here. Not here, this class doesn't manage the lifetime of those objects, just the quota for the memory they contain. https://codereview.chromium.org/2339933004/diff/40001/storage/browser/blob/bl... storage/browser/blob/blob_memory_controller.cc:374: total_memory_freeing += item->item()->length(); On 2016/09/21 22:56:58, pwnall wrote: > memory_to_be_freed? Done. https://codereview.chromium.org/2339933004/diff/40001/storage/browser/blob/bl... File storage/browser/blob/blob_memory_controller.h (right): https://codereview.chromium.org/2339933004/diff/40001/storage/browser/blob/bl... storage/browser/blob/blob_memory_controller.h:40: // This class is responsible for file & memory quota bookkeeping, creating files On 2016/09/21 22:56:58, pwnall wrote: > It might be easier to reason about what's in this class if you can come up with > one phrase that describes its responsibility. This paragraph could be a bullet > list, which makes me wonder whether the class violates SRP. > > It seems to me that the class' main responsibility is deciding where blob > contents is stored. If this is true, it follows that the memory controller would > decide how a blob's items are transferred from the renderer to the browser, and > when/which items move between disk and RAM. So, the bullet list gets to become a > 2nd paragraph. > > WDYT? MAkes sense. I tried to fix my language, let me know if it makes sense. https://codereview.chromium.org/2339933004/diff/40001/storage/browser/blob/bl... storage/browser/blob/blob_memory_controller.h:74: // The bool argument is if we were able to successfuly receive quota. On 2016/09/21 22:56:59, pwnall wrote: > is if -> is true if? > > Please feel free to discard this feedback (and let me know) if this shorthand is > common in the codebase. Fixed. https://codereview.chromium.org/2339933004/diff/40001/storage/browser/blob/bl... storage/browser/blob/blob_memory_controller.h:109: // This reserves quota for the given |unreserved_memory_items|. The items must On 2016/09/21 22:56:59, pwnall wrote: > "This" seems unnecessary here, and diverges from the cadence established above. > Also, in method comments, I'd expect "this" to refer to the C++ this, e.g. "this > memory controller". > > If you agree, please also update the comments below. Done. https://codereview.chromium.org/2339933004/diff/40001/storage/browser/blob/bl... storage/browser/blob/blob_memory_controller.h:168: // Called when we've finished creating files for ReserveFileQuota. We make On 2016/09/21 22:56:59, pwnall wrote: > Everything after the first sentence seems like an implementation detail, which > makes me think it belongs to the body of the function. AFAIK, the comments in > the header should indicate how one would use the function. removed. https://codereview.chromium.org/2339933004/diff/40001/storage/browser/blob/bl... storage/browser/blob/blob_memory_controller.h:182: // We schedule paging until our memory usage is below our memory limit. We use On 2016/09/21 22:56:58, pwnall wrote: > "We" seems redundant. The first sentence seems to re-state the method name. The > following sentences appear to describe the implementation (and thus belong in > the definition). Done. https://codereview.chromium.org/2339933004/diff/60001/storage/browser/blob/bl... File storage/browser/blob/blob_memory_controller.cc (right): https://codereview.chromium.org/2339933004/diff/60001/storage/browser/blob/bl... storage/browser/blob/blob_memory_controller.cc:46: std::vector<BlobMemoryController::FileCreationInfo> result; On 2016/09/22 21:06:02, pwnall wrote: > Do you need to DCHECK that you're in the File thread (not the IO thread) here, > or are you relying on the File::GetInfo implementation to do that for you? > > After a (very) cursory reading, I can't figure out if the IO thread will trip > ThreadRestrictions::AssertIOAllowed or not. It will, I ran into this earlier. ADding the thread restriction check for both methods. https://codereview.chromium.org/2339933004/diff/60001/storage/browser/blob/bl... storage/browser/blob/blob_memory_controller.cc:162: void BlobMemoryController::DisableFilePaging() { On 2016/09/22 21:06:02, pwnall wrote: > Should this method also clear file_runner_ (to avoid using it accidentally > afterwards)? > > Also, why do you not need to cancel pending requests here? We do need to cancel them. I believe I fixed this. I'll clear the runner as well. https://codereview.chromium.org/2339933004/diff/60001/storage/browser/blob/bl... File storage/browser/blob/blob_memory_controller.h (right): https://codereview.chromium.org/2339933004/diff/60001/storage/browser/blob/bl... storage/browser/blob/blob_memory_controller.h:118: const MemoryQuotaRequestCallback& success_callback); On 2016/09/22 21:06:02, pwnall wrote: > Is it appropriate to call this param success_callback? It seems like it's called > with a boolean that indicates success/failure. Done.
The CQ bit was unchecked by commit-bot@chromium.org
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-...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
I don't think these comments will significantly impact your tests, but going through them might save you a tiny bit of work. https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/b... File storage/browser/blob/blob_memory_controller.cc (right): https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:49: // Creates a file in the given directory w/ the given filename and size. Why does this comment say "and size"? I understand why that behavior is desirable, but I don't see where that is implemented. I'd document that an empty vector is used to signal an error. Also, "_a_ file" doesn't seem to match the arguments / implementation. https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:55: for (scoped_refptr<ShareableFileReference>& file_ref : file_references) { You're using file_ref here, but file_reference in the method below. How about standardizing on file_reference? https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:57: BlobMemoryController::FileCreationInfo& creation_info = result.back(); Is this a common idiom? If not, would it be possible to collect the information in local variables, and construct+push_back the vector element at the end of the loop? https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:79: BlobMemoryController::FileCreationInfo WriteItemsToFile( Based on the UMA metrics inside, this should only be used to evict in-memory items to file storage. How about renaming the method to EvictItemsToFile? I think Evict is a bit better than Page because "page" isn't clear on its own -- you need to either write "page in" / "page out", or have extra context. https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:80: std::vector<scoped_refptr<ShareableBlobDataItem>>* items, You don't seem to be changing the vector in this method. Would const& be more appropriate than *? https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:85: base::ThreadRestrictions::AssertIOAllowed(); Sorry, I wasn't clear earlier. What I meant was -- the File implementations use base::ThreadRestrictions::AssertIOAllowed(), and I wasn't sure if this is strict enough (e.g., only true for the File thread, or also true for the IO thread). Would it make sense to use DCHECK_CURRENTLY_ON(BrowserThread::FILE) to make sure we're on the File thread? https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:87: // Create our file. our -> the page? https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:101: for (const auto& refptr : *items) { refptr -> shareable_blob_item_reference? https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:120: // Grab our modification time and create our SharedFileReference to manage the Where is the SharedFileReference created? https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:140: void GetFileSizesAndTotalSize( Would it make sense turn total_size_output into the return value? In that case, I suppose you'd rename to GetTotalSizeAndFileSizes(). https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:166: // Both tasks store a list iterator to their current position in the task list, This comment seems to re-state the contents of the private: section. How about replacing it with a comment stating that the list_position_ iterator (I think my_ is redundant) is stored so the task can remove itself from the list when it is finished / canceled? https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:172: BaseQuotaAllocationTask() : allocation_size_(0) {} Do you need the default constructor to be able to have a vector of tasks? https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:176: ~BaseQuotaAllocationTask() override {} would = default work here? https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:178: void set_my_list_position(typename T::iterator current_position) { The argument name seems odd, given everything else around it. https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:210: void RunSuccessCallback(bool success) const { RunDoneCallback? RunCallback? https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:216: done_callback_.Run(success); Does the task not need to remove itself from the pending list here? https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:219: void Cancel() override { Is it worth DCHECK-ing that Cancel() is called on the right thread? https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:221: // This call destorys this object. typo destorys -> destroys https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:233: // This task Eh? https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:263: std::string file_name = I think it'd be nice to have the filename vending logic (lines 264 and 266) in a separate method in the controller. (1) it seems to repeat on lines 608/612 (2) it'd be nice to have a DCHECK stating that current_file_num can only be accessed on the I/O thread (3) this'd give you an easy doc comment for current_file_num_ ("e.g., state for GenerateUniqueFilePath") https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:281: void RunSuccessCallback(bool success, RunDoneCallback? RunCallback? https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:288: // This call destorys this object. typo: "destorys" https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:289: controller_->pending_file_quota_tasks_.erase(my_list_position()); For me, the tricky part here was that destroying the object invalidates the WeakPtr serving as "this" for the OnCreateFiles callback, so Cancel() means that CreateFiles() gets called, but OnCreateFiles() doesn't. https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:292: void OnCreateFiles(std::vector<FileCreationInfo> result) { You don't seem to be removing the task from pending_file_quota_tasks_ when it gets to this callback. Would it make sense to move the Erase call from Cancel() to a "finally"-like method (as in try / catch / finally) and call that in both OnCreateFiles and Cancel()? If I understand the code correctly, you'd have to be a bit careful with sequencing. For example, currently, DisableFilePaging() (called below) would iterate over the current task, so the callback would be called twice. https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:297: done_callback_.Run(false, std::vector<FileCreationInfo>()); It seems odd that you have a method wrapping running the callback, and then you run the callback directly here. https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:310: done_callback_.Run(true, std::move(result)); Again, seems odd that you're not using RunSuccessCallback here. https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:338: DCHECK(!storage_directory.empty()); Should this also DCHECK that file paging was disabled? https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:417: if (!file_paging_enabled_) { It seems like the code in 418-423 mostly duplicates the code in 446-453, modulo the MaybeScheduleMaybeUntilSystemHealthy() call. I think you can kill the code by adding !file_paging_enabled_ to the if on line 445. Alternatively, given that GetAvailableMemoryForBlobs() is pretty fast, you can probably skip the check altogether. Based on my understanding of the code, pending_memory_quota_tasks_ gets emptied when file storage is disabled (checked in line 429), and the ReserveMemoryQuota() caller should have checked that there's enough quota, so the current check on 445 should trigger. https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:456: // This means we're too big for memory. "This means" appears to be redundant here. https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:567: // Don't do paging when others are happening, as we don't change our I like eviction more than paging here, both in the comment and in the pending_pagings_ variable name. This is because eviction == paging out, and there's also paging in. https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:577: // We only page when we have enough items to fill a while page file. while -> whole https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:585: // Collect our items. I think you could extract a method from lines 582-598, and move the comment into the method's name. For example: size_t BlobMemoryController::CollectItemsToBeEvicted(std::vector<scoped_refptr<ShareableBlobDataItem>>* items_to_be_evicted); https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:631: void BlobMemoryController::OnPagingComplete( Again, how about Paging -> Eviction? https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:647: // Switch from the data backing item to a new file backing item. data-backed items and file-backed items? https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:656: *(shareable_item->item_mutable()) = new_item; Why is this better than a set_item()? https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/b... File storage/browser/blob/blob_memory_controller.h (right): https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.h:184: void RecordTracingCounters() const; From the perspective of code review / maintenance, it'd be good to document when this is supposed to get called.
Thnx for splitting this up (and thxn victor for looking at it too). I haven't looked closely at the blob_memory_controller.cc changes yet. The QuotaAllocationTask is a nice addition. https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/b... File storage/browser/blob/blob_data_builder.cc (right): https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/b... storage/browser/blob/blob_data_builder.cc:40: return path.size() >= prefix.size() && not sure the size comparison condition needed, StartsWith implies this test built in. https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/b... storage/browser/blob/blob_data_builder.cc:41: base::StartsWith(path, prefix, base::CompareCase::SENSITIVE); is there any chance that "_future_name_" is valid input from elsewhere? maybe comment about how/why that can't happen https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/b... File storage/browser/blob/blob_data_builder.h (right): https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/b... storage/browser/blob/blob_data_builder.h:87: // which needs to be unique on a per-builder bases. Multiple items can have basis (sp) as worded the comments are a little confusing, file_id is unique but there can be duplicates. you explained this more clearly in an earlier review comment, "data for multiple items can be stored in the same file at different offsets". https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/b... File storage/browser/blob/blob_data_item.h (right): https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/b... storage/browser/blob/blob_data_item.h:32: : public base::RefCountedThreadSafe<BlobDataItem> { see comment elsewhere about thread-safe-refcounts https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/b... File storage/browser/blob/blob_memory_controller.cc (right): https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:35: using FileCreationInfo = storage::BlobMemoryController::FileCreationInfo; looks like most of the usages are fully qualified, consider removing the using statement https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:44: BlobMemoryController::MemoryQuotaRequestCallback; lines 41 and 43 are dups also is the using statement required for any of the usages? i think all usages of MemoryQuotaRequestCallback may be from within the class, in which case, is the using statement utlized? https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/b... File storage/browser/blob/blob_memory_controller.h (right): https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.h:81: class QuotaAllocationTask { thnx for adding this abstraction https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.h:83: virtual ~QuotaAllocationTask(); looks like the caller should never delete these, if thats right, it'd be good to make the dtor private https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.h:92: void EnableFilePaging(const base::FilePath& storage_directory, I mentioned this on the larger cl too, but it got lost in the noise. I think it'd be an improvement if this class internally managed the creation of the backing directory. The class could lazily create the dir on demand as the amount of storage being called for increased. I see two runtime benefits to that (plus it shrinks the public interface a little). 1. The dir may not have to be created at all depending on blob usage. Compared to always having to incur file system interactions during browser startup. 2. Consider the case when the very first request to reserve quota occurs prior to EnableFilePaging being called, and its too large to fit in memory. With the interface as it is, the controller would have to deny the requests. If the class managed the directory internally, it could asyncly setup the dir as part of satisfying that large request. Wdyt? https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.h:131: std::vector<ShareableBlobDataItem*> unreserved_file_items, should these be vectors of scoped_refptrs? https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.h:134: // This frees quota for items that don't have any blob references. the comment confused me a little, i think you mean "for items that are not referenced by any blobs" https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/s... File storage/browser/blob/shareable_blob_data_item.cc (right): https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/s... storage/browser/blob/shareable_blob_data_item.cc:19: ShareableBlobDataItem::ShareableBlobDataItem(const std::string& target_uuid, maybe name this "referencing_blob_uuid" for clarity https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/s... File storage/browser/blob/shareable_blob_data_item.h (right): https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/s... storage/browser/blob/shareable_blob_data_item.h:27: // file thread to save old memory items to disk. I understand the file created/deletion has to happen on a background thread, but I think these data structures should remain non-thread-safe refcounted. There's a containment hierarchy here. ShareableBlobDataItem BlobDataItem BlobDataItem::Handle Making the root of the hierarchy a thread-safe-refcount forces all of them to have thread-safe-refcounts. The leaf in that tree is an abstract class that consumers of the blob system define. I don't think we should impose the thread-safe requirement upon them. Can this interactions with the background thread to create/delete files be massaged to not require this class (or its containees) to have a thread-safe-refcount? We can't delete BlobDataItem::Handle's on other threads and the easiest way to ensure that is to make all of these structures single-threaded only.
some .cc file comments https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/b... File storage/browser/blob/blob_memory_controller.cc (right): https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:50: std::vector<BlobMemoryController::FileCreationInfo> CreateFiles( It wasn't clear to me that CreateFiles() and WriteItemsToFile() didn't operate on the same file paths. Maybe rename these and comment about usage? CreateEmtptyFiles, CreateFileAndWriteItems. // Used for new unpopulated file items. vector<CreationInfo> CreateEmtptyFiles(vector<FilePaths>& paths, vector<int64_t>& sizes) // Used to page multiple memory items out to a single file. CreationInfo CreateFileAndWriteItems(Filepath& path, vector<DataElement*>) Please try to rework the inputs/outputs of these two functions to not require direct access to the refcounted data types. I took a stab at using more primitive data types at this level ^^^. Of course higher level code (Tasks/Controller level stuff) would have to ensure ShareableFileReferernce and [Shareable]BlobDataItem lifetimes are properly managed on the IO thread. https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:219: void Cancel() override { On 2016/09/26 13:18:08, pwnall wrote: > Is it worth DCHECK-ing that Cancel() is called on the right thread? i' rather see so very little done on background threads and the inputs/outputs so well defined, that it's obvious an exposed method like this is not called on the background thread all the blob system is strictly single threaded with the exception the file creation and 'paging' logic buried inside this class. This thinking plays into why I'd prefer to see all those structs remain RefCounted<> too. https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:350: memory_request->RunSuccessCallback(false); you might want to swap the queues into local variables while calling the completion callbacks to avoid the possibility of the lists being added to reentrantly (and also get all internal state variables below into a consistent state prior to calling out) https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:356: } if there are callbacks in flight from the background thread, would it make sense to reset the weakptr_factory (invalidate all outstanding weakptrs) here too so those get dropped https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:390: return Strategy::IPC; short and sweet, thnx for neutering the comments :) https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:407: item->item()->type() == DataElement::TYPE_BYTES); would DCHECK(length() > 0) be correct here, otherwise is it meaningful to have 0 byte length item in the REQUESTED state (argh, i can never remember if 0 length items are possible or not) https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:524: void BlobMemoryController::UpdateBlobItemInRecents( maybe rename the method to make it more clear the kind of items this is intended to accept and comment about the prescribed usage (in the .h file)... // Called when initially populated or upon later access. void NotifyMemoryItemUsed(populated_memory_item) https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/b... File storage/browser/blob/blob_memory_controller.h (right): https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.h:144: void RemoveBlobItemInRecents(const ShareableBlobDataItem& item); is the remove call needed or could this happen internally as an artifact of the call to Free stuff? https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.h:219: base::WeakPtrFactory<BlobMemoryController> ptr_factory_; consistency, typically in storage/browser/blob, this kind of data member is called weak_factory_
https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/b... File storage/browser/blob/blob_memory_controller.cc (right): https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:47: void DestructFiles(std::vector<BlobMemoryController::FileCreationInfo> files) {} I don't see this being used anywhere? It might make sense for ~FileCreationInfo to take care to destruct its base::File data member (if IsValid) on the background thread? The structs scoped_refptr<ShareableFileReference> data member (if non-null) should not be destructed on the background thread, so conflicting requirements.
The CQ bit was checked by dmurph@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_androi...)
Done! My next patch will start having more tests. Thanks for the comments! https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/b... File storage/browser/blob/blob_data_builder.cc (right): https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/b... storage/browser/blob/blob_data_builder.cc:40: return path.size() >= prefix.size() && On 2016/09/27 00:09:29, michaeln wrote: > not sure the size comparison condition needed, StartsWith implies this test > built in. Ah, right.Done. https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/b... storage/browser/blob/blob_data_builder.cc:41: base::StartsWith(path, prefix, base::CompareCase::SENSITIVE); On 2016/09/27 00:09:29, michaeln wrote: > is there any chance that "_future_name_" is valid input from elsewhere? maybe > comment about how/why that can't happen I mean, it's technically valid. I can DCHECK in AppendFile to make sure. Not sure what else I can do. https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/b... File storage/browser/blob/blob_data_builder.h (right): https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/b... storage/browser/blob/blob_data_builder.h:87: // which needs to be unique on a per-builder bases. Multiple items can have On 2016/09/27 00:09:29, michaeln wrote: > basis (sp) > as worded the comments are a little confusing, file_id is unique but there can > be duplicates. you explained this more clearly in an earlier review comment, > "data for multiple items can be stored in the same file at different offsets". Ah, yeah. I tried to make this more clear, let me know if it makes sense now. https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/b... File storage/browser/blob/blob_data_item.h (right): https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/b... storage/browser/blob/blob_data_item.h:32: : public base::RefCountedThreadSafe<BlobDataItem> { On 2016/09/27 00:09:29, michaeln wrote: > see comment elsewhere about thread-safe-refcounts Yes, you're right, I can remove this. https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/b... File storage/browser/blob/blob_memory_controller.cc (right): https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:35: using FileCreationInfo = storage::BlobMemoryController::FileCreationInfo; On 2016/09/27 00:09:29, michaeln wrote: > looks like most of the usages are fully qualified, consider removing the using > statement It shortens a lot of code, I unqualified the instances, hopefully that helps. https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:44: BlobMemoryController::MemoryQuotaRequestCallback; On 2016/09/27 00:09:29, michaeln wrote: > lines 41 and 43 are dups > also is the using statement required for any of the usages? i think all usages > of MemoryQuotaRequestCallback may be from within the class, in which case, is > the using statement utlized? Done. https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:47: void DestructFiles(std::vector<BlobMemoryController::FileCreationInfo> files) {} On 2016/09/28 21:19:27, michaeln wrote: > I don't see this being used anywhere? > It might make sense for ~FileCreationInfo to take care to destruct its > base::File data member (if IsValid) on the background thread? The structs > scoped_refptr<ShareableFileReference> data member (if non-null) should not be > destructed on the background thread, so conflicting requirements. > You're right. This is no longer used, so I'll remove it. https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:50: std::vector<BlobMemoryController::FileCreationInfo> CreateFiles( On 2016/09/28 01:08:04, michaeln wrote: > It wasn't clear to me that CreateFiles() and WriteItemsToFile() didn't operate > on the same file paths. Maybe rename these and comment about usage? > CreateEmtptyFiles, CreateFileAndWriteItems. > > // Used for new unpopulated file items. > vector<CreationInfo> CreateEmtptyFiles(vector<FilePaths>& paths, > vector<int64_t>& sizes) > > // Used to page multiple memory items out to a single file. > CreationInfo CreateFileAndWriteItems(Filepath& path, vector<DataElement*>) > > Please try to rework the inputs/outputs of these two functions to not require > direct access to the refcounted data types. I took a stab at using more > primitive data types at this level ^^^. Of course higher level code > (Tasks/Controller level stuff) would have to ensure ShareableFileReferernce and > [Shareable]BlobDataItem lifetimes are properly managed on the IO thread. Sounds good. Thanks for the chat about this, I think it's looking better now. https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:55: for (scoped_refptr<ShareableFileReference>& file_ref : file_references) { On 2016/09/26 13:18:07, pwnall wrote: > You're using file_ref here, but file_reference in the method below. How about > standardizing on file_reference? Done. https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:57: BlobMemoryController::FileCreationInfo& creation_info = result.back(); On 2016/09/26 13:18:08, pwnall wrote: > Is this a common idiom? If not, would it be possible to collect the information > in local variables, and construct+push_back the vector element at the end of the > loop? Yes it's somewhat common, as you don't have redundant memory usage. It's a little easier w/ the move constructor now, so I can change it. https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:79: BlobMemoryController::FileCreationInfo WriteItemsToFile( On 2016/09/26 13:18:08, pwnall wrote: > Based on the UMA metrics inside, this should only be used to evict in-memory > items to file storage. How about renaming the method to EvictItemsToFile? I > think Evict is a bit better than Page because "page" isn't clear on its own -- > you need to either write "page in" / "page out", or have extra context. Done. https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:80: std::vector<scoped_refptr<ShareableBlobDataItem>>* items, On 2016/09/26 13:18:08, pwnall wrote: > You don't seem to be changing the vector in this method. Would const& be more > appropriate than *? I think I can do this now. The callback actually saves const ref arguments by value internally, but if I use base::ConstRef as a wrapper on the bind-site it'll work. https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:85: base::ThreadRestrictions::AssertIOAllowed(); On 2016/09/26 13:18:08, pwnall wrote: > Sorry, I wasn't clear earlier. > > What I meant was -- the File implementations use > base::ThreadRestrictions::AssertIOAllowed(), and I wasn't sure if this is strict > enough (e.g., only true for the File thread, or also true for the IO thread). > > Would it make sense to use DCHECK_CURRENTLY_ON(BrowserThread::FILE) to make sure > we're on the File thread? Ah, yeah we don't have access to that here, that's in content/ land and we're in storage/ https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:87: // Create our file. On 2016/09/26 13:18:09, pwnall wrote: > our -> the page? Done. https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:101: for (const auto& refptr : *items) { On 2016/09/26 13:18:07, pwnall wrote: > refptr -> shareable_blob_item_reference? Done. https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:120: // Grab our modification time and create our SharedFileReference to manage the On 2016/09/26 13:18:09, pwnall wrote: > Where is the SharedFileReference created? Ah, this was moved out. https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:140: void GetFileSizesAndTotalSize( On 2016/09/26 13:18:09, pwnall wrote: > Would it make sense turn total_size_output into the return value? In that case, > I suppose you'd rename to GetTotalSizeAndFileSizes(). Sure. https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:166: // Both tasks store a list iterator to their current position in the task list, On 2016/09/26 13:18:07, pwnall wrote: > This comment seems to re-state the contents of the private: section. How about > replacing it with a comment stating that the list_position_ iterator (I think > my_ is redundant) is stored so the task can remove itself from the list when it > is finished / canceled? Done. https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:172: BaseQuotaAllocationTask() : allocation_size_(0) {} On 2016/09/26 13:18:09, pwnall wrote: > Do you need the default constructor to be able to have a vector of tasks? No, we need it for FileQuotaAllocationTask, where we don't have the allocation size right away. https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:176: ~BaseQuotaAllocationTask() override {} On 2016/09/26 13:18:08, pwnall wrote: > would = default work here? Done. https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:178: void set_my_list_position(typename T::iterator current_position) { On 2016/09/26 13:18:08, pwnall wrote: > The argument name seems odd, given everything else around it. Done. https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:210: void RunSuccessCallback(bool success) const { On 2016/09/26 13:18:08, pwnall wrote: > RunDoneCallback? RunCallback? Done. https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:216: done_callback_.Run(success); On 2016/09/26 13:18:08, pwnall wrote: > Does the task not need to remove itself from the pending list here? No, we do that externally as it'll invalidate the iterators :( https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:219: void Cancel() override { On 2016/09/28 01:08:03, michaeln wrote: > On 2016/09/26 13:18:08, pwnall wrote: > > Is it worth DCHECK-ing that Cancel() is called on the right thread? > > i' rather see so very little done on background threads and the inputs/outputs > so well defined, that it's obvious an exposed method like this is not called on > the background thread > > all the blob system is strictly single threaded with the exception the file > creation and 'paging' logic buried inside this class. This thinking plays into > why I'd prefer to see all those structs remain RefCounted<> too. I'll put a comment about IO thread expectations in the header for the whole class. https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:221: // This call destorys this object. On 2016/09/26 13:18:09, pwnall wrote: > typo destorys -> destroys Done. https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:233: // This task On 2016/09/26 13:18:09, pwnall wrote: > Eh? Done. https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:263: std::string file_name = On 2016/09/26 13:18:08, pwnall wrote: > I think it'd be nice to have the filename vending logic (lines 264 and 266) in a > separate method in the controller. > > (1) it seems to repeat on lines 608/612 > (2) it'd be nice to have a DCHECK stating that current_file_num can only be > accessed on the I/O thread > (3) this'd give you an easy doc comment for current_file_num_ ("e.g., state for > GenerateUniqueFilePath") Sounds good. I can't quite do the checks, but I can document that. https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:281: void RunSuccessCallback(bool success, On 2016/09/26 13:18:08, pwnall wrote: > RunDoneCallback? RunCallback? Done. https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:288: // This call destorys this object. On 2016/09/26 13:18:07, pwnall wrote: > typo: "destorys" Done. https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:289: controller_->pending_file_quota_tasks_.erase(my_list_position()); On 2016/09/26 13:18:07, pwnall wrote: > For me, the tricky part here was that destroying the object invalidates the > WeakPtr serving as "this" for the OnCreateFiles callback, so Cancel() means that > CreateFiles() gets called, but OnCreateFiles() doesn't. Yes, this is why I documented that calling 'Cancel' will NOT call the success callback. https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:292: void OnCreateFiles(std::vector<FileCreationInfo> result) { On 2016/09/26 13:18:09, pwnall wrote: > You don't seem to be removing the task from pending_file_quota_tasks_ when it > gets to this callback. Would it make sense to move the Erase call from Cancel() > to a "finally"-like method (as in try / catch / finally) and call that in both > OnCreateFiles and Cancel()? > > If I understand the code correctly, you'd have to be a bit careful with > sequencing. For example, currently, DisableFilePaging() (called below) would > iterate over the current task, so the callback would be called twice. Good point about sequencing. Fixed. https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:297: done_callback_.Run(false, std::vector<FileCreationInfo>()); On 2016/09/26 13:18:08, pwnall wrote: > It seems odd that you have a method wrapping running the callback, and then you > run the callback directly here. removed. https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:310: done_callback_.Run(true, std::move(result)); On 2016/09/26 13:18:09, pwnall wrote: > Again, seems odd that you're not using RunSuccessCallback here. Done. https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:338: DCHECK(!storage_directory.empty()); On 2016/09/26 13:18:07, pwnall wrote: > Should this also DCHECK that file paging was disabled? Done. https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:350: memory_request->RunSuccessCallback(false); On 2016/09/28 01:08:03, michaeln wrote: > you might want to swap the queues into local variables while calling the > completion callbacks to avoid the possibility of the lists being added to > reentrantly (and also get all internal state variables below into a consistent > state prior to calling out) Done. https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:356: } On 2016/09/28 01:08:03, michaeln wrote: > if there are callbacks in flight from the background thread, would it make sense > to reset the weakptr_factory (invalidate all outstanding weakptrs) here too so > those get dropped Done. https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:407: item->item()->type() == DataElement::TYPE_BYTES); On 2016/09/28 01:08:04, michaeln wrote: > would DCHECK(length() > 0) be correct here, otherwise is it meaningful to have 0 > byte length item in the REQUESTED state (argh, i can never remember if 0 length > items are possible or not) Not possible, but good to DCHECK everywhere just in case. https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:417: if (!file_paging_enabled_) { On 2016/09/26 13:18:09, pwnall wrote: > It seems like the code in 418-423 mostly duplicates the code in 446-453, modulo > the MaybeScheduleMaybeUntilSystemHealthy() call. > > I think you can kill the code by adding !file_paging_enabled_ to the if on line > 445. Alternatively, given that GetAvailableMemoryForBlobs() is pretty fast, you > can probably skip the check altogether. Based on my understanding of the code, > pending_memory_quota_tasks_ gets emptied when file storage is disabled (checked > in line 429), and the ReserveMemoryQuota() caller should have checked that > there's enough quota, so the current check on 445 should trigger. Nice idea, done. https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:456: // This means we're too big for memory. On 2016/09/26 13:18:07, pwnall wrote: > "This means" appears to be redundant here. Done. https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:524: void BlobMemoryController::UpdateBlobItemInRecents( On 2016/09/28 01:08:04, michaeln wrote: > maybe rename the method to make it more clear the kind of items this is intended > to accept and comment about the prescribed usage (in the .h file)... > > // Called when initially populated or upon later access. > void NotifyMemoryItemUsed(populated_memory_item) Good idea, thanks. https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:567: // Don't do paging when others are happening, as we don't change our On 2016/09/26 13:18:09, pwnall wrote: > I like eviction more than paging here, both in the comment and in the > pending_pagings_ variable name. This is because eviction == paging out, and > there's also paging in. Done. https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:577: // We only page when we have enough items to fill a while page file. On 2016/09/26 13:18:07, pwnall wrote: > while -> whole Done. https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:585: // Collect our items. On 2016/09/26 13:18:09, pwnall wrote: > I think you could extract a method from lines 582-598, and move the comment into > the method's name. For example: > > size_t > BlobMemoryController::CollectItemsToBeEvicted(std::vector<scoped_refptr<ShareableBlobDataItem>>* > items_to_be_evicted); Done. https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:631: void BlobMemoryController::OnPagingComplete( On 2016/09/26 13:18:08, pwnall wrote: > Again, how about Paging -> Eviction? Done. https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:647: // Switch from the data backing item to a new file backing item. On 2016/09/26 13:18:08, pwnall wrote: > data-backed items and file-backed items? Done. https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:656: *(shareable_item->item_mutable()) = new_item; On 2016/09/26 13:18:09, pwnall wrote: > Why is this better than a set_item()? Done. https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/b... File storage/browser/blob/blob_memory_controller.h (right): https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.h:81: class QuotaAllocationTask { On 2016/09/27 00:09:29, michaeln wrote: > thnx for adding this abstraction Acknowledged. https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.h:83: virtual ~QuotaAllocationTask(); On 2016/09/27 00:09:29, michaeln wrote: > looks like the caller should never delete these, if thats right, it'd be good to > make the dtor private Done. Making protected so subclasses can call it, as well as unique_ptr. https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.h:92: void EnableFilePaging(const base::FilePath& storage_directory, On 2016/09/27 00:09:30, michaeln wrote: > I mentioned this on the larger cl too, but it got lost in the noise. I think > it'd be an improvement if this class internally managed the creation of the > backing directory. The class could lazily create the dir on demand as the amount > of storage being called for increased. I see two runtime benefits to that (plus > it shrinks the public interface a little). > > 1. The dir may not have to be created at all depending on blob usage. Compared > to always having to incur file system interactions during browser startup. > > 2. Consider the case when the very first request to reserve quota occurs prior > to EnableFilePaging being called, and its too large to fit in memory. With the > interface as it is, the controller would have to deny the requests. If the class > managed the directory internally, it could asyncly setup the dir as part of > satisfying that large request. > > Wdyt? Sounds good, I put these into the constructor, and to have paging disabled then you put a nullptr as the task runner. https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.h:131: std::vector<ShareableBlobDataItem*> unreserved_file_items, On 2016/09/27 00:09:29, michaeln wrote: > should these be vectors of scoped_refptrs? Hmm.. Maybe we should, as we grab a reference anyways. Does that mean we should make the above method match? That one usually won't be async, so the refptrs aren't usually needed (and then we grab one if we need the async version). https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.h:134: // This frees quota for items that don't have any blob references. On 2016/09/27 00:09:29, michaeln wrote: > the comment confused me a little, i think you mean "for items that are not > referenced by any blobs" Done. https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.h:144: void RemoveBlobItemInRecents(const ShareableBlobDataItem& item); On 2016/09/28 01:08:04, michaeln wrote: > is the remove call needed or could this happen internally as an artifact of the > call to Free stuff? Yes it can! woo! https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.h:184: void RecordTracingCounters() const; On 2016/09/26 13:18:09, pwnall wrote: > From the perspective of code review / maintenance, it'd be good to document when > this is supposed to get called. Done. https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.h:219: base::WeakPtrFactory<BlobMemoryController> ptr_factory_; On 2016/09/28 01:08:04, michaeln wrote: > consistency, typically in storage/browser/blob, this kind of data member is > called weak_factory_ Done. https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/s... File storage/browser/blob/shareable_blob_data_item.cc (right): https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/s... storage/browser/blob/shareable_blob_data_item.cc:19: ShareableBlobDataItem::ShareableBlobDataItem(const std::string& target_uuid, On 2016/09/27 00:09:30, michaeln wrote: > maybe name this "referencing_blob_uuid" for clarity Done. https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/s... File storage/browser/blob/shareable_blob_data_item.h (right): https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/s... storage/browser/blob/shareable_blob_data_item.h:27: // file thread to save old memory items to disk. On 2016/09/27 00:09:30, michaeln wrote: > I understand the file created/deletion has to happen on a background thread, but > I think these data structures should remain non-thread-safe refcounted. There's > a containment hierarchy here. > > ShareableBlobDataItem > BlobDataItem > BlobDataItem::Handle > > Making the root of the hierarchy a thread-safe-refcount forces all of them to > have thread-safe-refcounts. The leaf in that tree is an abstract class that > consumers of the blob system define. I don't think we should impose the > thread-safe requirement upon them. > > Can this interactions with the background thread to create/delete files be > massaged to not require this class (or its containees) to have a > thread-safe-refcount? We can't delete BlobDataItem::Handle's on other threads > and the easiest way to ensure that is to make all of these structures > single-threaded only. Done. Thanks for the discussion, I'm glad we could keep things single-threaded.
this is looking pretty good to me, i have a couple nits and am wondering if you want to add more tests now or later i'd like to take a final pass at it still, lmk if you're happy with where things stand and are ready to land it https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/b... File storage/browser/blob/blob_data_builder.cc (right): https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/b... storage/browser/blob/blob_data_builder.cc:41: base::StartsWith(path, prefix, base::CompareCase::SENSITIVE); > I mean, it's technically valid. I can DCHECK in AppendFile to make sure. Not > sure what else I can do. This seems like something we have to resolve? I don't think we should be able to confuse actual input with _future_files_. Are actual inputs required to be absolute paths? If so, I think we're good. https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/s... File storage/browser/blob/shareable_blob_data_item.h (right): https://codereview.chromium.org/2339933004/diff/120001/storage/browser/blob/s... storage/browser/blob/shareable_blob_data_item.h:27: // file thread to save old memory items to disk. On 2016/09/29 00:44:23, dmurph wrote: > On 2016/09/27 00:09:30, michaeln wrote: > > I understand the file created/deletion has to happen on a background thread, > but > > I think these data structures should remain non-thread-safe refcounted. > There's > > a containment hierarchy here. > > > > ShareableBlobDataItem > > BlobDataItem > > BlobDataItem::Handle > > > > Making the root of the hierarchy a thread-safe-refcount forces all of them to > > have thread-safe-refcounts. The leaf in that tree is an abstract class that > > consumers of the blob system define. I don't think we should impose the > > thread-safe requirement upon them. > > > > Can this interactions with the background thread to create/delete files be > > massaged to not require this class (or its containees) to have a > > thread-safe-refcount? We can't delete BlobDataItem::Handle's on other threads > > and the easiest way to ensure that is to make all of these structures > > single-threaded only. > > Done. Thanks for the discussion, I'm glad we could keep things single-threaded. woot! https://codereview.chromium.org/2339933004/diff/140001/content/browser/blob_s... File content/browser/blob_storage/blob_memory_controller_unittest.cc (right): https://codereview.chromium.org/2339933004/diff/140001/content/browser/blob_s... content/browser/blob_storage/blob_memory_controller_unittest.cc:40: ASSERT_EQ(true, base::CreateNewTempDirectory("blob_storage", &temp_dir_)); i think base::ScopedTempDir would be a good fit for this https://codereview.chromium.org/2339933004/diff/140001/content/browser/blob_s... content/browser/blob_storage/blob_memory_controller_unittest.cc:224: // * Write test for memory request when quota is available. do you plan on filling in more test coverage in this cl or leaving for later? https://codereview.chromium.org/2339933004/diff/140001/storage/browser/blob/b... File storage/browser/blob/blob_memory_controller.cc (right): https://codereview.chromium.org/2339933004/diff/140001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:107: creation_info.path = std::move(file_path); stupid c++ question? does this do anything the callers underlying file_path value? seems like this might move the constref such that the file_path local is not longer refers to the underlaying path data. i don't quite get it? aren't we going to have to copy the bytes into the creation_info struct. { const FilePath path("fred"); CreateFileAndWriteItems(..., path, ...); assert(path == "fred"); // this assertion holds, right? } https://codereview.chromium.org/2339933004/diff/140001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:144: std::vector<scoped_refptr<ShareableBlobDataItem>> WrapInRefPtrs( if the sig of the methods are changed, might not need this helper https://codereview.chromium.org/2339933004/diff/140001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:171: file_sizes_output->push_back(size_pair.second); does the order of values in the array matter? https://codereview.chromium.org/2339933004/diff/140001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:274: controller_->disk_used_ += total_size; nit: i was looking around for += allocation_size() to balance the -= allocation_size() statements elsewhere https://codereview.chromium.org/2339933004/diff/140001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:440: if (!pending_memory_quota_tasks_.empty()) { it'd might be good to share this block of code with the block below at line 471, amybe a helper function to AppendMemoryTask() https://codereview.chromium.org/2339933004/diff/140001/storage/browser/blob/b... File storage/browser/blob/blob_memory_controller.h (right): https://codereview.chromium.org/2339933004/diff/140001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.h:48: // (UpdateBlobItemInRecents, RemoveBlobItemInRecents). the methods names in the comment are stale now https://codereview.chromium.org/2339933004/diff/140001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.h:83: // IO thread only. since the outerclass says io only, maybe this comment isn't needed here https://codereview.chromium.org/2339933004/diff/140001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.h:84: class QuotaAllocationTask { style nit: declaration order, i think the struct and class should be adjacent to one another and the using alias's coming after https://codereview.chromium.org/2339933004/diff/140001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.h:123: std::vector<ShareableBlobDataItem*> unreserved_memory_items, would it work to make the vector a const ref? its funky that the unittests constructs a vector a scoped_refs and seperated has to muck with a vector of ptrs https://codereview.chromium.org/2339933004/diff/140001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.h:135: std::vector<scoped_refptr<ShareableBlobDataItem>> unreserved_file_items, i'd suggest a const ref vector<> here too for symmetry with the other method, they're essentially the same thing, an arbitrary difference calls unnecessary attention. making them look boringly alike might be good https://codereview.chromium.org/2339933004/diff/140001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.h:154: void SetLimitsForTesting(BlobStorageLimits limits) { limits_ = limits; } nit: unix_hacker_naming() https://codereview.chromium.org/2339933004/diff/140001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.h:229: // item to the recent_item_cache_ above. would it make sense to represent this as a distinct ShareableBlobDataItem state value instead of in this seperate collection? nevermind, just looked at those state values, probably not a good idea to mix in type transitions to that set of values.
The CQ bit was checked by dmurph@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_comp...)
The CQ bit was checked by dmurph@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Hello! I added some more tests, PTAL. I'm mostly concerned w/ the new DeleteOnTaskRunnerIfControllerGone methods. I need a way to destroy the return value (except for the shareable file item) on the File thread if the callback object is gone. I was trying to find the best way to do this - not sure what that is though. Thanks! Daniel https://codereview.chromium.org/2339933004/diff/140001/content/browser/blob_s... File content/browser/blob_storage/blob_memory_controller_unittest.cc (right): https://codereview.chromium.org/2339933004/diff/140001/content/browser/blob_s... content/browser/blob_storage/blob_memory_controller_unittest.cc:40: ASSERT_EQ(true, base::CreateNewTempDirectory("blob_storage", &temp_dir_)); On 2016/10/04 00:11:54, michaeln wrote: > i think base::ScopedTempDir would be a good fit for this Ah, nice catch. https://codereview.chromium.org/2339933004/diff/140001/content/browser/blob_s... content/browser/blob_storage/blob_memory_controller_unittest.cc:224: // * Write test for memory request when quota is available. On 2016/10/04 00:11:54, michaeln wrote: > do you plan on filling in more test coverage in this cl or leaving for later? This cl :) https://codereview.chromium.org/2339933004/diff/140001/storage/browser/blob/b... File storage/browser/blob/blob_memory_controller.cc (right): https://codereview.chromium.org/2339933004/diff/140001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:107: creation_info.path = std::move(file_path); On 2016/10/04 00:11:54, michaeln wrote: > stupid c++ question? does this do anything the callers underlying file_path > value? seems like this might move the constref such that the file_path local is > not longer refers to the underlaying path data. i don't quite get it? aren't we > going to have to copy the bytes into the creation_info struct. > > { > const FilePath path("fred"); > CreateFileAndWriteItems(..., path, ...); > assert(path == "fred"); // this assertion holds, right? > } Ah, actually, this does a copy. If we std::move'd it into this method, then yes, that file path would be gone in the caller. But because we're given a const ref, then this doesn't actually change that object and does a copy-then-move, which is weird. https://codereview.chromium.org/2339933004/diff/140001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:144: std::vector<scoped_refptr<ShareableBlobDataItem>> WrapInRefPtrs( On 2016/10/04 00:11:54, michaeln wrote: > if the sig of the methods are changed, might not need this helper Done. https://codereview.chromium.org/2339933004/diff/140001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:171: file_sizes_output->push_back(size_pair.second); On 2016/10/04 00:11:55, michaeln wrote: > does the order of values in the array matter? Yeah, it needs to be in file_id order, which is enforced by SmallMap<map<>> https://codereview.chromium.org/2339933004/diff/140001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:274: controller_->disk_used_ += total_size; On 2016/10/04 00:11:54, michaeln wrote: > nit: i was looking around for += allocation_size() to balance the -= > allocation_size() statements elsewhere Done. https://codereview.chromium.org/2339933004/diff/140001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:440: if (!pending_memory_quota_tasks_.empty()) { On 2016/10/04 00:11:54, michaeln wrote: > it'd might be good to share this block of code with the block below at line 471, > amybe a helper function to AppendMemoryTask() Done. https://codereview.chromium.org/2339933004/diff/140001/storage/browser/blob/b... File storage/browser/blob/blob_memory_controller.h (right): https://codereview.chromium.org/2339933004/diff/140001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.h:48: // (UpdateBlobItemInRecents, RemoveBlobItemInRecents). On 2016/10/04 00:11:55, michaeln wrote: > the methods names in the comment are stale now Done. https://codereview.chromium.org/2339933004/diff/140001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.h:83: // IO thread only. On 2016/10/04 00:11:55, michaeln wrote: > since the outerclass says io only, maybe this comment isn't needed here Done. https://codereview.chromium.org/2339933004/diff/140001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.h:84: class QuotaAllocationTask { On 2016/10/04 00:11:55, michaeln wrote: > style nit: declaration order, i think the struct and class should be adjacent to > one another and the using alias's coming after Done. https://codereview.chromium.org/2339933004/diff/140001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.h:123: std::vector<ShareableBlobDataItem*> unreserved_memory_items, On 2016/10/04 00:11:55, michaeln wrote: > would it work to make the vector a const ref? its funky that the unittests > constructs a vector a scoped_refs and seperated has to muck with a vector of > ptrs So the file quota one always needs to grab a copy of the items, so if we want them to be the same I'd like to keep this as a vector by-value. I then std::move it if I need to, but the bad side of this matching is that I'm increasing the refcount of the items when I don't necessarily need to keep them (whereas this is always the case in the file quota one). https://codereview.chromium.org/2339933004/diff/140001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.h:135: std::vector<scoped_refptr<ShareableBlobDataItem>> unreserved_file_items, On 2016/10/04 00:11:55, michaeln wrote: > i'd suggest a const ref vector<> here too for symmetry with the other method, > they're essentially the same thing, an arbitrary difference calls unnecessary > attention. making them look boringly alike might be good So we want to save this vector and send it to another thread, so I like sending it by value and using std::move. https://codereview.chromium.org/2339933004/diff/140001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.h:154: void SetLimitsForTesting(BlobStorageLimits limits) { limits_ = limits; } On 2016/10/04 00:11:55, michaeln wrote: > nit: unix_hacker_naming() Done. https://codereview.chromium.org/2339933004/diff/140001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.h:229: // item to the recent_item_cache_ above. On 2016/10/04 00:11:55, michaeln wrote: > would it make sense to represent this as a distinct ShareableBlobDataItem state > value instead of in this seperate collection? nevermind, just looked at those > state values, probably not a good idea to mix in type transitions to that set of > values. Acknowledged.
The CQ bit was unchecked by commit-bot@chromium.org
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_androi...)
The CQ bit was checked by dmurph@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_androi...)
The CQ bit was checked by dmurph@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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/...)
hey, i'll take a look this afternoon
this looks really good, i only have one substantive comment about how memory quota is reclaimed https://codereview.chromium.org/2339933004/diff/200001/storage/browser/blob/b... File storage/browser/blob/blob_data_item.h (right): https://codereview.chromium.org/2339933004/diff/200001/storage/browser/blob/b... storage/browser/blob/blob_data_item.h:46: explicit BlobDataItem(std::unique_ptr<DataElement> item); do these need to be public? what are the new consumers that need to cons these up? the fact that they're private is a good indication to random readers that they probably won't need to construct these objects themselves (from the point of view of using the storage lib rather than implementing the storage lib). https://codereview.chromium.org/2339933004/diff/200001/storage/browser/blob/b... File storage/browser/blob/blob_memory_controller.cc (right): https://codereview.chromium.org/2339933004/diff/200001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:47: UMA_HISTOGRAM_ENUMERATION("Storage.Blob.DirectorySuccess.Error", -error, i think you could merge these two stats into one, Blob.CreateDirectroyResult and 0 corresponds with FILE_OK https://codereview.chromium.org/2339933004/diff/200001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:103: UMA_HISTOGRAM_MEMORY_KB("Storage.Blob.PageFileSize", total_size_bytes / 1024); this stat looks interesting https://codereview.chromium.org/2339933004/diff/200001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:144: UMA_HISTOGRAM_BOOLEAN("Storage.Blob.PageFileInfoSuccess", success); I'm not sure these functions warrant 8 many uma stats? If we ever see problems here we can intrument in greater detail, but to start with, I think we should not have so many. Maybe pair it down to... CreateDirectoryResult PagingDisabled (File::Error) PageFileSize https://codereview.chromium.org/2339933004/diff/200001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:236: can these be private? https://codereview.chromium.org/2339933004/diff/200001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:435: for (const scoped_refptr<ShareableBlobDataItem>& item : could use const auto& here, might be rid of a line wrap https://codereview.chromium.org/2339933004/diff/200001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:461: // If we're past our blob memory limit, then schedule our paging. this comment seems out of place, looks like it belongs inside of MaybeScheduleEvictionUntilSystemHealthy? https://codereview.chromium.org/2339933004/diff/200001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:463: for (const scoped_refptr<ShareableBlobDataItem>& item : ditto const auto& https://codereview.chromium.org/2339933004/diff/200001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:539: recent_item_cache_bytes_ += static_cast<size_t>(item->item()->length()); nit: i think the name of the "recent_item" data members might be a little more informative? Really the collection is *all* POPULATED_WITH_QUOTA items (not in the act of being paged out), not just recent ones, is that right? maybe populated_memory_items_? https://codereview.chromium.org/2339933004/diff/200001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:575: void BlobMemoryController::MaybeGrantPendingQuotaRequests() { since it only deals with memory requests, MaybeGrantPendingMemoryRequests? https://codereview.chromium.org/2339933004/diff/200001/storage/browser/blob/b... File storage/browser/blob/blob_memory_controller.h (right): https://codereview.chromium.org/2339933004/diff/200001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.h:139: void MaybeFreeMemoryQuotaForItems( What do think about making the "free" call a function of the ShareableBlobDataItem? That might eliminate the maybeness and be less error prone. It's nice the file quota is returned automatically when the file is no longer needed. I think would be nice of mem quota was similarly reclaimed automatically without the caller having to do something extra. https://codereview.chromium.org/2339933004/diff/200001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.h:142: void ForceFreeMemoryQuotaForItem( what is the usecase for this method? i don't see it being used https://codereview.chromium.org/2339933004/diff/200001/storage/browser/blob/i... File storage/browser/blob/internal_blob_data.cc (right): https://codereview.chromium.org/2339933004/diff/200001/storage/browser/blob/i... storage/browser/blob/internal_blob_data.cc:61: data_item->referencing_blobs_mutable()->erase(blob_uuid); Would it be less error prone to "maybe free" each item when its collection of referencing blobs becomes empty? https://codereview.chromium.org/2339933004/diff/200001/storage/browser/blob/s... File storage/browser/blob/shareable_blob_data_item.h (right): https://codereview.chromium.org/2339933004/diff/200001/storage/browser/blob/s... storage/browser/blob/shareable_blob_data_item.h:26: // We need to be RefCountedThreadSafe as references of this item are passed to a comment is stale
looks good overall to me too https://codereview.chromium.org/2339933004/diff/200001/storage/browser/blob/b... File storage/browser/blob/blob_data_builder.cc (right): https://codereview.chromium.org/2339933004/diff/200001/storage/browser/blob/b... storage/browser/blob/blob_data_builder.cc:28: const char kFutureFileName[] = "_future_name_"; nit: do we want to use FilePath::CharType kFutureFileName[] = FILE_PATH_LITERAL("_future_name_") instead and remove some of string conversions below? https://codereview.chromium.org/2339933004/diff/200001/storage/browser/blob/b... File storage/browser/blob/blob_data_builder.h (right): https://codereview.chromium.org/2339933004/diff/200001/storage/browser/blob/b... storage/browser/blob/blob_data_builder.h:88: // between diffent 'future' files that will be used to store data for this nit: diffent -> different this items -> these items https://codereview.chromium.org/2339933004/diff/200001/storage/browser/blob/b... File storage/browser/blob/blob_memory_controller.cc (right): https://codereview.chromium.org/2339933004/diff/200001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:61: std::vector<base::FilePath> file_paths) { nit: const ref https://codereview.chromium.org/2339933004/diff/200001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:62: base::ThreadRestrictions::AssertIOAllowed(); I think these are automatically checked in File's constructor or in the methods that perform IO operations-- is there a reason we need to explicitly put this assertion here and below? https://codereview.chromium.org/2339933004/diff/200001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:100: std::vector<DataElement*> items, const ref https://codereview.chromium.org/2339933004/diff/200001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:104: base::ThreadRestrictions::AssertIOAllowed(); ditto https://codereview.chromium.org/2339933004/diff/200001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:156: base::SmallMap<std::map<uint64_t, uint64_t>> file_sizes; nit: maybe file_sizes -> file_id_to_sizes for better readability https://codereview.chromium.org/2339933004/diff/200001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:180: template <typename T> nit: T -> TaskList or TASK_LIST for better readability https://codereview.chromium.org/2339933004/diff/200001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:213: MemoryQuotaRequestCallback done_callback) nit: some of params can be probably const ref's https://codereview.chromium.org/2339933004/diff/200001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:225: item->set_state(ShareableBlobDataItem::QUOTA_GRANTED); nit: getting an item with const ref and calling set_state might feel a bit weird... (but we have lots of the same pattern with scoped_refptrs in this file, maybe it's just ok?) https://codereview.chromium.org/2339933004/diff/200001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:334: RunDoneCallback(true, true, std::move(files)); nit: can we comment bool params? true /* success */, true /* delete_object */ By the way are there cases we call RunDoneCallback other than with 'true, true' or 'false, false' combinations? If not we could probably remove delete_object argument https://codereview.chromium.org/2339933004/diff/200001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:388: // Don't call the callbacks until we have a consistant state. consistant -> consistent https://codereview.chromium.org/2339933004/diff/200001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:418: if (total_transportation_bytes > limits_.max_ipc_memory_size) { should this be total_transportation_bytes - preemptive_transported_bytes ? https://codereview.chromium.org/2339933004/diff/200001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:448: return base::WeakPtr<QuotaAllocationTask>(); nit: if memory_items is not empty we might be leaving its items in QUOTA_REQUESTED state? https://codereview.chromium.org/2339933004/diff/200001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:467: if (file_paging_enabled_) this check's not necessary (we're not doing it at the other call site) https://codereview.chromium.org/2339933004/diff/200001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:593: // Process the recent item list and remove items until we have at least the comment isn't finishing https://codereview.chromium.org/2339933004/diff/200001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:631: std::vector<DataElement*> items_for_disk; nit: items_for_disk -> items_for_paging ? https://codereview.chromium.org/2339933004/diff/200001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:683: result.file_reference = std::move(file_reference); do we need this line? https://codereview.chromium.org/2339933004/diff/200001/storage/browser/blob/b... File storage/browser/blob/blob_memory_controller.h (right): https://codereview.chromium.org/2339933004/diff/200001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.h:40: // This class's main responsability is deciding how blob data gets stored. responsability -> responsibility https://codereview.chromium.org/2339933004/diff/200001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.h:99: // Disables the disk. This cancels all pending file creations and paging nit: disk -> file paging for consistency? https://codereview.chromium.org/2339933004/diff/200001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.h:154: void set_limits_for_testing(BlobStorageLimits limits) { limits_ = limits; } nit: const BlobStorageLimits& limits https://codereview.chromium.org/2339933004/diff/200001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.h:235: std::unordered_set<uint64_t> items_saving_to_disk_; nit: saving_to_disk_ or paging_to_file_ ?
The CQ bit was checked by dmurph@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Hello! The two largest changes here: I added a BlobMemoryController;:MemoryAllocation. This is just a typedef for a ScopedClosureRunner. I set this on the ShareableBlobDataItem, and when these are called I: 1. Decrement the memory usage, and 2. Remove the item from the populated_memory_items_ cache (if there). The second change is to change the blob_memory_usage_ to be the TOTAL memory usage, where we include the in_flight_memory amount. I had to do this because of the callback above, where the decrement call had to apply to the correct buffer. So I merged them. We still use the in_flight counter to determine how much disk space we have. Thanks for the comments! https://codereview.chromium.org/2339933004/diff/200001/storage/browser/blob/b... File storage/browser/blob/blob_data_builder.cc (right): https://codereview.chromium.org/2339933004/diff/200001/storage/browser/blob/b... storage/browser/blob/blob_data_builder.cc:28: const char kFutureFileName[] = "_future_name_"; On 2016/10/12 15:26:41, kinuko (slow) wrote: > nit: do we want to use FilePath::CharType kFutureFileName[] = > FILE_PATH_LITERAL("_future_name_") instead and remove some of string conversions > below? Hm... I don't know if that removes any conversions, but maybe it's more readable? https://codereview.chromium.org/2339933004/diff/200001/storage/browser/blob/b... File storage/browser/blob/blob_data_builder.h (right): https://codereview.chromium.org/2339933004/diff/200001/storage/browser/blob/b... storage/browser/blob/blob_data_builder.h:88: // between diffent 'future' files that will be used to store data for this On 2016/10/12 15:26:41, kinuko (slow) wrote: > nit: > diffent -> different > this items -> these items Done. https://codereview.chromium.org/2339933004/diff/200001/storage/browser/blob/b... File storage/browser/blob/blob_data_item.h (right): https://codereview.chromium.org/2339933004/diff/200001/storage/browser/blob/b... storage/browser/blob/blob_data_item.h:46: explicit BlobDataItem(std::unique_ptr<DataElement> item); On 2016/10/11 20:55:23, michaeln wrote: > do these need to be public? what are the new consumers that need to cons these > up? > > the fact that they're private is a good indication to random readers that they > probably won't need to construct these objects themselves (from the point of > view of using the storage lib rather than implementing the storage lib). Done. https://codereview.chromium.org/2339933004/diff/200001/storage/browser/blob/b... File storage/browser/blob/blob_memory_controller.cc (right): https://codereview.chromium.org/2339933004/diff/200001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:47: UMA_HISTOGRAM_ENUMERATION("Storage.Blob.DirectorySuccess.Error", -error, On 2016/10/11 20:55:24, michaeln wrote: > i think you could merge these two stats into one, Blob.CreateDirectroyResult and > 0 corresponds with FILE_OK Done. https://codereview.chromium.org/2339933004/diff/200001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:61: std::vector<base::FilePath> file_paths) { On 2016/10/12 15:26:41, kinuko (slow) wrote: > nit: const ref We do a value here as we std::move our vector (base::Passed()). This clarifies that this method owns the vector & the destruction of the file paths inside. https://codereview.chromium.org/2339933004/diff/200001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:62: base::ThreadRestrictions::AssertIOAllowed(); On 2016/10/12 15:26:41, kinuko (slow) wrote: > I think these are automatically checked in File's constructor or in the methods > that perform IO operations-- is there a reason we need to explicitly put this > assertion here and below? Just to clarify to the reader that we expect to be on a thread that does IO. It's an early-fail. I like it if that's alright. https://codereview.chromium.org/2339933004/diff/200001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:100: std::vector<DataElement*> items, On 2016/10/12 15:26:41, kinuko (slow) wrote: > const ref Same as above, we std::move our vector into this callback, so this clarifies the ownership. https://codereview.chromium.org/2339933004/diff/200001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:103: UMA_HISTOGRAM_MEMORY_KB("Storage.Blob.PageFileSize", total_size_bytes / 1024); On 2016/10/11 20:55:23, michaeln wrote: > this stat looks interesting Acknowledged. https://codereview.chromium.org/2339933004/diff/200001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:104: base::ThreadRestrictions::AssertIOAllowed(); On 2016/10/12 15:26:42, kinuko (slow) wrote: > ditto Same answer as above. Let me know if you feel strongly, I can change it. https://codereview.chromium.org/2339933004/diff/200001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:144: UMA_HISTOGRAM_BOOLEAN("Storage.Blob.PageFileInfoSuccess", success); On 2016/10/11 20:55:23, michaeln wrote: > I'm not sure these functions warrant 8 many uma stats? If we ever see problems > here we can intrument in greater detail, but to start with, I think we should > not have so many. Maybe pair it down to... > > CreateDirectoryResult > PagingDisabled (File::Error) > PageFileSize Done. https://codereview.chromium.org/2339933004/diff/200001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:156: base::SmallMap<std::map<uint64_t, uint64_t>> file_sizes; On 2016/10/12 15:26:41, kinuko (slow) wrote: > nit: maybe file_sizes -> file_id_to_sizes for better readability Done. https://codereview.chromium.org/2339933004/diff/200001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:180: template <typename T> On 2016/10/12 15:26:41, kinuko (slow) wrote: > nit: T -> TaskList or TASK_LIST for better readability Done. https://codereview.chromium.org/2339933004/diff/200001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:213: MemoryQuotaRequestCallback done_callback) On 2016/10/12 15:26:41, kinuko (slow) wrote: > nit: some of params can be probably const ref's I'm doing std::moves here, so I don't want the const ref. https://codereview.chromium.org/2339933004/diff/200001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:225: item->set_state(ShareableBlobDataItem::QUOTA_GRANTED); On 2016/10/12 15:26:41, kinuko (slow) wrote: > nit: getting an item with const ref and calling set_state might feel a bit > weird... (but we have lots of the same pattern with scoped_refptrs in this file, > maybe it's just ok?) I can remove the const. https://codereview.chromium.org/2339933004/diff/200001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:236: On 2016/10/11 20:55:24, michaeln wrote: > can these be private? Done. https://codereview.chromium.org/2339933004/diff/200001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:334: RunDoneCallback(true, true, std::move(files)); On 2016/10/12 15:26:41, kinuko (slow) wrote: > nit: can we comment bool params? > > true /* success */, true /* delete_object */ > > By the way are there cases we call RunDoneCallback other than with 'true, true' > or 'false, false' combinations? If not we could probably remove delete_object > argument Done. https://codereview.chromium.org/2339933004/diff/200001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:388: // Don't call the callbacks until we have a consistant state. On 2016/10/12 15:26:42, kinuko (slow) wrote: > consistant -> consistent Done. https://codereview.chromium.org/2339933004/diff/200001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:418: if (total_transportation_bytes > limits_.max_ipc_memory_size) { On 2016/10/12 15:26:41, kinuko (slow) wrote: > should this be total_transportation_bytes - preemptive_transported_bytes ? No, we're doing all or nothing here. It's a more complex transport problem if we have a partial-shortcut state. On the renderer side we only populated memory if we're < IPC size. We would hit this if we're waiting on memory tasks, in which case we don't have enough memory to fit the preemptive bytes. Does that make sense? We can look at this for future iteration improvement, but for now we don't include shortcut memory if our size is > IPC. https://codereview.chromium.org/2339933004/diff/200001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:435: for (const scoped_refptr<ShareableBlobDataItem>& item : On 2016/10/11 20:55:23, michaeln wrote: > could use const auto& here, might be rid of a line wrap Done. https://codereview.chromium.org/2339933004/diff/200001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:448: return base::WeakPtr<QuotaAllocationTask>(); On 2016/10/12 15:26:41, kinuko (slow) wrote: > nit: if memory_items is not empty we might be leaving its items in > QUOTA_REQUESTED state? We can't, as this would only hit if we don't have any items. We dcheck on length > 0 (we aren't supposed to have 0 length items anywhere in the pipeline, and they're filtered out on the renderer). I can dcheck if you like. https://codereview.chromium.org/2339933004/diff/200001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:461: // If we're past our blob memory limit, then schedule our paging. On 2016/10/11 20:55:24, michaeln wrote: > this comment seems out of place, looks like it belongs inside of > MaybeScheduleEvictionUntilSystemHealthy? Done. https://codereview.chromium.org/2339933004/diff/200001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:463: for (const scoped_refptr<ShareableBlobDataItem>& item : On 2016/10/11 20:55:23, michaeln wrote: > ditto const auto& Done. https://codereview.chromium.org/2339933004/diff/200001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:467: if (file_paging_enabled_) On 2016/10/12 15:26:41, kinuko (slow) wrote: > this check's not necessary (we're not doing it at the other call site) Done. https://codereview.chromium.org/2339933004/diff/200001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:539: recent_item_cache_bytes_ += static_cast<size_t>(item->item()->length()); On 2016/10/11 20:55:24, michaeln wrote: > nit: i think the name of the "recent_item" data members might be a little more > informative? Really the collection is *all* POPULATED_WITH_QUOTA items (not in > the act of being paged out), not just recent ones, is that right? > > maybe populated_memory_items_? Done. https://codereview.chromium.org/2339933004/diff/200001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:575: void BlobMemoryController::MaybeGrantPendingQuotaRequests() { On 2016/10/11 20:55:24, michaeln wrote: > since it only deals with memory requests, MaybeGrantPendingMemoryRequests? Done. https://codereview.chromium.org/2339933004/diff/200001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:593: // Process the recent item list and remove items until we have at least On 2016/10/12 15:26:41, kinuko (slow) wrote: > the comment isn't finishing Done. https://codereview.chromium.org/2339933004/diff/200001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:631: std::vector<DataElement*> items_for_disk; On 2016/10/12 15:26:41, kinuko (slow) wrote: > nit: items_for_disk -> items_for_paging ? Done. https://codereview.chromium.org/2339933004/diff/200001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:683: result.file_reference = std::move(file_reference); On 2016/10/12 15:26:42, kinuko (slow) wrote: > do we need this line? Ah, yeah I guess not. https://codereview.chromium.org/2339933004/diff/200001/storage/browser/blob/b... File storage/browser/blob/blob_memory_controller.h (right): https://codereview.chromium.org/2339933004/diff/200001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.h:40: // This class's main responsability is deciding how blob data gets stored. On 2016/10/12 15:26:42, kinuko (slow) wrote: > responsability -> responsibility Done. https://codereview.chromium.org/2339933004/diff/200001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.h:99: // Disables the disk. This cancels all pending file creations and paging On 2016/10/12 15:26:42, kinuko (slow) wrote: > nit: disk -> file paging > for consistency? Done. https://codereview.chromium.org/2339933004/diff/200001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.h:139: void MaybeFreeMemoryQuotaForItems( On 2016/10/11 20:55:24, michaeln wrote: > What do think about making the "free" call a function of the > ShareableBlobDataItem? That might eliminate the maybeness and be less error > prone. It's nice the file quota is returned automatically when the file is no > longer needed. I think would be nice of mem quota was similarly reclaimed > automatically without the caller having to do something extra. Done. https://codereview.chromium.org/2339933004/diff/200001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.h:142: void ForceFreeMemoryQuotaForItem( On 2016/10/11 20:55:24, michaeln wrote: > what is the usecase for this method? i don't see it being used When we have created (and asked quota for) a 'future' bytes item which we'll use to copy data from a referenced blob. When we go to do the copy, it could have been paged to disk, which results in a file item that we no longer need to copy from. So we need to free that memory and just do a file item reference. W/ the addition of the allocation callback in the shareabledataitem, this is unneeded. https://codereview.chromium.org/2339933004/diff/200001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.h:154: void set_limits_for_testing(BlobStorageLimits limits) { limits_ = limits; } On 2016/10/12 15:26:42, kinuko (slow) wrote: > nit: const BlobStorageLimits& limits Done. https://codereview.chromium.org/2339933004/diff/200001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.h:235: std::unordered_set<uint64_t> items_saving_to_disk_; On 2016/10/12 15:26:42, kinuko (slow) wrote: > nit: saving_to_disk_ or paging_to_file_ ? Done. https://codereview.chromium.org/2339933004/diff/200001/storage/browser/blob/i... File storage/browser/blob/internal_blob_data.cc (right): https://codereview.chromium.org/2339933004/diff/200001/storage/browser/blob/i... storage/browser/blob/internal_blob_data.cc:61: data_item->referencing_blobs_mutable()->erase(blob_uuid); On 2016/10/11 20:55:24, michaeln wrote: > Would it be less error prone to "maybe free" each item when its collection of > referencing blobs becomes empty? Yeah. Done. https://codereview.chromium.org/2339933004/diff/200001/storage/browser/blob/s... File storage/browser/blob/shareable_blob_data_item.h (right): https://codereview.chromium.org/2339933004/diff/200001/storage/browser/blob/s... storage/browser/blob/shareable_blob_data_item.h:26: // We need to be RefCountedThreadSafe as references of this item are passed to a On 2016/10/11 20:55:24, michaeln wrote: > comment is stale Done.
The CQ bit was unchecked by commit-bot@chromium.org
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/...)
The CQ bit was checked by dmurph@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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/...)
mostly naming comments but also the opaqueness of the scopedclosurerunner might hurt more than it helps also i'd like to look thru the tests again (but i don't expect to find anything there). https://codereview.chromium.org/2339933004/diff/200001/storage/browser/blob/b... File storage/browser/blob/blob_memory_controller.cc (right): https://codereview.chromium.org/2339933004/diff/200001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:448: return base::WeakPtr<QuotaAllocationTask>(); On 2016/10/13 00:39:30, dmurph wrote: > On 2016/10/12 15:26:41, kinuko (slow) wrote: > > nit: if memory_items is not empty we might be leaving its items in > > QUOTA_REQUESTED state? > > We can't, as this would only hit if we don't have any items. We dcheck on length > > 0 (we aren't supposed to have 0 length items anywhere in the pipeline, and > they're filtered out on the renderer). > > I can dcheck if you like. i had noticed this too and it made me look closer, i didn't say anything because it was correct, but since kinuko paused here too, maybe we could restructure it for clarity? I think if you put an items.empty() check and corresponding early return prior to the loop, that would do it for correctness. After the bottom of the loop you could have DCHECK_GT(0, total_bytes_needed) for greater clarity. wdyt? https://codereview.chromium.org/2339933004/diff/240001/storage/browser/blob/b... File storage/browser/blob/blob_memory_controller.cc (right): https://codereview.chromium.org/2339933004/diff/240001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:594: pending_evictions_++; i think this value is either 0 or 1 if that's right should this be something more bool'ish is_eviction_in_progress_ is_evicting_ https://codereview.chromium.org/2339933004/diff/240001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:647: shareable_item->memory_quota_allocation()->RunAndReset(); i wish the ScopedClosureRunner were operator bool testable? https://codereview.chromium.org/2339933004/diff/240001/storage/browser/blob/b... File storage/browser/blob/blob_memory_controller.h (right): https://codereview.chromium.org/2339933004/diff/240001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.h:46: // * Freeing quota (MaybeFreeQuotaForItems, ForceFreeMemoryQuotaForItem) stale comments https://codereview.chromium.org/2339933004/diff/240001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.h:78: using MemoryAllocation = base::ScopedClosureRunner; This scoped closure runner functions properly, but i think casting this in terms of a thing (memallocation) that can be deleted would help with grok'ability. The closurerunner isn't if(testable) which leaves me queasy as to what it is and if i (as a consumer of this class) even have one? And i have no hint, based on the naming, what method the runner might be tied to? wdyt of an arrangement like this, that makes it more obvious how the puzzle pieces fit together? struct MemoryAllocation { MemoryAllocation(weakptr, int, int) {} ~MemoryAllocation() { if (bmc) bmc->RevokeMemoryAllocation(item_id, length); } weakptr bmc; int item_id; size_t length }; and use ownptrs to express ownership like we conventionally do through out the project ShareableItem { std::unique_ptr<MemoryAllocation> memory_allocation_; } i know it is a few extra lines of code in the .h file, but it also serves as documentation and makes memory_allocation_ if(testable), debuggable, and grok'able so i think its a good trade (the runtime costs are lighter weight not that it matters much). https://codereview.chromium.org/2339933004/diff/240001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.h:187: void DecrementMemoryQuotaAndRemoveFromRecents(uint64_t item_id, i'd vote to call these two methods something more symmetrical (aka turnOn() and turnOff()). And to something easier to remember by virtual of fewer and more conventional terms. (recents is not a term seen elswehere in this class). The "object" being manipulated is a MemoryAllocation, the "actions" are Grant and Revoke(?) Might be nice to put the methods in order of execution too... grant first and revoke. GrantMemoryAllocations() RevokeMemoryAllocation() FreeMemoryAllocation() Release() Undo() Ungrant() A rule of thumb, when possible dumb it down to a singular action on an object, "VerbNoun". Since the grant function operates on a collection, 's'. https://codereview.chromium.org/2339933004/diff/240001/storage/browser/blob/s... File storage/browser/blob/shareable_blob_data_item.h (right): https://codereview.chromium.org/2339933004/diff/240001/storage/browser/blob/s... storage/browser/blob/shareable_blob_data_item.h:104: BlobMemoryController::MemoryAllocation memory_quota_allocation_; very opaque and mysterious data member?
The CQ bit was checked by dmurph@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by dmurph@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
Thanks! Updated, and it should build fine on windows now. https://codereview.chromium.org/2339933004/diff/200001/storage/browser/blob/b... File storage/browser/blob/blob_memory_controller.cc (right): https://codereview.chromium.org/2339933004/diff/200001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:448: return base::WeakPtr<QuotaAllocationTask>(); On 2016/10/14 01:53:26, michaeln wrote: > On 2016/10/13 00:39:30, dmurph wrote: > > On 2016/10/12 15:26:41, kinuko (slow) wrote: > > > nit: if memory_items is not empty we might be leaving its items in > > > QUOTA_REQUESTED state? > > > > We can't, as this would only hit if we don't have any items. We dcheck on > length > > > 0 (we aren't supposed to have 0 length items anywhere in the pipeline, and > > they're filtered out on the renderer). > > > > I can dcheck if you like. > > i had noticed this too and it made me look closer, i didn't say anything because > it was correct, but since kinuko paused here too, maybe we could restructure it > for clarity? > > I think if you put an items.empty() check and corresponding early return prior > to the loop, that would do it for correctness. After the bottom of the loop you > could have DCHECK_GT(0, total_bytes_needed) for greater clarity. wdyt? Done. https://codereview.chromium.org/2339933004/diff/240001/storage/browser/blob/b... File storage/browser/blob/blob_memory_controller.cc (right): https://codereview.chromium.org/2339933004/diff/240001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:594: pending_evictions_++; On 2016/10/14 01:53:26, michaeln wrote: > i think this value is either 0 or 1 > if that's right should this be something more bool'ish > > is_eviction_in_progress_ > is_evicting_ I need this to be a number, as I can be saving multiple blob file groupings to disk, and each of these will call OnEvictionComplete. So I need to count them and decrement them. https://codereview.chromium.org/2339933004/diff/240001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:647: shareable_item->memory_quota_allocation()->RunAndReset(); On 2016/10/14 01:53:26, michaeln wrote: > i wish the ScopedClosureRunner were operator bool testable? Done. https://codereview.chromium.org/2339933004/diff/240001/storage/browser/blob/b... File storage/browser/blob/blob_memory_controller.h (right): https://codereview.chromium.org/2339933004/diff/240001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.h:78: using MemoryAllocation = base::ScopedClosureRunner; On 2016/10/14 01:53:26, michaeln wrote: > This scoped closure runner functions properly, but i think casting this in terms > of a thing (memallocation) that can be deleted would help with grok'ability. The > closurerunner isn't if(testable) which leaves me queasy as to what it is and if > i (as a consumer of this class) even have one? And i have no hint, based on the > naming, what method the runner might be tied to? > > wdyt of an arrangement like this, that makes it more obvious how the puzzle > pieces fit together? > > struct MemoryAllocation { > MemoryAllocation(weakptr, int, int) {} > ~MemoryAllocation() { > if (bmc) > bmc->RevokeMemoryAllocation(item_id, length); > } > weakptr bmc; > int item_id; > size_t length > }; > > and use ownptrs to express ownership like we conventionally do through out the > project > > ShareableItem { > std::unique_ptr<MemoryAllocation> memory_allocation_; > } > > i know it is a few extra lines of code in the .h file, but it also serves as > documentation and makes memory_allocation_ if(testable), debuggable, and > grok'able so i think its a good trade (the runtime costs are lighter weight not > that it matters much). Done. https://codereview.chromium.org/2339933004/diff/240001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.h:187: void DecrementMemoryQuotaAndRemoveFromRecents(uint64_t item_id, On 2016/10/14 01:53:26, michaeln wrote: > i'd vote to call these two methods something more symmetrical (aka turnOn() and > turnOff()). And to something easier to remember by virtual of fewer and more > conventional terms. (recents is not a term seen elswehere in this class). > > The "object" being manipulated is a MemoryAllocation, the "actions" are Grant > and Revoke(?) Might be nice to put the methods in order of execution too... > grant first and revoke. > > GrantMemoryAllocations() > RevokeMemoryAllocation() FreeMemoryAllocation() Release() Undo() Ungrant() > > A rule of thumb, when possible dumb it down to a singular action on an object, > "VerbNoun". Since the grant function operates on a collection, 's'. > Done. https://codereview.chromium.org/2339933004/diff/240001/storage/browser/blob/s... File storage/browser/blob/shareable_blob_data_item.h (right): https://codereview.chromium.org/2339933004/diff/240001/storage/browser/blob/s... storage/browser/blob/shareable_blob_data_item.h:104: BlobMemoryController::MemoryAllocation memory_quota_allocation_; On 2016/10/14 01:53:26, michaeln wrote: > very opaque and mysterious data member? Done.
Thanks! Updated, and it should build fine on windows now. https://codereview.chromium.org/2339933004/diff/240001/storage/browser/blob/b... File storage/browser/blob/blob_memory_controller.cc (right): https://codereview.chromium.org/2339933004/diff/240001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:594: pending_evictions_++; On 2016/10/14 01:53:26, michaeln wrote: > i think this value is either 0 or 1 > if that's right should this be something more bool'ish > > is_eviction_in_progress_ > is_evicting_ I need this to be a number, as I can be saving multiple blob file groupings to disk, and each of these will call OnEvictionComplete. So I need to count them and decrement them. https://codereview.chromium.org/2339933004/diff/240001/storage/browser/blob/b... File storage/browser/blob/blob_memory_controller.h (right): https://codereview.chromium.org/2339933004/diff/240001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.h:78: using MemoryAllocation = base::ScopedClosureRunner; On 2016/10/14 01:53:26, michaeln wrote: > This scoped closure runner functions properly, but i think casting this in terms > of a thing (memallocation) that can be deleted would help with grok'ability. The > closurerunner isn't if(testable) which leaves me queasy as to what it is and if > i (as a consumer of this class) even have one? And i have no hint, based on the > naming, what method the runner might be tied to? > > wdyt of an arrangement like this, that makes it more obvious how the puzzle > pieces fit together? > > struct MemoryAllocation { > MemoryAllocation(weakptr, int, int) {} > ~MemoryAllocation() { > if (bmc) > bmc->RevokeMemoryAllocation(item_id, length); > } > weakptr bmc; > int item_id; > size_t length > }; > > and use ownptrs to express ownership like we conventionally do through out the > project > > ShareableItem { > std::unique_ptr<MemoryAllocation> memory_allocation_; > } > > i know it is a few extra lines of code in the .h file, but it also serves as > documentation and makes memory_allocation_ if(testable), debuggable, and > grok'able so i think its a good trade (the runtime costs are lighter weight not > that it matters much). Done. https://codereview.chromium.org/2339933004/diff/240001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.h:187: void DecrementMemoryQuotaAndRemoveFromRecents(uint64_t item_id, On 2016/10/14 01:53:26, michaeln wrote: > i'd vote to call these two methods something more symmetrical (aka turnOn() and > turnOff()). And to something easier to remember by virtual of fewer and more > conventional terms. (recents is not a term seen elswehere in this class). > > The "object" being manipulated is a MemoryAllocation, the "actions" are Grant > and Revoke(?) Might be nice to put the methods in order of execution too... > grant first and revoke. > > GrantMemoryAllocations() > RevokeMemoryAllocation() FreeMemoryAllocation() Release() Undo() Ungrant() > > A rule of thumb, when possible dumb it down to a singular action on an object, > "VerbNoun". Since the grant function operates on a collection, 's'. > Done. https://codereview.chromium.org/2339933004/diff/240001/storage/browser/blob/s... File storage/browser/blob/shareable_blob_data_item.h (right): https://codereview.chromium.org/2339933004/diff/240001/storage/browser/blob/s... storage/browser/blob/shareable_blob_data_item.h:104: BlobMemoryController::MemoryAllocation memory_quota_allocation_; On 2016/10/14 01:53:26, michaeln wrote: > very opaque and mysterious data member? Done.
The CQ bit was checked by dmurph@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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/...)
lgtm, i see some nits but otherwise looks nice! (woohoo:) i'm not sure if kinuko or anyone else in the reviewers list has any comments https://codereview.chromium.org/2339933004/diff/240001/storage/browser/blob/b... File storage/browser/blob/blob_memory_controller.h (right): https://codereview.chromium.org/2339933004/diff/240001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.h:78: using MemoryAllocation = base::ScopedClosureRunner; On 2016/10/14 23:31:48, dmurph wrote: > On 2016/10/14 01:53:26, michaeln wrote: > > This scoped closure runner functions properly, but i think casting this in > terms > > of a thing (memallocation) that can be deleted would help with grok'ability. > The > > closurerunner isn't if(testable) which leaves me queasy as to what it is and > if > > i (as a consumer of this class) even have one? And i have no hint, based on > the > > naming, what method the runner might be tied to? > > > > wdyt of an arrangement like this, that makes it more obvious how the puzzle > > pieces fit together? > > > > struct MemoryAllocation { > > MemoryAllocation(weakptr, int, int) {} > > ~MemoryAllocation() { > > if (bmc) > > bmc->RevokeMemoryAllocation(item_id, length); > > } > > weakptr bmc; > > int item_id; > > size_t length > > }; > > > > and use ownptrs to express ownership like we conventionally do through out the > > project > > > > ShareableItem { > > std::unique_ptr<MemoryAllocation> memory_allocation_; > > } > > > > i know it is a few extra lines of code in the .h file, but it also serves as > > documentation and makes memory_allocation_ if(testable), debuggable, and > > grok'able so i think its a good trade (the runtime costs are lighter weight > not > > that it matters much). > > Done. thnx! https://codereview.chromium.org/2339933004/diff/300001/content/browser/blob_s... File content/browser/blob_storage/blob_memory_controller_unittest.cc (right): https://codereview.chromium.org/2339933004/diff/300001/content/browser/blob_s... content/browser/blob_storage/blob_memory_controller_unittest.cc:195: EXPECT_EQ(ItemState::QUOTA_GRANTED, items[2]->state()); maybe also expect the memory_allocation_? https://codereview.chromium.org/2339933004/diff/300001/content/browser/blob_s... content/browser/blob_storage/blob_memory_controller_unittest.cc:210: controller.ReserveMemoryQuota(items, GetMemoryRequestCallback()); also check the return values for the expected nullness https://codereview.chromium.org/2339933004/diff/300001/content/browser/blob_s... content/browser/blob_storage/blob_memory_controller_unittest.cc:251: // (no callback). test for non-null task return value as the indicator of the asyncness https://codereview.chromium.org/2339933004/diff/300001/content/browser/blob_s... content/browser/blob_storage/blob_memory_controller_unittest.cc:275: EXPECT_EQ(kTestBlobStorageMaxBlobMemorySize, controller.disk_usage()); also test for a now-null task as the indicator of doneness and check memory_quota_result_ https://codereview.chromium.org/2339933004/diff/300001/content/browser/blob_s... content/browser/blob_storage/blob_memory_controller_unittest.cc:452: const size_t kSize2 = kTestBlobStorageMaxFileSizeBytes; ah... this test depends on (2 x maxfilesize) <= maxmemorysize, maybe an ASSERT about that? https://codereview.chromium.org/2339933004/diff/300001/storage/browser/blob/b... File storage/browser/blob/blob_data_item.h (right): https://codereview.chromium.org/2339933004/diff/300001/storage/browser/blob/b... storage/browser/blob/blob_data_item.h:67: const scoped_refptr<DataHandle>& data_handle() const { return data_handle_; } i don't see where this is used? https://codereview.chromium.org/2339933004/diff/300001/storage/browser/blob/b... File storage/browser/blob/blob_memory_controller.cc (right): https://codereview.chromium.org/2339933004/diff/300001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:590: while (pending_memory_quota_total_size_ + blob_memory_used_ > oh, duh, its a loop :) https://codereview.chromium.org/2339933004/diff/300001/storage/browser/blob/b... File storage/browser/blob/blob_memory_controller.h (right): https://codereview.chromium.org/2339933004/diff/300001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.h:111: virtual ~BlobMemoryController(); does this need to be virtual? https://codereview.chromium.org/2339933004/diff/300001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.h:132: // call |done_callback|. This can happen synchronously. might mention passing ownership of a MemoryAllocation to the ShareableItem too https://codereview.chromium.org/2339933004/diff/300001/storage/browser/blob/s... File storage/browser/blob/shareable_blob_data_item.h (right): https://codereview.chromium.org/2339933004/diff/300001/storage/browser/blob/s... storage/browser/blob/shareable_blob_data_item.h:55: void set_item(scoped_refptr<BlobDataItem> item); style nit: maybe remove the blank lines between the item getters and setters https://codereview.chromium.org/2339933004/diff/300001/storage/browser/blob/s... storage/browser/blob/shareable_blob_data_item.h:72: void set_state(State state) { state_ = state; } style nit: ditto blank lines https://codereview.chromium.org/2339933004/diff/300001/storage/browser/blob/s... storage/browser/blob/shareable_blob_data_item.h:95: bool has_memory_allocation() { return static_cast<bool>(memory_allocation_); } style nit: might not need this inline method, the DCHECK in he controller is the only callsite and it could test the member directly https://codereview.chromium.org/2339933004/diff/300001/storage/common/blob_st... File storage/common/blob_storage/blob_storage_constants.h (right): https://codereview.chromium.org/2339933004/diff/300001/storage/common/blob_st... storage/common/blob_storage/blob_storage_constants.h:43: uint64_t max_blob_disk_space = 5ull * 1024 * 1024 * 1024; the size of the storage device probably needs to be considered for this one?
The CQ bit was checked by dmurph@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://chromiumcodereview.appspot.com/2339933004/diff/300001/content/browser... File content/browser/blob_storage/blob_memory_controller_unittest.cc (right): https://chromiumcodereview.appspot.com/2339933004/diff/300001/content/browser... content/browser/blob_storage/blob_memory_controller_unittest.cc:195: EXPECT_EQ(ItemState::QUOTA_GRANTED, items[2]->state()); On 2016/10/18 00:56:24, michaeln wrote: > maybe also expect the memory_allocation_? Done. https://chromiumcodereview.appspot.com/2339933004/diff/300001/content/browser... content/browser/blob_storage/blob_memory_controller_unittest.cc:210: controller.ReserveMemoryQuota(items, GetMemoryRequestCallback()); On 2016/10/18 00:56:24, michaeln wrote: > also check the return values for the expected nullness Done. https://chromiumcodereview.appspot.com/2339933004/diff/300001/content/browser... content/browser/blob_storage/blob_memory_controller_unittest.cc:251: // (no callback). On 2016/10/18 00:56:24, michaeln wrote: > test for non-null task return value as the indicator of the asyncness Done. https://chromiumcodereview.appspot.com/2339933004/diff/300001/content/browser... content/browser/blob_storage/blob_memory_controller_unittest.cc:275: EXPECT_EQ(kTestBlobStorageMaxBlobMemorySize, controller.disk_usage()); On 2016/10/18 00:56:24, michaeln wrote: > also test for a now-null task as the indicator of doneness and check > memory_quota_result_ Done. https://chromiumcodereview.appspot.com/2339933004/diff/300001/content/browser... content/browser/blob_storage/blob_memory_controller_unittest.cc:452: const size_t kSize2 = kTestBlobStorageMaxFileSizeBytes; On 2016/10/18 00:56:24, michaeln wrote: > ah... this test depends on (2 x maxfilesize) <= maxmemorysize, maybe an ASSERT > about that? Done. https://chromiumcodereview.appspot.com/2339933004/diff/300001/storage/browser... File storage/browser/blob/blob_data_item.h (right): https://chromiumcodereview.appspot.com/2339933004/diff/300001/storage/browser... storage/browser/blob/blob_data_item.h:67: const scoped_refptr<DataHandle>& data_handle() const { return data_handle_; } On 2016/10/18 00:56:24, michaeln wrote: > i don't see where this is used? Done. https://chromiumcodereview.appspot.com/2339933004/diff/300001/storage/browser... File storage/browser/blob/blob_memory_controller.h (right): https://chromiumcodereview.appspot.com/2339933004/diff/300001/storage/browser... storage/browser/blob/blob_memory_controller.h:111: virtual ~BlobMemoryController(); On 2016/10/18 00:56:24, michaeln wrote: > does this need to be virtual? Done. https://chromiumcodereview.appspot.com/2339933004/diff/300001/storage/browser... storage/browser/blob/blob_memory_controller.h:132: // call |done_callback|. This can happen synchronously. On 2016/10/18 00:56:24, michaeln wrote: > might mention passing ownership of a MemoryAllocation to the ShareableItem too Done. https://chromiumcodereview.appspot.com/2339933004/diff/300001/storage/browser... File storage/browser/blob/shareable_blob_data_item.h (right): https://chromiumcodereview.appspot.com/2339933004/diff/300001/storage/browser... storage/browser/blob/shareable_blob_data_item.h:55: void set_item(scoped_refptr<BlobDataItem> item); On 2016/10/18 00:56:24, michaeln wrote: > style nit: maybe remove the blank lines between the item getters and setters Done. https://chromiumcodereview.appspot.com/2339933004/diff/300001/storage/browser... storage/browser/blob/shareable_blob_data_item.h:72: void set_state(State state) { state_ = state; } On 2016/10/18 00:56:24, michaeln wrote: > style nit: ditto blank lines Done. https://chromiumcodereview.appspot.com/2339933004/diff/300001/storage/browser... storage/browser/blob/shareable_blob_data_item.h:95: bool has_memory_allocation() { return static_cast<bool>(memory_allocation_); } On 2016/10/18 00:56:24, michaeln wrote: > style nit: might not need this inline method, the DCHECK in he controller is the > only callsite and it could test the member directly Done. https://chromiumcodereview.appspot.com/2339933004/diff/300001/storage/common/... File storage/common/blob_storage/blob_storage_constants.h (right): https://chromiumcodereview.appspot.com/2339933004/diff/300001/storage/common/... storage/common/blob_storage/blob_storage_constants.h:43: uint64_t max_blob_disk_space = 5ull * 1024 * 1024 * 1024; On 2016/10/18 00:56:25, michaeln wrote: > the size of the storage device probably needs to be considered for this one? I added a TODO.
The CQ bit was unchecked by commit-bot@chromium.org
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/...)
The CQ bit was checked by dmurph@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
dmurph@chromium.org changed reviewers: + mpearson@google.com
+mpearson for histogram review. I have duplicate reasons in there (explaining when we create a file). Let me know which version is better or if I can be more descriptive or succinct. Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
non-owner LGTM with nits. https://codereview.chromium.org/2339933004/diff/340001/content/browser/blob_s... File content/browser/blob_storage/blob_memory_controller_unittest.cc (right): https://codereview.chromium.org/2339933004/diff/340001/content/browser/blob_s... content/browser/blob_storage/blob_memory_controller_unittest.cc:155: // Not too large, as disk isn't enabled. Does the "as disk isn't enabled" fragment belong to the next comment and test case? https://codereview.chromium.org/2339933004/diff/340001/content/browser/blob_s... content/browser/blob_storage/blob_memory_controller_unittest.cc:169: EXPECT_EQ(Strategy::FILE, controller.DetermineStrategy( Is this a fencepost test? If so, should it be preceded by an EXPECT for Strategy::SHARED_MEMORY? https://codereview.chromium.org/2339933004/diff/340001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2339933004/diff/340001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:60267: + Recorded when we create the blob storage directory for the blob storage double space after "for"?
mpearson@chromium.org changed reviewers: + mpearson@chromium.org
>>> +mpearson for histogram review. I have duplicate reasons in there (explaining when we create a file). Let me know which version is better or if I can be more descriptive or succinct. >>> I don't see duplicate reasons. I think in general your descriptions are good and at the appropriate scope (no need to be more descriptive or more succinct). --mark https://chromiumcodereview.appspot.com/2339933004/diff/340001/tools/metrics/h... File tools/metrics/histograms/histograms.xml (right): https://chromiumcodereview.appspot.com/2339933004/diff/340001/tools/metrics/h... tools/metrics/histograms/histograms.xml:60268: + system. This lazily happens when we need to save blob data to files. This What does "lazily" add in this context? Does it simply mean "... if the storage directory does not exist"? If so, please say that instead. If lazily has something to do with asynchronous processing (we do it on a delay, or when the CPU is idle, or something), please explain. https://chromiumcodereview.appspot.com/2339933004/diff/340001/tools/metrics/h... tools/metrics/histograms/histograms.xml:60268: + system. This lazily happens when we need to save blob data to files. This The "this" as the beginning of the third sentence refers to saving blob data, right? (as opposed to the "this" as the beginning of the second sentence which refers to create a directory) https://chromiumcodereview.appspot.com/2339933004/diff/340001/tools/metrics/h... tools/metrics/histograms/histograms.xml:60316: + Size of a page file created for old blob data by the blob storage system. Does "old" add anything here? I understand the whole description if I skip over this word but with it I'm not sure what's happening. ditto in the below description https://chromiumcodereview.appspot.com/2339933004/diff/340001/tools/metrics/h... tools/metrics/histograms/histograms.xml:60327: + data to disk - when the system is close to it's in-memory limit - or nit: it's -> its
sorry for slow review, here're some more comments. https://codereview.chromium.org/2339933004/diff/340001/storage/browser/blob/b... File storage/browser/blob/blob_memory_controller.cc (right): https://codereview.chromium.org/2339933004/diff/340001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:49: return error; nit: if we initialize error = FILE_OK on line 44 I think we can simply return error (per the function comment) https://codereview.chromium.org/2339933004/diff/340001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:203: : allocation_size_(allocation_size) {} I don't think having this base task class is making the code any simpler (rather it might be adding more lines)? If I'm not mistaken we don't really need this common base class, am I right? https://codereview.chromium.org/2339933004/diff/340001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:355: // This will call call our callback and delete the object correctly. nit: call call -> call https://codereview.chromium.org/2339933004/diff/340001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:556: memory_task->RunDoneCallback(true); nit: Would it be possible to always increment blob_memory_used_ in GrantMemoryAllocations instead? We do decrement blob_memory_used_ in RevokeMemoryAlloc so it'd feel more consistent... https://codereview.chromium.org/2339933004/diff/340001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:570: DCHECK(item); nit: this DCHECK seems redundant, if it's null we'll crash at line 571 which would get necessary dump https://codereview.chromium.org/2339933004/diff/340001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:634: total_items_size)); Could this code perform a tight loop to post so many CreateFileAndWriteItems tasks if large memory is requested at once? (I'm wondering if we want to move this PostTaskAndReplyWithResult out of the loop to perform a batch IO task) https://codereview.chromium.org/2339933004/diff/340001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:644: if (!file_paging_enabled_) nit: checking this and using dedicated weak factory (file_task_factory_) feels a bit redundant, and we don't seem to explicitly invalidate the other factory (quota_bookkeeping_factory_) either. It makes me feel we can just have one regular weak factory for all those tasks and remove file_task_factory_.Invalidate line in DisablePaging? https://codereview.chromium.org/2339933004/diff/340001/storage/browser/blob/b... File storage/browser/blob/blob_memory_controller.h (right): https://codereview.chromium.org/2339933004/diff/340001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.h:190: // we swap the bytes items for file items, and and update our bookkeeping. nit: and and -> and
https://codereview.chromium.org/2339933004/diff/200001/storage/browser/blob/b... File storage/browser/blob/blob_data_builder.cc (right): https://codereview.chromium.org/2339933004/diff/200001/storage/browser/blob/b... storage/browser/blob/blob_data_builder.cc:28: const char kFutureFileName[] = "_future_name_"; On 2016/10/13 00:39:29, dmurph wrote: > On 2016/10/12 15:26:41, kinuko (slow) wrote: > > nit: do we want to use FilePath::CharType kFutureFileName[] = > > FILE_PATH_LITERAL("_future_name_") instead and remove some of string > conversions > > below? > > Hm... I don't know if that removes any conversions, but maybe it's more > readable? It's definitely a minor issue, but it removes FromUTF8Unsafe() and MaybeAsASCII() conversions on Windows https://codereview.chromium.org/2339933004/diff/220001/storage/browser/blob/b... File storage/browser/blob/blob_data_builder.cc (right): https://codereview.chromium.org/2339933004/diff/220001/storage/browser/blob/b... storage/browser/blob/blob_data_builder.cc:46: return base::StartsWith(path, prefix, base::CompareCase::SENSITIVE); nit: if you used FilePath::CharType for kFutureFIleName this method must have also used StringType const FilePath::StringType prefix(kFutureFileName); return base::StartsWith(element.path().value(), prefix, ...);
The CQ bit was checked by dmurph@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks! I updated based on comments, and also removed the in_flight_memory_size field in blob limits as this should always be the same as the min page file size. So that's working now. I added one more test too just to make sure we can do a full evict. Please take a look :) Also, any guidance on the non-clang windows builds would be appreciated, I have no idea what's happening there. https://chromiumcodereview.appspot.com/2339933004/diff/200001/storage/browser... File storage/browser/blob/blob_data_builder.cc (right): https://chromiumcodereview.appspot.com/2339933004/diff/200001/storage/browser... storage/browser/blob/blob_data_builder.cc:28: const char kFutureFileName[] = "_future_name_"; On 2016/10/19 21:34:09, kinuko (slowish) wrote: > On 2016/10/13 00:39:29, dmurph wrote: > > On 2016/10/12 15:26:41, kinuko (slow) wrote: > > > nit: do we want to use FilePath::CharType kFutureFileName[] = > > > FILE_PATH_LITERAL("_future_name_") instead and remove some of string > > conversions > > > below? > > > > Hm... I don't know if that removes any conversions, but maybe it's more > > readable? > > It's definitely a minor issue, but it removes FromUTF8Unsafe() and > MaybeAsASCII() conversions on Windows Cool, I think I got it working. https://chromiumcodereview.appspot.com/2339933004/diff/220001/storage/browser... File storage/browser/blob/blob_data_builder.cc (right): https://chromiumcodereview.appspot.com/2339933004/diff/220001/storage/browser... storage/browser/blob/blob_data_builder.cc:46: return base::StartsWith(path, prefix, base::CompareCase::SENSITIVE); On 2016/10/19 21:34:10, kinuko (slowish) wrote: > nit: if you used FilePath::CharType for kFutureFIleName this method must have > also used StringType > > const FilePath::StringType prefix(kFutureFileName); > return base::StartsWith(element.path().value(), prefix, ...); Done. https://chromiumcodereview.appspot.com/2339933004/diff/340001/content/browser... File content/browser/blob_storage/blob_memory_controller_unittest.cc (right): https://chromiumcodereview.appspot.com/2339933004/diff/340001/content/browser... content/browser/blob_storage/blob_memory_controller_unittest.cc:155: // Not too large, as disk isn't enabled. On 2016/10/18 23:20:13, pwnall wrote: > Does the "as disk isn't enabled" fragment belong to the next comment and test > case? Done. https://chromiumcodereview.appspot.com/2339933004/diff/340001/content/browser... content/browser/blob_storage/blob_memory_controller_unittest.cc:169: EXPECT_EQ(Strategy::FILE, controller.DetermineStrategy( On 2016/10/18 23:20:13, pwnall wrote: > Is this a fencepost test? If so, should it be preceded by an EXPECT for > Strategy::SHARED_MEMORY? Done. https://chromiumcodereview.appspot.com/2339933004/diff/340001/storage/browser... File storage/browser/blob/blob_memory_controller.cc (right): https://chromiumcodereview.appspot.com/2339933004/diff/340001/storage/browser... storage/browser/blob/blob_memory_controller.cc:49: return error; On 2016/10/19 21:08:55, kinuko (slowish) wrote: > nit: if we initialize error = FILE_OK on line 44 I think we can simply return > error (per the function comment) Done. https://chromiumcodereview.appspot.com/2339933004/diff/340001/storage/browser... storage/browser/blob/blob_memory_controller.cc:203: : allocation_size_(allocation_size) {} On 2016/10/19 21:08:56, kinuko (slowish) wrote: > I don't think having this base task class is making the code any simpler (rather > it might be adding more lines)? If I'm not mistaken we don't really need this > common base class, am I right? We don't need it. Removed. https://chromiumcodereview.appspot.com/2339933004/diff/340001/storage/browser... storage/browser/blob/blob_memory_controller.cc:355: // This will call call our callback and delete the object correctly. On 2016/10/19 21:08:56, kinuko (slowish) wrote: > nit: call call -> call Done. https://chromiumcodereview.appspot.com/2339933004/diff/340001/storage/browser... storage/browser/blob/blob_memory_controller.cc:556: memory_task->RunDoneCallback(true); On 2016/10/19 21:08:56, kinuko (slowish) wrote: > nit: Would it be possible to always increment blob_memory_used_ in > GrantMemoryAllocations instead? We do decrement blob_memory_used_ in > RevokeMemoryAlloc so it'd feel more consistent... Definitely! https://chromiumcodereview.appspot.com/2339933004/diff/340001/storage/browser... storage/browser/blob/blob_memory_controller.cc:570: DCHECK(item); On 2016/10/19 21:08:56, kinuko (slowish) wrote: > nit: this DCHECK seems redundant, if it's null we'll crash at line 571 which > would get necessary dump Done. https://chromiumcodereview.appspot.com/2339933004/diff/340001/storage/browser... storage/browser/blob/blob_memory_controller.cc:634: total_items_size)); On 2016/10/19 21:08:55, kinuko (slowish) wrote: > Could this code perform a tight loop to post so many CreateFileAndWriteItems > tasks if large memory is requested at once? (I'm wondering if we want to move > this PostTaskAndReplyWithResult out of the loop to perform a batch IO task) So we're already batching everything to be > limits_.min_page_file_size, which is 5MB ... worst case is a full eviction, so 495 / 5 = 99 tasks scheduled. It's not a bad idea, but it makes the code a little weirder - I have make vectors of vectors for OnEvictionComplete which I'd like to avoid. I'd prefer it as is, , but if you feel strongly here I can definitely do it. https://chromiumcodereview.appspot.com/2339933004/diff/340001/storage/browser... storage/browser/blob/blob_memory_controller.cc:644: if (!file_paging_enabled_) On 2016/10/19 21:08:56, kinuko (slowish) wrote: > nit: checking this and using dedicated weak factory (file_task_factory_) feels a > bit redundant, and we don't seem to explicitly invalidate the other factory > (quota_bookkeeping_factory_) either. It makes me feel we can just have one > regular weak factory for all those tasks and remove > file_task_factory_.Invalidate line in DisablePaging? Sounds good, done. https://chromiumcodereview.appspot.com/2339933004/diff/340001/storage/browser... File storage/browser/blob/blob_memory_controller.h (right): https://chromiumcodereview.appspot.com/2339933004/diff/340001/storage/browser... storage/browser/blob/blob_memory_controller.h:190: // we swap the bytes items for file items, and and update our bookkeeping. On 2016/10/19 21:08:56, kinuko (slowish) wrote: > nit: and and -> and Done. https://chromiumcodereview.appspot.com/2339933004/diff/340001/tools/metrics/h... File tools/metrics/histograms/histograms.xml (right): https://chromiumcodereview.appspot.com/2339933004/diff/340001/tools/metrics/h... tools/metrics/histograms/histograms.xml:60267: + Recorded when we create the blob storage directory for the blob storage On 2016/10/18 23:20:13, pwnall wrote: > double space after "for"? Done. https://chromiumcodereview.appspot.com/2339933004/diff/340001/tools/metrics/h... tools/metrics/histograms/histograms.xml:60268: + system. This lazily happens when we need to save blob data to files. This On 2016/10/18 23:30:49, Mark P (sick) wrote: > The "this" as the beginning of the third sentence refers to saving blob data, > right? (as opposed to the "this" as the beginning of the second sentence which > refers to create a directory) Done. https://chromiumcodereview.appspot.com/2339933004/diff/340001/tools/metrics/h... tools/metrics/histograms/histograms.xml:60268: + system. This lazily happens when we need to save blob data to files. This On 2016/10/18 23:30:49, Mark P (sick) wrote: > What does "lazily" add in this context? > Does it simply mean "... if the storage directory does not exist"? If so, > please say that instead. > If lazily has something to do with asynchronous processing (we do it on a delay, > or when the CPU is idle, or something), please explain. Clarified, it happens when we need to store blob data to files (on demand) if it doesn't exist. https://chromiumcodereview.appspot.com/2339933004/diff/340001/tools/metrics/h... tools/metrics/histograms/histograms.xml:60316: + Size of a page file created for old blob data by the blob storage system. On 2016/10/18 23:30:49, Mark P (sick) wrote: > Does "old" add anything here? I understand the whole description if I skip over > this word but with it I'm not sure what's happening. > ditto in the below description Done. https://chromiumcodereview.appspot.com/2339933004/diff/340001/tools/metrics/h... tools/metrics/histograms/histograms.xml:60327: + data to disk - when the system is close to it's in-memory limit - or On 2016/10/18 23:30:49, Mark P (sick) wrote: > nit: it's -> its Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
histograms.xml lgtm
Thanks, I think this lgtm (review on tests is a bit lightweight, hope they are covered by others) https://codereview.chromium.org/2339933004/diff/360001/content/browser/blob_s... File content/browser/blob_storage/blob_memory_controller_unittest.cc (right): https://codereview.chromium.org/2339933004/diff/360001/content/browser/blob_s... content/browser/blob_storage/blob_memory_controller_unittest.cc:42: ASSERT_EQ(true, temp_dir_.CreateUniqueTempDir()); nit: ASSERT_TRUE? (For all similar lines, unless you have a reason to prefer ASSERT_EQ(true, ...)) https://codereview.chromium.org/2339933004/diff/360001/content/browser/blob_s... content/browser/blob_storage/blob_memory_controller_unittest.cc:81: files_created_.swap(info); nit: { } for consistency https://codereview.chromium.org/2339933004/diff/360001/content/browser/blob_s... content/browser/blob_storage/blob_memory_controller_unittest.cc:107: // eventually calls back to our system and decrements our disk usage. nit: line wraps look a bit off https://codereview.chromium.org/2339933004/diff/360001/storage/browser/blob/b... File storage/browser/blob/blob_memory_controller.cc (right): https://codereview.chromium.org/2339933004/diff/360001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:63: return std::make_pair(std::vector<FileCreationInfo>(), dir_create_status); nit: {} for consistency (or remove {} for other single-line body blocks) https://codereview.chromium.org/2339933004/diff/360001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:75: creation_info.error); nit: {} for multi-line body https://codereview.chromium.org/2339933004/diff/360001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:83: creation_info.error); ditto https://codereview.chromium.org/2339933004/diff/360001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:107: return creation_info; nit: {} for consistency (here and below... looks like we have many of these, not sure how strictly we want to make these consistent but in general it'd be still good to do so...) https://codereview.chromium.org/2339933004/diff/360001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:184: : controller(controller), item_id(item_id), length(length) {} nit: can we put an empty line between ctor and dtor for readability https://codereview.chromium.org/2339933004/diff/360001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:192: // The my_list_position_ iterator is stored so that we can remove ourself from nit: this comment might make better sense if it's placed before set_my_list_position() or my_list_position_ member variable. https://codereview.chromium.org/2339933004/diff/360001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:249: // the task list when it is cancelled. ditto. https://codereview.chromium.org/2339933004/diff/360001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:253: FileQuotaAllocationTask( It might be beneficial to note that it posts a task to create a file for paging right away in ctor https://codereview.chromium.org/2339933004/diff/360001/storage/browser/blob/b... File storage/browser/blob/blob_memory_controller.h (right): https://codereview.chromium.org/2339933004/diff/360001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.h:249: base::WeakPtrFactory<BlobMemoryController> ptr_factory_; weak_factory_ to follow the convention?
https://codereview.chromium.org/2339933004/diff/360001/storage/browser/blob/b... File storage/browser/blob/blob_memory_controller.h (right): https://codereview.chromium.org/2339933004/diff/360001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.h:167: class BaseQuotaAllocationTask; fyi: no longer needed
The CQ bit was checked by dmurph@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks! I changed the way I append the file name to maybe fix the windows issue (it looks like it's appending utf16 instead of wchar in AppendAscii, which is weird). https://chromiumcodereview.appspot.com/2339933004/diff/360001/content/browser... File content/browser/blob_storage/blob_memory_controller_unittest.cc (right): https://chromiumcodereview.appspot.com/2339933004/diff/360001/content/browser... content/browser/blob_storage/blob_memory_controller_unittest.cc:42: ASSERT_EQ(true, temp_dir_.CreateUniqueTempDir()); On 2016/10/20 16:15:34, kinuko (slowish) wrote: > nit: ASSERT_TRUE? (For all similar lines, unless you have a reason to prefer > ASSERT_EQ(true, ...)) Done. https://chromiumcodereview.appspot.com/2339933004/diff/360001/content/browser... content/browser/blob_storage/blob_memory_controller_unittest.cc:81: files_created_.swap(info); On 2016/10/20 16:15:34, kinuko (slowish) wrote: > nit: { } for consistency Done. https://chromiumcodereview.appspot.com/2339933004/diff/360001/content/browser... content/browser/blob_storage/blob_memory_controller_unittest.cc:107: // eventually calls back to our system and decrements our disk usage. On 2016/10/20 16:15:34, kinuko (slowish) wrote: > nit: line wraps look a bit off Done. https://chromiumcodereview.appspot.com/2339933004/diff/360001/storage/browser... File storage/browser/blob/blob_memory_controller.cc (right): https://chromiumcodereview.appspot.com/2339933004/diff/360001/storage/browser... storage/browser/blob/blob_memory_controller.cc:63: return std::make_pair(std::vector<FileCreationInfo>(), dir_create_status); On 2016/10/20 16:15:34, kinuko (slowish) wrote: > nit: {} for consistency (or remove {} for other single-line body blocks) Removing {} from single line body blocks, this was my intent. https://chromiumcodereview.appspot.com/2339933004/diff/360001/storage/browser... storage/browser/blob/blob_memory_controller.cc:75: creation_info.error); On 2016/10/20 16:15:34, kinuko (slowish) wrote: > nit: {} for multi-line body Done. https://chromiumcodereview.appspot.com/2339933004/diff/360001/storage/browser... storage/browser/blob/blob_memory_controller.cc:83: creation_info.error); On 2016/10/20 16:15:34, kinuko (slowish) wrote: > ditto Done. https://chromiumcodereview.appspot.com/2339933004/diff/360001/storage/browser... storage/browser/blob/blob_memory_controller.cc:107: return creation_info; On 2016/10/20 16:15:34, kinuko (slowish) wrote: > nit: {} for consistency (here and below... looks like we have many of these, not > sure how strictly we want to make these consistent but in general it'd be still > good to do so...) Done. https://chromiumcodereview.appspot.com/2339933004/diff/360001/storage/browser... storage/browser/blob/blob_memory_controller.cc:184: : controller(controller), item_id(item_id), length(length) {} On 2016/10/20 16:15:34, kinuko (slowish) wrote: > nit: can we put an empty line between ctor and dtor for readability Done. https://chromiumcodereview.appspot.com/2339933004/diff/360001/storage/browser... storage/browser/blob/blob_memory_controller.cc:192: // The my_list_position_ iterator is stored so that we can remove ourself from On 2016/10/20 16:15:34, kinuko (slowish) wrote: > nit: this comment might make better sense if it's placed before > set_my_list_position() or my_list_position_ member variable. Done. https://chromiumcodereview.appspot.com/2339933004/diff/360001/storage/browser... storage/browser/blob/blob_memory_controller.cc:249: // the task list when it is cancelled. On 2016/10/20 16:15:34, kinuko (slowish) wrote: > ditto. Done. https://chromiumcodereview.appspot.com/2339933004/diff/360001/storage/browser... storage/browser/blob/blob_memory_controller.cc:253: FileQuotaAllocationTask( On 2016/10/20 16:15:34, kinuko (slowish) wrote: > It might be beneficial to note that it posts a task to create a file for paging > right away in ctor Done. https://chromiumcodereview.appspot.com/2339933004/diff/360001/storage/browser... File storage/browser/blob/blob_memory_controller.h (right): https://chromiumcodereview.appspot.com/2339933004/diff/360001/storage/browser... storage/browser/blob/blob_memory_controller.h:249: base::WeakPtrFactory<BlobMemoryController> ptr_factory_; On 2016/10/20 16:15:34, kinuko (slowish) wrote: > weak_factory_ to follow the convention? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by dmurph@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
The CQ bit was checked by dmurph@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by dmurph@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by dmurph@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Found the windows bug! It had to do with argument evaluation order, which was a little strange to me. But it's fixed!
The CQ bit was unchecked by dmurph@chromium.org
The CQ bit was checked by dmurph@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from michaeln@chromium.org, pwnall@chromium.org, mpearson@chromium.org, kinuko@chromium.org Link to the patchset: https://codereview.chromium.org/2339933004/#ps460001 (title: "Fixed windows bug!")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [BlobStorage] BlobMemoryController & tests This is a split off of https://codereview.chromium.org/2055053003/. This adds the BlobMemoryController & tests, as well as any other small required changes. I avoided doing too much refactoring (like the move to BlobStatus everywhere, and removing BlobStorageRegistry::Entry for InternalBlobData) to just focus on the BlobMemoryController API. BUG=375297 R=michaeln@chromium.org,kinuko@chromium.org,mek@chromium.org ========== to ========== [BlobStorage] BlobMemoryController & tests This is a split off of https://codereview.chromium.org/2055053003/. This adds the BlobMemoryController & tests, as well as any other small required changes. I avoided doing too much refactoring (like the move to BlobStatus everywhere, and removing BlobStorageRegistry::Entry for InternalBlobData) to just focus on the BlobMemoryController API. BUG=375297 R=michaeln@chromium.org,kinuko@chromium.org,mek@chromium.org ==========
Message was sent while issue was closed.
Committed patchset #24 (id:460001)
Message was sent while issue was closed.
Description was changed from ========== [BlobStorage] BlobMemoryController & tests This is a split off of https://codereview.chromium.org/2055053003/. This adds the BlobMemoryController & tests, as well as any other small required changes. I avoided doing too much refactoring (like the move to BlobStatus everywhere, and removing BlobStorageRegistry::Entry for InternalBlobData) to just focus on the BlobMemoryController API. BUG=375297 R=michaeln@chromium.org,kinuko@chromium.org,mek@chromium.org ========== to ========== [BlobStorage] BlobMemoryController & tests This is a split off of https://codereview.chromium.org/2055053003/. This adds the BlobMemoryController & tests, as well as any other small required changes. I avoided doing too much refactoring (like the move to BlobStatus everywhere, and removing BlobStorageRegistry::Entry for InternalBlobData) to just focus on the BlobMemoryController API. BUG=375297 R=michaeln@chromium.org,kinuko@chromium.org,mek@chromium.org Committed: https://crrev.com/0b0e36d9a68b386ab2e8e6c921122bb0f6e7e0e9 Cr-Commit-Position: refs/heads/master@{#426901} ==========
Message was sent while issue was closed.
Patchset 24 (id:??) landed as https://crrev.com/0b0e36d9a68b386ab2e8e6c921122bb0f6e7e0e9 Cr-Commit-Position: refs/heads/master@{#426901} |
