Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2552153002:
[BlobStorage] Enabling disk paging and direct storage. (Closed)
Description[BlobStorage] Enabling disk paging and direct storage.
Adds disk configuration calculations that are slightly specialized for
Android, ChromeOS, and general desktop. The max memory space and disk
spaced are set based on the total RAM and disk of the device.
BUG=375297
Review-Url: https://codereview.chromium.org/2552153002
Cr-Commit-Position: refs/heads/master@{#443139}
Committed: https://chromium.googlesource.com/chromium/src/+/bf104bd8baf5e73b71c82ba60249cb778b8db099
Patch Set 1 #
Total comments: 8
Patch Set 2 : Added test & fix for memory controller paging too much #Patch Set 3 : Added detection for getting close to disk full, and disk usage freezing #
Total comments: 2
Patch Set 4 : Added an early-exit clause so files don't get created unnecessarily #
Total comments: 12
Patch Set 5 : Added min disk availibility and adjustment of storage limits when approach (and leaving) disk limits #Patch Set 6 : forgot some comments #
Total comments: 10
Patch Set 7 : build fix, adding UMA metrics #
Total comments: 8
Patch Set 8 : test fix, and comments #Patch Set 9 : fixed windows build #
Total comments: 12
Patch Set 10 : Marijn's comments #Patch Set 11 : rebase #Patch Set 12 : Disk space getter is now a func ptr #
Total comments: 16
Patch Set 13 : Fixed & added test for disk quota near min_available, and comments #
Total comments: 2
Patch Set 14 : Marijn's comments #
Total comments: 14
Patch Set 15 : Comments from Mark #
Total comments: 13
Patch Set 16 : Mark's comment #Patch Set 17 : added comments to WebContentsImplTest #Messages
Total messages: 100 (62 generated)
dmurph@chromium.org changed reviewers: + jsbell@chromium.org, michaeln@chromium.org, pwnall@chromium.org
Hello Josh, Michael, and Victor: Can you PTAL at this change? It enables disk for the blob storage system. Daniel
dmurph@chromium.org changed reviewers: + kinuko@chromium.org
+kinuko for knowledge, and FYI
dmurph@chromium.org changed reviewers: + mek@google.com
+mek as well for knowledge, FYI, and review
https://codereview.chromium.org/2552153002/diff/1/storage/browser/blob/blob_m... File storage/browser/blob/blob_memory_controller.cc (right): https://codereview.chromium.org/2552153002/diff/1/storage/browser/blob/blob_m... storage/browser/blob/blob_memory_controller.cc:53: // * Disk - 10% hmmm... i'm not sure how to evaluate these numbers?
https://codereview.chromium.org/2552153002/diff/1/storage/browser/blob/blob_m... File storage/browser/blob/blob_memory_controller.cc (right): https://codereview.chromium.org/2552153002/diff/1/storage/browser/blob/blob_m... storage/browser/blob/blob_memory_controller.cc:53: // * Disk - 10% On 2016/12/06 21:58:23, michaeln wrote: > hmmm... i'm not sure how to evaluate these numbers? Josh, Victor, and I chatted for a while about them. What would you like? I can clarify that this is relative to device stats.
https://codereview.chromium.org/2552153002/diff/1/storage/browser/blob/blob_m... File storage/browser/blob/blob_memory_controller.cc (right): https://codereview.chromium.org/2552153002/diff/1/storage/browser/blob/blob_m... storage/browser/blob/blob_memory_controller.cc:53: // * Disk - 10% On 2016/12/06 22:12:43, dmurph wrote: > On 2016/12/06 21:58:23, michaeln wrote: > > hmmm... i'm not sure how to evaluate these numbers? > > Josh, Victor, and I chatted for a while about them. What would you like? I can > clarify that this is relative to device stats. I'm not sure, i haven't thought much about the policy for this resource utilization and presented with hard numbers suddenly makes it real. The amount of freediskspace() comes to mind when considering disk usage. Maybe whatever 'limit' is chosen should be tempered in light of actual availability when we're being asked to alloc something. The 50% cros number concerns me. Storage on my personal chromeos device is fairly filled up. The blob system probably shouldn't be able to consume 100% of what's remaining. Would it do that as coded right now? https://codereview.chromium.org/2552153002/diff/1/storage/browser/blob/blob_m... storage/browser/blob/blob_memory_controller.cc:65: #if !defined(OS_CHROMEOS) && !defined(OS_ANDROID) && defined(ARCH_CPU_64_BITS) is there such a thing as a 64bit system with < 2G of physical memory, would it make sense to constrain this along the lines of std::min(2G, 20%)?
https://codereview.chromium.org/2552153002/diff/1/storage/browser/blob/blob_m... File storage/browser/blob/blob_memory_controller.cc (right): https://codereview.chromium.org/2552153002/diff/1/storage/browser/blob/blob_m... storage/browser/blob/blob_memory_controller.cc:53: // * Disk - 10% On 2016/12/06 23:11:25, michaeln wrote: > On 2016/12/06 22:12:43, dmurph wrote: > > On 2016/12/06 21:58:23, michaeln wrote: > > > hmmm... i'm not sure how to evaluate these numbers? > > > > Josh, Victor, and I chatted for a while about them. What would you like? I can > > clarify that this is relative to device stats. > > I'm not sure, i haven't thought much about the policy for this resource > utilization and presented with hard numbers suddenly makes it real. > > The amount of freediskspace() comes to mind when considering disk usage. Maybe > whatever 'limit' is chosen should be tempered in light of actual availability > when we're being asked to alloc something. > > The 50% cros number concerns me. Storage on my personal chromeos device is > fairly filled up. The blob system probably shouldn't be able to consume 100% of > what's remaining. Would it do that as coded right now? Free disk space changes, so I didn't want to use that. We want to accommodate the use case of people using blob-heavy applications - see https://docs.google.com/document/d/1vc2YtaqX0j6nkPGmjo7P4ObXqAPT99Q1zxFGbYdnz... for some examples. We aren't totally reserving this - this is just the ceiling for blobs. Chromebooks have not much disk space, so we wanted a high percentage here. If we hit a full disk, blobs will fail as normal, and they will go away when you close the tab / blobs are dereferenced. https://codereview.chromium.org/2552153002/diff/1/storage/browser/blob/blob_m... storage/browser/blob/blob_memory_controller.cc:65: #if !defined(OS_CHROMEOS) && !defined(OS_ANDROID) && defined(ARCH_CPU_64_BITS) On 2016/12/06 23:11:25, michaeln wrote: > is there such a thing as a 64bit system with < 2G of physical memory, would it > make sense to constrain this along the lines of std::min(2G, 20%)? The reasoning here is that if it's x64, then it has virtual memory and we're not concerned. We explicitly don't do this on android or chromeos though, which I don't think has that. So I would even do max(2G, 20%)
Just a couple of nits about comments. Haven't actually read the CL yet. https://codereview.chromium.org/2552153002/diff/1/storage/browser/blob/blob_m... File storage/browser/blob/blob_memory_controller.cc (right): https://codereview.chromium.org/2552153002/diff/1/storage/browser/blob/blob_m... storage/browser/blob/blob_memory_controller.cc:46: // * Ram - 20% Nit: Capitalize RAM or just say 'Memory' https://codereview.chromium.org/2552153002/diff/1/storage/browser/blob/blob_m... storage/browser/blob/blob_memory_controller.cc:52: // * Ram - 20%, or 2 gigs if x64. Nit: GB or GiB
Another note: On ChromeOS, we're measuring the disk size of the 'stateful' partition - the one that doesn't have the system stuff on it. 50% is always of the partition that our profile directory is on. This shouldn't kill anything about the system if we are full. And it goes away when tabs are close or worst case when you log out / system restart.
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...
@all, please take a look! I added disk-almost-full detection! Much better experience! No breakage! woot! (still need to mock out disk availability detection to do thorough tests, but the changes aren't very complex. PTAL!)
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_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
i've got some questions about it, but i think this looks like a good approach https://codereview.chromium.org/2552153002/diff/40001/storage/browser/blob/bl... File storage/browser/blob/blob_memory_controller.cc (right): https://codereview.chromium.org/2552153002/diff/40001/storage/browser/blob/bl... storage/browser/blob/blob_memory_controller.cc:618: } else { it'd be nice if this branch wasn't needed, i guess its for testing is that right? could the tests provide a 'file_runner_'? https://codereview.chromium.org/2552153002/diff/60001/storage/browser/blob/bl... File storage/browser/blob/blob_memory_controller.cc (right): https://codereview.chromium.org/2552153002/diff/60001/storage/browser/blob/bl... storage/browser/blob/blob_memory_controller.cc:171: return std::make_pair(std::move(creation_info), kUnknownDiskAvailability); why say 'unkwonwavail' here, we know what it is, why not let it thru https://codereview.chromium.org/2552153002/diff/60001/storage/browser/blob/bl... storage/browser/blob/blob_memory_controller.cc:183: return std::make_pair(std::move(creation_info), disk_availability); since we failed to create the file and won't be writing any data to disk, should pair.second be actual |free_disk_space|? https://codereview.chromium.org/2552153002/diff/60001/storage/browser/blob/bl... storage/browser/blob/blob_memory_controller.cc:206: return std::make_pair(std::move(creation_info), disk_availability); similar question about |free_disk| vs the locally adjusted number, but also would it make sense to delete the borked file prior to return? https://codereview.chromium.org/2552153002/diff/60001/storage/browser/blob/bl... storage/browser/blob/blob_memory_controller.cc:642: void BlobMemoryController::FreezeDiskUsage(uint64_t avail_disk) { Is "Freeze" a misnomer? If i'm understanding the CL properly the max_disk limit is adjusted in light of the actual availability but additional disk space can still be allocated, either when avail is not below the min_avail mark, or as disk_used_ comes down. If i'm getting it right, I think AdjustMaxDiskLimit(avail_disk) might be more clear. Also, when avail increases (say the user deletes a giant pile of non-blob junk from disk), should max_disk restore itself to its larger nominally desired value? In which case 'adjust' could mean decrease or increase. limits_.desired_max_disk vs limits_.effective_max_disk (taking availability into account) https://codereview.chromium.org/2552153002/diff/60001/storage/common/blob_sto... File storage/common/blob_storage/blob_storage_constants.h (right): https://codereview.chromium.org/2552153002/diff/60001/storage/common/blob_sto... storage/common/blob_storage/blob_storage_constants.h:34: return 2ull * memory_limit_before_paging(); why is this number a function of the mem limit? unless there's a good reason, i think a datamember that defaults to a hardcoded value would be easier to grok and work with
https://codereview.chromium.org/2552153002/diff/60001/storage/browser/blob/bl... File storage/browser/blob/blob_memory_controller.cc (right): https://codereview.chromium.org/2552153002/diff/60001/storage/browser/blob/bl... storage/browser/blob/blob_memory_controller.cc:48: // CrOS: Can you please add explanations of where (which media / disk partition) this space resides, and note whether filling up the space would result in system instability or not?
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...
@mek, @pwnall, can you PTAL? Michael is OOO this week. I updated the system to be sensitive to the available disk space, where we try to maintain a minimum space buffer, and we actively adjust disk space limit to roughly keep our usage below encroaching on that minimum space. We reevaluate our disk space whenever we do a disk operation, so if we end up getting more space and then do a disk operation (perhaps after a disk blob was deleted), then we get our space back. Tests were added for all of this. Last thing to do is add some UMAs for these edge cases. https://codereview.chromium.org/2552153002/diff/40001/storage/browser/blob/bl... File storage/browser/blob/blob_memory_controller.cc (right): https://codereview.chromium.org/2552153002/diff/40001/storage/browser/blob/bl... storage/browser/blob/blob_memory_controller.cc:618: } else { On 2016/12/08 21:17:33, michaeln wrote: > it'd be nice if this branch wasn't needed, i guess its for testing is that > right? could the tests provide a 'file_runner_'? Chatted about. This is to differentiate between incognito (Disk disabled) and regular modes. https://codereview.chromium.org/2552153002/diff/60001/storage/browser/blob/bl... File storage/browser/blob/blob_memory_controller.cc (right): https://codereview.chromium.org/2552153002/diff/60001/storage/browser/blob/bl... storage/browser/blob/blob_memory_controller.cc:48: // CrOS: On 2016/12/08 21:59:24, pwnall wrote: > Can you please add explanations of where (which media / disk partition) this > space resides, and note whether filling up the space would result in system > instability or not? Done. https://codereview.chromium.org/2552153002/diff/60001/storage/browser/blob/bl... storage/browser/blob/blob_memory_controller.cc:171: return std::make_pair(std::move(creation_info), kUnknownDiskAvailability); On 2016/12/08 21:17:33, michaeln wrote: > why say 'unkwonwavail' here, we know what it is, why not let it thru Done. https://codereview.chromium.org/2552153002/diff/60001/storage/browser/blob/bl... storage/browser/blob/blob_memory_controller.cc:183: return std::make_pair(std::move(creation_info), disk_availability); On 2016/12/08 21:17:33, michaeln wrote: > since we failed to create the file and won't be writing any data to disk, should > pair.second be actual |free_disk_space|? Done. https://codereview.chromium.org/2552153002/diff/60001/storage/browser/blob/bl... storage/browser/blob/blob_memory_controller.cc:206: return std::make_pair(std::move(creation_info), disk_availability); On 2016/12/08 21:17:33, michaeln wrote: > similar question about |free_disk| vs the locally adjusted number, but also > would it make sense to delete the borked file prior to return? Done. https://codereview.chromium.org/2552153002/diff/60001/storage/browser/blob/bl... storage/browser/blob/blob_memory_controller.cc:642: void BlobMemoryController::FreezeDiskUsage(uint64_t avail_disk) { On 2016/12/08 21:17:33, michaeln wrote: > Is "Freeze" a misnomer? If i'm understanding the CL properly the max_disk limit > is adjusted in light of the actual availability but additional disk space can > still be allocated, either when avail is not below the min_avail mark, or as > disk_used_ comes down. > > If i'm getting it right, I think AdjustMaxDiskLimit(avail_disk) might be more > clear. > > Also, when avail increases (say the user deletes a giant pile of non-blob junk > from disk), should max_disk restore itself to its larger nominally desired > value? In which case 'adjust' could mean decrease or increase. > > limits_.desired_max_disk > vs > limits_.effective_max_disk (taking availability into account) > As per our offline discussion, this is a great idea. Changed. https://codereview.chromium.org/2552153002/diff/60001/storage/common/blob_sto... File storage/common/blob_storage/blob_storage_constants.h (right): https://codereview.chromium.org/2552153002/diff/60001/storage/common/blob_sto... storage/common/blob_storage/blob_storage_constants.h:34: return 2ull * memory_limit_before_paging(); On 2016/12/08 21:17:33, michaeln wrote: > why is this number a function of the mem limit? unless there's a good reason, i > think a datamember that defaults to a hardcoded value would be easier to grok > and work with It's basically because memory_limit_before_paging() is the minimum size of a file that's being sent straight to disk - where we ask for new empty files. So I wanted to do a multiple of that. I can make this multiple higher. WDYT?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_compile_dbg_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_...)
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_...)
Here are some quick thoughts. https://codereview.chromium.org/2552153002/diff/100001/storage/browser/blob/b... File storage/browser/blob/blob_memory_controller.cc (right): https://codereview.chromium.org/2552153002/diff/100001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:42: const int64_t kUnknownDiskAvailability = -1ll; Can you make this a constexpr? https://codereview.chromium.org/2552153002/diff/100001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:62: BlobStorageLimits output; What are you using this for? https://codereview.chromium.org/2552153002/diff/100001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:72: static const size_t kTwoGigabytes = 2ull * 1024 * 1024 * 1024; Can you make this a constexpr and remove static? I assume you're using the constant name for readability, in which case I think that constexpr would let the compiler do the best job in terms of figuring out storage and instructions. https://codereview.chromium.org/2552153002/diff/100001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:113: // returned FileCreationInfos. Also returns the current available disk space currently? https://codereview.chromium.org/2552153002/diff/100001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:155: // populate file reference in returned FileCreationInfo. Also returns the disk free disk space?
mek@chromium.org changed reviewers: + mek@chromium.org - mek@google.com
https://codereview.chromium.org/2552153002/diff/120001/content/browser/blob_s... File content/browser/blob_storage/blob_memory_controller_unittest.cc (right): https://codereview.chromium.org/2552153002/diff/120001/content/browser/blob_s... content/browser/blob_storage/blob_memory_controller_unittest.cc:875: EXPECT_CALL(mock_disk_space, AmountOfFreeDiskSpace()) It seems a bit overkill to use gmock for this... You could just have a setter in your class instead. But if you are going to use gmock, you probably want to check/clear expectations (VerifyAndClearExpectations) between the various tests (and possible use StrictMock as well). That way at least you reduce the risk of getting very confusing test failures if for example the number of times AmountOfFreeDiskSpace is called changes. https://codereview.chromium.org/2552153002/diff/120001/content/browser/blob_s... content/browser/blob_storage/blob_memory_controller_unittest.cc:889: // 2. When we have < limits.min_available_disk_space(), then we'll modify our It's not quite clear to me what if any the dependencies between the various "subtests" here are. It seems like they're mostly independent, in which case it might make more sense to make them actual separate tests, rather than have them all in the same test? https://codereview.chromium.org/2552153002/diff/120001/storage/browser/blob/b... File storage/browser/blob/blob_memory_controller.cc (right): https://codereview.chromium.org/2552153002/diff/120001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:66: disk_enabled ? base::SysInfo::AmountOfTotalDiskSpace(storage_dir) : 0ull; What do you do if AmountOfTotalDiskSpace returns -1 (to indicate failure to retrieve the disk space)? https://codereview.chromium.org/2552153002/diff/120001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:118: std::tuple<std::vector<FileCreationInfo>, File::Error, int64_t> I think using a struct with named fields would be much more readable than using std::tuple for this.
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! The tests should be fixed as well This was causing an issue with a test that created the browser client on the stack but didn't run out the message loop, so the initialization of the chrome blob storage context was an issue. Daniel https://codereview.chromium.org/2552153002/diff/100001/storage/browser/blob/b... File storage/browser/blob/blob_memory_controller.cc (right): https://codereview.chromium.org/2552153002/diff/100001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:42: const int64_t kUnknownDiskAvailability = -1ll; On 2016/12/20 23:09:09, pwnall wrote: > Can you make this a constexpr? Done. https://codereview.chromium.org/2552153002/diff/100001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:62: BlobStorageLimits output; On 2016/12/20 23:09:09, pwnall wrote: > What are you using this for? Done. https://codereview.chromium.org/2552153002/diff/100001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:72: static const size_t kTwoGigabytes = 2ull * 1024 * 1024 * 1024; On 2016/12/20 23:09:09, pwnall wrote: > Can you make this a constexpr and remove static? I assume you're using the > constant name for readability, in which case I think that constexpr would let > the compiler do the best job in terms of figuring out storage and instructions. Done. https://codereview.chromium.org/2552153002/diff/100001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:113: // returned FileCreationInfos. Also returns the current available disk space On 2016/12/20 23:09:09, pwnall wrote: > currently? Done. https://codereview.chromium.org/2552153002/diff/100001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:155: // populate file reference in returned FileCreationInfo. Also returns the disk On 2016/12/20 23:09:09, pwnall wrote: > free disk space? Done. https://codereview.chromium.org/2552153002/diff/120001/content/browser/blob_s... File content/browser/blob_storage/blob_memory_controller_unittest.cc (right): https://codereview.chromium.org/2552153002/diff/120001/content/browser/blob_s... content/browser/blob_storage/blob_memory_controller_unittest.cc:875: EXPECT_CALL(mock_disk_space, AmountOfFreeDiskSpace()) On 2016/12/21 17:40:36, Marijn Kruisselbrink wrote: > It seems a bit overkill to use gmock for this... You could just have a setter in > your class instead. > But if you are going to use gmock, you probably want to check/clear expectations > (VerifyAndClearExpectations) between the various tests (and possible use > StrictMock as well). That way at least you reduce the risk of getting very > confusing test failures if for example the number of times AmountOfFreeDiskSpace > is called changes. Done. https://codereview.chromium.org/2552153002/diff/120001/content/browser/blob_s... content/browser/blob_storage/blob_memory_controller_unittest.cc:889: // 2. When we have < limits.min_available_disk_space(), then we'll modify our On 2016/12/21 17:40:36, Marijn Kruisselbrink wrote: > It's not quite clear to me what if any the dependencies between the various > "subtests" here are. It seems like they're mostly independent, in which case it > might make more sense to make them actual separate tests, rather than have them > all in the same test? Sounds good, done. https://codereview.chromium.org/2552153002/diff/120001/storage/browser/blob/b... File storage/browser/blob/blob_memory_controller.cc (right): https://codereview.chromium.org/2552153002/diff/120001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:66: disk_enabled ? base::SysInfo::AmountOfTotalDiskSpace(storage_dir) : 0ull; On 2016/12/21 17:40:36, Marijn Kruisselbrink wrote: > What do you do if AmountOfTotalDiskSpace returns -1 (to indicate failure to > retrieve the disk space)? Added comment, we ignore and use default. https://codereview.chromium.org/2552153002/diff/120001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:118: std::tuple<std::vector<FileCreationInfo>, File::Error, int64_t> On 2016/12/21 17:40:36, Marijn Kruisselbrink wrote: > I think using a struct with named fields would be much more readable than using > std::tuple for this. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
looks good, just some minor nits https://codereview.chromium.org/2552153002/diff/160001/content/browser/blob_s... File content/browser/blob_storage/blob_memory_controller_unittest.cc (right): https://codereview.chromium.org/2552153002/diff/160001/content/browser/blob_s... content/browser/blob_storage/blob_memory_controller_unittest.cc:45: virtual ~MockDiskSpaceTestGetter() {} should be override rather than virtual https://codereview.chromium.org/2552153002/diff/160001/content/browser/blob_s... content/browser/blob_storage/blob_memory_controller_unittest.cc:886: testing::Mock::VerifyAndClearExpectations(&mock_disk_space); You don't need to call this method if it's the end of your test anyway (the destructor of a mock object takes care of verifying expectations). My comment from before was because you had multiple tests in one long test. I'm still also not sure what the benefit is of using gmock over a simple class with a setter/getter; it doesn't seem like the test should care about how often AmountOfFreeDiskSpace() is called (and if you do care, you should probably be using StrictMock)? And it's not like using gmock saves you much code either. But feel free to ignore my dislike of gmock usage where it doesn't seem to gain anything. https://codereview.chromium.org/2552153002/diff/160001/storage/browser/blob/b... File storage/browser/blob/blob_memory_controller.cc (right): https://codereview.chromium.org/2552153002/diff/160001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:69: // Don't do speciatly configuration for error size (-1). *specialty https://codereview.chromium.org/2552153002/diff/160001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:129: int64_t disk_availability; nit: your default constructor leaves this uninitialized; which might be okay, but might be cleaner to always make sure everything is initialized
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! Michael & Victor, can you PTAL? https://codereview.chromium.org/2552153002/diff/160001/content/browser/blob_s... File content/browser/blob_storage/blob_memory_controller_unittest.cc (right): https://codereview.chromium.org/2552153002/diff/160001/content/browser/blob_s... content/browser/blob_storage/blob_memory_controller_unittest.cc:45: virtual ~MockDiskSpaceTestGetter() {} On 2017/01/05 22:32:23, Marijn Kruisselbrink wrote: > should be override rather than virtual Done. https://codereview.chromium.org/2552153002/diff/160001/content/browser/blob_s... content/browser/blob_storage/blob_memory_controller_unittest.cc:886: testing::Mock::VerifyAndClearExpectations(&mock_disk_space); On 2017/01/05 22:32:23, Marijn Kruisselbrink wrote: > You don't need to call this method if it's the end of your test anyway (the > destructor of a mock object takes care of verifying expectations). My comment > from before was because you had multiple tests in one long test. > > I'm still also not sure what the benefit is of using gmock over a simple class > with a setter/getter; it doesn't seem like the test should care about how often > AmountOfFreeDiskSpace() is called (and if you do care, you should probably be > using StrictMock)? And it's not like using gmock saves you much code either. But > feel free to ignore my dislike of gmock usage where it doesn't seem to gain > anything. Ah, that makes sense. I switched it to StrictMock as I want to verify that the method is called exactly one time per expect_call. https://codereview.chromium.org/2552153002/diff/160001/storage/browser/blob/b... File storage/browser/blob/blob_memory_controller.cc (right): https://codereview.chromium.org/2552153002/diff/160001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:69: // Don't do speciatly configuration for error size (-1). On 2017/01/05 22:32:23, Marijn Kruisselbrink wrote: > *specialty Done. https://codereview.chromium.org/2552153002/diff/160001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:129: int64_t disk_availability; On 2017/01/05 22:32:23, Marijn Kruisselbrink wrote: > nit: your default constructor leaves this uninitialized; which might be okay, > but might be cleaner to always make sure everything is initialized Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2552153002/diff/160001/storage/browser/blob/b... File storage/browser/blob/blob_memory_controller.cc (right): https://codereview.chromium.org/2552153002/diff/160001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:152: free_disk_space = base::SysInfo::AmountOfFreeDiskSpace(blob_storage_dir); nit: might be nice to avoid or better bury this branching logic for testing, its repeated in two places? A free_disk_space_func functionptr data member whose default value points to the base::SysInfo method might be a little cleaner. Callsites could unconditionally call the funcptr. https://codereview.chromium.org/2552153002/diff/160001/storage/browser/blob/b... File storage/browser/blob/blob_memory_controller.h (right): https://codereview.chromium.org/2552153002/diff/160001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.h:170: class DiskSpaceTestGetter { class for this might be overkill, would a function ptr might be a better fit?
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@chromium.org
Thanks! +mpearson for histograms review. https://codereview.chromium.org/2552153002/diff/160001/storage/browser/blob/b... File storage/browser/blob/blob_memory_controller.cc (right): https://codereview.chromium.org/2552153002/diff/160001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:152: free_disk_space = base::SysInfo::AmountOfFreeDiskSpace(blob_storage_dir); On 2017/01/06 20:58:52, michaeln wrote: > nit: might be nice to avoid or better bury this branching logic for testing, its > repeated in two places? > > A free_disk_space_func functionptr data member whose default value points to the > base::SysInfo method might be a little cleaner. Callsites could unconditionally > call the funcptr. Done. https://codereview.chromium.org/2552153002/diff/160001/storage/browser/blob/b... File storage/browser/blob/blob_memory_controller.h (right): https://codereview.chromium.org/2552153002/diff/160001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.h:170: class DiskSpaceTestGetter { On 2017/01/06 20:58:52, michaeln wrote: > class for this might be overkill, would a function ptr might be a better fit? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2552153002/diff/220001/storage/browser/blob/b... File storage/browser/blob/blob_memory_controller.cc (right): https://codereview.chromium.org/2552153002/diff/220001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:89: UMA_HISTOGRAM_COUNTS_1M("Storage.Blob.MaxDiskSpace", Per my data logging tech talk, I'm always worried about histogram emits that happen in helper implementation like this. It's easy for someone else to add a call to this function and not realize that in so doing they're messing up your UMA statistics. How can you be sure that no one else will call CalculateBlobStorageLimits() at another time? https://codereview.chromium.org/2552153002/diff/220001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:709: limits_.desired_max_disk_space) { This test doesn't seem right to me. And suppose effective_max_disk_space=5mb, min_available_disk_space = 4mb and desired_max_disk_space=5mb and avail_disk=8mb. We see mode NORMAL. I would think we'd be happy with this. After all, the available disk is more than the desired max. Yet, here you might be making an adjustment. That doesn't always seem right. (If disk_used is 0, then we'd leave the effective_max_disk_space to 5. This isn't an adjustment and yet you'd emit an LOWERED message to the histogram. If disk_used is 4mb, we'd increase the effective max to 8, which is an increase from 5, yet you'd emit it under the LOWERED message again.) I must be missing something here.
Here a few more small comments. FWIW, michaeln's first function pointer suggestion really nailed the thing that bothered me the most, but I didn't know how to fix nicely. https://codereview.chromium.org/2552153002/diff/220001/content/browser/blob_s... File content/browser/blob_storage/blob_memory_controller_unittest.cc (right): https://codereview.chromium.org/2552153002/diff/220001/content/browser/blob_s... content/browser/blob_storage/blob_memory_controller_unittest.cc:30: using ::testing::StrictMock; Is this used anywhere? I don't see it in the diff. https://codereview.chromium.org/2552153002/diff/220001/content/browser/blob_s... content/browser/blob_storage/blob_memory_controller_unittest.cc:677: items, GetMemoryRequestCallbackToOutput(memory_requested + i)); I'm not an expert in the style guide around pointer arithmetic, but I'd find it easier to read &memory_requested[i]. It tells me you're passing a single array element, not a buffer starting at that position. This is a super-tiny nit, feel free to ignore if anyone disagrees. https://codereview.chromium.org/2552153002/diff/220001/content/browser/blob_s... content/browser/blob_storage/blob_memory_controller_unittest.cc:692: // Note: this can fail if the bot's disk is almost full. Can you add an ASSERT above that checks for as much disk space as you need, and fails if the precondition is not met? Ideally, it'd have a message that helps someone reading the logs understand what happened. For the sake of simplicity, I'd be OK with a gross over-estimation of the needed free space (1GB?), in the interest of having a simple check with a clear message. https://codereview.chromium.org/2552153002/diff/220001/content/browser/blob_s... content/browser/blob_storage/blob_memory_controller_unittest.cc:749: // Still can't add anything. It seems to me that the 2 expectations here are described quite well by the "All quota should be allocated" comment above. If that's the case, you can remove lines 748+749. https://codereview.chromium.org/2552153002/diff/220001/content/browser/blob_s... content/browser/blob_storage/blob_memory_controller_unittest.cc:776: EXPECT_FALSE(file_runner_->HasPendingTask()); Is this check redundant with the one on line 767? https://codereview.chromium.org/2552153002/diff/220001/storage/browser/blob/b... File storage/browser/blob/blob_memory_controller.cc (right): https://codereview.chromium.org/2552153002/diff/220001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:172: std::pair<FileCreationInfo, int64_t> CreateFileAndWriteItems( Would it add too much code to use a struct with named members instead of std::pair here? It'd remove the need to mentally track what the int64_t in the pair means.
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 replied to comments inline, PTAL. https://codereview.chromium.org/2552153002/diff/220001/content/browser/blob_s... File content/browser/blob_storage/blob_memory_controller_unittest.cc (right): https://codereview.chromium.org/2552153002/diff/220001/content/browser/blob_s... content/browser/blob_storage/blob_memory_controller_unittest.cc:30: using ::testing::StrictMock; On 2017/01/07 00:46:21, pwnall wrote: > Is this used anywhere? I don't see it in the diff. Done. https://codereview.chromium.org/2552153002/diff/220001/content/browser/blob_s... content/browser/blob_storage/blob_memory_controller_unittest.cc:677: items, GetMemoryRequestCallbackToOutput(memory_requested + i)); On 2017/01/07 00:46:21, pwnall wrote: > I'm not an expert in the style guide around pointer arithmetic, but I'd find it > easier to read &memory_requested[i]. It tells me you're passing a single array > element, not a buffer starting at that position. > > This is a super-tiny nit, feel free to ignore if anyone disagrees. Done. https://codereview.chromium.org/2552153002/diff/220001/content/browser/blob_s... content/browser/blob_storage/blob_memory_controller_unittest.cc:692: // Note: this can fail if the bot's disk is almost full. On 2017/01/07 00:46:21, pwnall wrote: > Can you add an ASSERT above that checks for as much disk space as you need, and > fails if the precondition is not met? Ideally, it'd have a message that helps > someone reading the logs understand what happened. > > For the sake of simplicity, I'd be OK with a gross over-estimation of the needed > free space (1GB?), in the interest of having a simple check with a clear > message. Done. I added a check for the most we'll ever use, which is 1000 bytes. https://codereview.chromium.org/2552153002/diff/220001/content/browser/blob_s... content/browser/blob_storage/blob_memory_controller_unittest.cc:749: // Still can't add anything. On 2017/01/07 00:46:21, pwnall wrote: > It seems to me that the 2 expectations here are described quite well by the "All > quota should be allocated" comment above. If that's the case, you can remove > lines 748+749. Done. https://codereview.chromium.org/2552153002/diff/220001/content/browser/blob_s... content/browser/blob_storage/blob_memory_controller_unittest.cc:776: EXPECT_FALSE(file_runner_->HasPendingTask()); On 2017/01/07 00:46:21, pwnall wrote: > Is this check redundant with the one on line 767? Done. https://codereview.chromium.org/2552153002/diff/220001/storage/browser/blob/b... File storage/browser/blob/blob_memory_controller.cc (right): https://codereview.chromium.org/2552153002/diff/220001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:89: UMA_HISTOGRAM_COUNTS_1M("Storage.Blob.MaxDiskSpace", On 2017/01/07 00:07:58, Mark P wrote: > Per my data logging tech talk, I'm always worried about histogram emits that > happen in helper implementation like this. It's easy for someone else to add a > call to this function and not realize that in so doing they're messing up your > UMA statistics. How can you be sure that no one else will call > CalculateBlobStorageLimits() at another time? I added it to the private section and friended the only class that should call it. Also added a comment. https://codereview.chromium.org/2552153002/diff/220001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:172: std::pair<FileCreationInfo, int64_t> CreateFileAndWriteItems( On 2017/01/07 00:46:21, pwnall wrote: > Would it add too much code to use a struct with named members instead of > std::pair here? It'd remove the need to mentally track what the int64_t in the > pair means. It would pollute the .h file, as it's a parameter in a private method, and the class needs to have a default constructor, args constructor, and move constructor & assignment, as well as members, so it would be a lot of code for not much clarification (I immediately use variable names that are clear). So I'd like to keep it the same. I added a comment in the .h file for OnEvictionComplete to clarify the int as "avail_disk". https://codereview.chromium.org/2552153002/diff/220001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:709: limits_.desired_max_disk_space) { On 2017/01/07 00:07:58, Mark P wrote: > This test doesn't seem right to me. > And suppose effective_max_disk_space=5mb, min_available_disk_space = 4mb and > desired_max_disk_space=5mb and avail_disk=8mb. We see mode NORMAL. > I would think we'd be happy with this. After all, the available disk is more > than the desired max. Yet, here you might be making an adjustment. > > That doesn't always seem right. (If disk_used is 0, then we'd leave the > effective_max_disk_space to 5. This isn't an adjustment and yet you'd emit an > LOWERED message to the histogram. If disk_used is 4mb, we'd increase the > effective max to 8, which is an increase from 5, yet you'd emit it under the > LOWERED message again.) > > I must be missing something here. So in that example, with disk_used_ = 0, we actually reduce effective disk to 4mb (8mb - 4mb). This is correct, we are lowered. However, you second example is correctly weird (we would be at 8mb). I fixed the if-statement here to account for the disk_used, and added a test for it (DiskSpaceBeforeMinAvailable). We were incorrectly detecting this before it was needed, and having the quota increased past the max. Thanks for the find! It looks like this was the only test case not tested (we have below min_available, within min_available + max, and now within min_available + max + usage). Now we correctly don't trigger for that condition, as we're using 4mb (disk_usage_ = 4) and we have 8mb free, so that means we've used 4mb of our 5mb quota. Since we just have 1mb left of our potential quota, this leaves more than enough room for our min availability of 4 (we have 7mb 'available').
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2552153002/diff/240001/content/browser/blob_s... File content/browser/blob_storage/blob_memory_controller_unittest.cc (right): https://codereview.chromium.org/2552153002/diff/240001/content/browser/blob_s... content/browser/blob_storage/blob_memory_controller_unittest.cc:20: #include "testing/gmock/include/gmock/gmock.h" nit: You're no longer using gmock I believe?
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! mpearson & pwnall: Can you PTAL? https://codereview.chromium.org/2552153002/diff/240001/content/browser/blob_s... File content/browser/blob_storage/blob_memory_controller_unittest.cc (right): https://codereview.chromium.org/2552153002/diff/240001/content/browser/blob_s... content/browser/blob_storage/blob_memory_controller_unittest.cc:20: #include "testing/gmock/include/gmock/gmock.h" On 2017/01/10 21:41:06, Marijn Kruisselbrink wrote: > nit: You're no longer using gmock I believe? good catch, thanks.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Sorry for the delay. Too many other reviews (mostly design docs and experiments)... --mark https://codereview.chromium.org/2552153002/diff/260001/storage/browser/blob/b... File storage/browser/blob/blob_memory_controller.cc (right): https://codereview.chromium.org/2552153002/diff/260001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:711: avail_disk + disk_used_ - limits_.min_available_disk_space(); I feel like it's possible that this could increase the effective_max_disk_space above desired_max_disk_space. Are you sure it can't? https://codereview.chromium.org/2552153002/diff/260001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:712: if (curr_status != ADJUSTED) { Why are you skipping emitted if this is ADJUSTED? It seems possible to me that you could've adjusted it to a different value than before. In general, I'd prefer if these three tests that guard the UMA_HISTOGRAM emits all are of the form if (old value != new value) UMA_HISTOGRAM_... That makes the behavior clearer and less likely to have edge cases. https://codereview.chromium.org/2552153002/diff/260001/storage/browser/blob/b... File storage/browser/blob/blob_memory_controller.h (right): https://codereview.chromium.org/2552153002/diff/260001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.h:177: protected: nit: why did you add this? https://codereview.chromium.org/2552153002/diff/260001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.h:186: // This should not be called more than once in a browser run as we record UMA "A browser run" sounds to me like something that can happen only once between restarts. Is my interpretation correct? It doesn't agree with what you say in histograms.xml. https://codereview.chromium.org/2552153002/diff/260001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.h:187: // stats that we expect to only be called once. nit: called -> emitted (or recorded) https://codereview.chromium.org/2552153002/diff/260001/storage/common/blob_st... File storage/common/blob_storage/blob_storage_constants.h (right): https://codereview.chromium.org/2552153002/diff/260001/storage/common/blob_st... storage/common/blob_storage/blob_storage_constants.h:33: uint64_t min_available_disk_space() const { As someone reviewing code that uses this struct, I find the variable / function names confusing, especially min_available_disk_space and desired_max_disk_space. I read these originally as the minimum available disk space that the blog system needs and the desired maximum disk space the blob system needs. If I read the comments within this struct (which it took me a while to do, because I thought my interpretation was obvious based on the names), the difference is clear. The former refers to min available *outside the blob system* and the latter refers to *within the blob system*. Take this or leave it. You may want to revise these to prevent confusing from future engineers.
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 Mark! That was helpful, I hope I clarified things w/ the variable name change and a little more specificity in the histograms.xml. I'm also planning on writing up a README.md for the whole blob system after this, as well as a BestPractices.md for how to use blobs. Hopefully this will also help. https://codereview.chromium.org/2552153002/diff/260001/storage/browser/blob/b... File storage/browser/blob/blob_memory_controller.cc (right): https://codereview.chromium.org/2552153002/diff/260001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:711: avail_disk + disk_used_ - limits_.min_available_disk_space(); On 2017/01/10 23:56:59, Mark P wrote: > I feel like it's possible that this could increase the effective_max_disk_space > above desired_max_disk_space. Are you sure it can't? Yes, if you subtract |limits_.min_available_disk_space()| from both sides of the if-statement equation, it guarantees that |limits_.effective_max_disk_space| is < max. I tried to make this clearer with a comment, and I combined avail_disk + disk_used_ into a single variable to make it clearer. https://codereview.chromium.org/2552153002/diff/260001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:712: if (curr_status != ADJUSTED) { On 2017/01/10 23:57:00, Mark P wrote: > Why are you skipping emitted if this is ADJUSTED? It seems possible to me that > you could've adjusted it to a different value than before. > > In general, I'd prefer if these three tests that guard the UMA_HISTOGRAM emits > all are of the form > if (old value != new value) > UMA_HISTOGRAM_... > > That makes the behavior clearer and less likely to have edge cases. > I want to measure state changes here, not value changes within a state. I clarified this in histograms.xml, and kept the behavior here. However, to protect more against having the same |limits_.effective_max_disk_space|, I added a check to each if-statement. https://codereview.chromium.org/2552153002/diff/260001/storage/browser/blob/b... File storage/browser/blob/blob_memory_controller.h (right): https://codereview.chromium.org/2552153002/diff/260001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.h:177: protected: On 2017/01/10 23:57:00, Mark P wrote: > nit: why did you add this? rmeoved. https://codereview.chromium.org/2552153002/diff/260001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.h:186: // This should not be called more than once in a browser run as we record UMA On 2017/01/10 23:57:00, Mark P wrote: > "A browser run" sounds to me like something that can happen only once between > restarts. Is my interpretation correct? It doesn't agree with what you say in > histograms.xml. Done. https://codereview.chromium.org/2552153002/diff/260001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.h:187: // stats that we expect to only be called once. On 2017/01/10 23:57:00, Mark P wrote: > nit: called -> emitted > (or recorded) Done. https://codereview.chromium.org/2552153002/diff/260001/storage/common/blob_st... File storage/common/blob_storage/blob_storage_constants.h (right): https://codereview.chromium.org/2552153002/diff/260001/storage/common/blob_st... storage/common/blob_storage/blob_storage_constants.h:33: uint64_t min_available_disk_space() const { On 2017/01/10 23:57:00, Mark P wrote: > As someone reviewing code that uses this struct, I find the variable / function > names confusing, especially min_available_disk_space and desired_max_disk_space. > > I read these originally as the minimum available disk space that the blog system > needs and the desired maximum disk space the blob system needs. > > If I read the comments within this struct (which it took me a while to do, > because I thought my interpretation was obvious based on the names), the > difference is clear. The former refers to min available *outside the blob > system* and the latter refers to *within the blob system*. > > Take this or leave it. You may want to revise these to prevent confusing from > future engineers. > I changed this to |min_available_external_disk_space()|
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_...)
metrics lgtm with a number of comments to still wrap up --mark https://codereview.chromium.org/2552153002/diff/260001/storage/browser/blob/b... File storage/browser/blob/blob_memory_controller.cc (right): https://codereview.chromium.org/2552153002/diff/260001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:712: if (curr_status != ADJUSTED) { Please put this statement as a comment somewhere here. > I want to measure state changes here, not value changes within a state. https://codereview.chromium.org/2552153002/diff/280001/storage/browser/blob/b... File storage/browser/blob/blob_memory_controller.cc (right): https://codereview.chromium.org/2552153002/diff/280001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:706: if (curr_status != FROZEN && optional nit: parens around binary operators in this test and the ones below https://codereview.chromium.org/2552153002/diff/280001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:714: // Subtraction is avoided the if statement to prevent overflows. I cannot parse this sentence. Also, it's usually *addition* that causes overflows. https://codereview.chromium.org/2552153002/diff/280001/storage/browser/blob/b... File storage/browser/blob/blob_memory_controller.h (right): https://codereview.chromium.org/2552153002/diff/280001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.h:185: // This should only be called once per storage partition initialization as we This comment isn't consistent with histograms.xml, which says it can be called as part of "any file operations in the blob system". https://codereview.chromium.org/2552153002/diff/280001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2552153002/diff/280001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:64371: + Recorded when the blob system changes adjustment types on it's disk size. nit: it's -> its https://codereview.chromium.org/2552153002/diff/280001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:64374: + can happen after any file operations in the blob system (paging data to nit: remove extra space https://codereview.chromium.org/2552153002/diff/280001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:64375: + disk or saving a new blob directly to disk). This also records the I don't think this last sentence adds anything.
Thanks! https://codereview.chromium.org/2552153002/diff/260001/storage/browser/blob/b... File storage/browser/blob/blob_memory_controller.cc (right): https://codereview.chromium.org/2552153002/diff/260001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:712: if (curr_status != ADJUSTED) { On 2017/01/11 17:49:14, Mark P wrote: > Please put this statement as a comment somewhere here. > > I want to measure state changes here, not value changes within a state. Done. https://codereview.chromium.org/2552153002/diff/280001/storage/browser/blob/b... File storage/browser/blob/blob_memory_controller.cc (right): https://codereview.chromium.org/2552153002/diff/280001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:706: if (curr_status != FROZEN && On 2017/01/11 17:49:14, Mark P wrote: > optional nit: parens around binary operators in this test and the ones below Acknowledged. I prefer w/o parens. https://codereview.chromium.org/2552153002/diff/280001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.cc:714: // Subtraction is avoided the if statement to prevent overflows. On 2017/01/11 17:49:14, Mark P wrote: > I cannot parse this sentence. > Also, it's usually *addition* that causes overflows. Whoops, meant underflow. Since I moved the addition into the above variable, I removed the comment. https://codereview.chromium.org/2552153002/diff/280001/storage/browser/blob/b... File storage/browser/blob/blob_memory_controller.h (right): https://codereview.chromium.org/2552153002/diff/280001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.h:185: // This should only be called once per storage partition initialization as we On 2017/01/11 17:49:14, Mark P wrote: > This comment isn't consistent with histograms.xml, which says it can be called > as part of "any file operations in the blob system". That's for "MaxDiskSpaceAdjustment". "MaxDiskSpace", which is what is recorded here, is recorded once per storage partition initialization, which is what it says in the histograms.xml https://codereview.chromium.org/2552153002/diff/280001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2552153002/diff/280001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:64371: + Recorded when the blob system changes adjustment types on it's disk size. On 2017/01/11 17:49:14, Mark P wrote: > nit: it's -> its Done. https://codereview.chromium.org/2552153002/diff/280001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:64374: + can happen after any file operations in the blob system (paging data to On 2017/01/11 17:49:14, Mark P wrote: > nit: remove extra space Done. https://codereview.chromium.org/2552153002/diff/280001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:64375: + disk or saving a new blob directly to disk). This also records the On 2017/01/11 17:49:14, Mark P wrote: > I don't think this last sentence adds anything. Done.
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, mek@chromium.org, mpearson@chromium.org Link to the patchset: https://codereview.chromium.org/2552153002/#ps300001 (title: "Mark's comment")
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
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
dmurph@chromium.org changed reviewers: + boliu@chromium.org
+boliu for content/browser/web_contents/web_contents_impl_unittest.cc
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...
On 2017/01/11 19:35:11, dmurph wrote: > +boliu for content/browser/web_contents/web_contents_impl_unittest.cc lgtm
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, mpearson@chromium.org, mek@chromium.org Link to the patchset: https://codereview.chromium.org/2552153002/#ps320001 (title: "added comments to WebContentsImplTest")
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
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...)
https://codereview.chromium.org/2552153002/diff/280001/storage/browser/blob/b... File storage/browser/blob/blob_memory_controller.h (right): https://codereview.chromium.org/2552153002/diff/280001/storage/browser/blob/b... storage/browser/blob/blob_memory_controller.h:185: // This should only be called once per storage partition initialization as we On 2017/01/11 18:25:29, dmurph wrote: > On 2017/01/11 17:49:14, Mark P wrote: > > This comment isn't consistent with histograms.xml, which says it can be called > > as part of "any file operations in the blob system". > > That's for "MaxDiskSpaceAdjustment". "MaxDiskSpace", which is what is recorded > here, is recorded once per storage partition initialization, which is what it > says in the histograms.xml Acknowledged. Oops, sorry I mixed them up. /me sheepish
The CQ bit was checked by dmurph@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 320001, "attempt_start_ts": 1484180864547060,
"parent_rev": "99bed8e7ec46b0943261ffb46894503605aeba3f", "commit_rev":
"bf104bd8baf5e73b71c82ba60249cb778b8db099"}
Message was sent while issue was closed.
Description was changed from ========== [BlobStorage] Enabling disk paging and direct storage. Adds disk configuration calculations that are slightly specialized for Android, ChromeOS, and general desktop. The max memory space and disk spaced are set based on the total RAM and disk of the device. BUG=375297 ========== to ========== [BlobStorage] Enabling disk paging and direct storage. Adds disk configuration calculations that are slightly specialized for Android, ChromeOS, and general desktop. The max memory space and disk spaced are set based on the total RAM and disk of the device. BUG=375297 Review-Url: https://codereview.chromium.org/2552153002 Cr-Commit-Position: refs/heads/master@{#443139} Committed: https://chromium.googlesource.com/chromium/src/+/bf104bd8baf5e73b71c82ba60249... ==========
Message was sent while issue was closed.
Committed patchset #17 (id:320001) as https://chromium.googlesource.com/chromium/src/+/bf104bd8baf5e73b71c82ba60249... |
