[BlobStorage] BlobMemoryController & tests

storage/browser/blob/blob_memory_controller.cc

Index: storage/browser/blob/blob_memory_controller.cc
diff --git a/storage/browser/blob/blob_memory_controller.cc b/storage/browser/blob/blob_memory_controller.cc
new file mode 100644
index 0000000000000000000000000000000000000000..2d466342a041400255c7ddc8ca3f67a32ddc94bc
--- /dev/null
+++ b/storage/browser/blob/blob_memory_controller.cc
@@ -0,0 +1,699 @@
+// Copyright 2016 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "storage/browser/blob/blob_memory_controller.h"
+
+#include <algorithm>
+
+#include "base/callback.h"
+#include "base/callback_helpers.h"
+#include "base/containers/small_map.h"
+#include "base/files/file_util.h"
+#include "base/location.h"
+#include "base/memory/ptr_util.h"
+#include "base/metrics/histogram_macros.h"
+#include "base/numerics/safe_conversions.h"
+#include "base/numerics/safe_math.h"
+#include "base/single_thread_task_runner.h"
+#include "base/stl_util.h"
+#include "base/strings/string_number_conversions.h"
+#include "base/task_runner.h"
+#include "base/task_runner_util.h"
+#include "base/threading/thread_restrictions.h"
+#include "base/time/time.h"
+#include "base/trace_event/trace_event.h"
+#include "base/tuple.h"
+#include "storage/browser/blob/blob_data_builder.h"
+#include "storage/browser/blob/blob_data_item.h"
+#include "storage/browser/blob/shareable_blob_data_item.h"
+#include "storage/browser/blob/shareable_file_reference.h"
+#include "storage/common/data_element.h"
+
+using base::File;
+using base::FilePath;
+using FileCreationInfo = storage::BlobMemoryController::FileCreationInfo;

[michaeln, 2016/09/27 00:09:29]

looks like most of the usages are fully qualified, consider removing the using
statement

[dmurph, 2016/09/29 00:44:22]

It shortens a lot of code, I unqualified the instances, hopefully that helps.

+
+namespace storage {
+namespace {
+using QuotaAllocationTask = BlobMemoryController::QuotaAllocationTask;
+
+using MemoryQuotaRequestCallback =
+    BlobMemoryController::MemoryQuotaRequestCallback;
+using MemoryQuotaRequestCallback =
+    BlobMemoryController::MemoryQuotaRequestCallback;

[michaeln, 2016/09/27 00:09:29]

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?

[dmurph, 2016/09/29 00:44:22]

Done.

+
+// Here so we can destruct files on the correct thread if the user cancels.
+void DestructFiles(std::vector<BlobMemoryController::FileCreationInfo> files) {}

[michaeln, 2016/09/28 21:19:27]

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.

[dmurph, 2016/09/29 00:44:20]

You're right. This is no longer used, so I'll remove it.

+
+// Creates a file in the given directory w/ the given filename and size.

[pwnall, 2016/09/26 13:18:08]

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.

+std::vector<BlobMemoryController::FileCreationInfo> CreateFiles(

[michaeln, 2016/09/28 01:08:04]

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.

[dmurph, 2016/09/29 00:44:21]

Sounds good. Thanks for the chat about this, I think it's looking better now.

+    std::vector<scoped_refptr<ShareableFileReference>> file_references) {
+  base::ThreadRestrictions::AssertIOAllowed();
+  std::vector<BlobMemoryController::FileCreationInfo> result;
+
+  for (scoped_refptr<ShareableFileReference>& file_ref : file_references) {

[pwnall, 2016/09/26 13:18:07]

You're using file_ref here, but file_reference in the method below. How about
standardizing on file_reference?

[dmurph, 2016/09/29 00:44:21]

Done.

+    result.push_back(BlobMemoryController::FileCreationInfo());
+    BlobMemoryController::FileCreationInfo& creation_info = result.back();

[pwnall, 2016/09/26 13:18:08]

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?

[dmurph, 2016/09/29 00:44:21]

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.

+    // Try to open our file.
+    File file(file_ref->path(), File::FLAG_CREATE_ALWAYS | File::FLAG_WRITE);
+    creation_info.file_reference = std::move(file_ref);
+    creation_info.error = file.error_details();
+    UMA_HISTOGRAM_ENUMERATION("Storage.Blob.TransportFileCreate",
+                              -creation_info.error, -File::FILE_ERROR_MAX);
+    if (creation_info.error != File::FILE_OK)
+      return std::vector<BlobMemoryController::FileCreationInfo>();
+
+    // Grab the file info to get the "last modified" time and store the file.
+    File::Info file_info;
+    bool success = file.GetInfo(&file_info);
+    UMA_HISTOGRAM_BOOLEAN("Storage.Blob.TransportFileInfoSuccess", success);
+    creation_info.error = success ? File::FILE_OK : File::FILE_ERROR_FAILED;
+    if (!success)
+      return std::vector<BlobMemoryController::FileCreationInfo>();
+    creation_info.file = std::move(file);
+  }
+  return result;
+}
+
+BlobMemoryController::FileCreationInfo WriteItemsToFile(

[pwnall, 2016/09/26 13:18:08]

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.

[dmurph, 2016/09/29 00:44:21]

Done.

+    std::vector<scoped_refptr<ShareableBlobDataItem>>* items,

[pwnall, 2016/09/26 13:18:08]

You don't seem to be changing the vector in this method. Would const& be more
appropriate than *?

[dmurph, 2016/09/29 00:44:20]

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.

+    size_t total_size_bytes,
+    scoped_refptr<ShareableFileReference> file_reference) {
+  DCHECK_NE(0u, total_size_bytes);
+  UMA_HISTOGRAM_MEMORY_KB("Storage.Blob.PageFileSize", total_size_bytes / 1024);
+  base::ThreadRestrictions::AssertIOAllowed();

[pwnall, 2016/09/26 13:18:08]

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?

[dmurph, 2016/09/29 00:44:22]

Ah, yeah we don't have access to that here, that's in content/ land and we're in
storage/

+
+  // Create our file.

[pwnall, 2016/09/26 13:18:09]

our -> the page?

[dmurph, 2016/09/29 00:44:22]

Done.

+  BlobMemoryController::FileCreationInfo creation_info;
+  File file(file_reference->path(),
+            File::FLAG_CREATE_ALWAYS | File::FLAG_WRITE);
+  creation_info.file_reference = std::move(file_reference);
+  creation_info.error = file.error_details();
+  UMA_HISTOGRAM_ENUMERATION("Storage.Blob.PageFileCreate", -creation_info.error,
+                            -File::FILE_ERROR_MAX);
+  if (creation_info.error != File::FILE_OK)
+    return creation_info;
+
+  // Write data.
+  file.SetLength(total_size_bytes);
+  int bytes_written = 0;
+  for (const auto& refptr : *items) {

[pwnall, 2016/09/26 13:18:07]

refptr -> shareable_blob_item_reference?

[dmurph, 2016/09/29 00:44:21]

Done.

+    const DataElement& element = refptr->item()->data_element();
+    DCHECK_EQ(DataElement::TYPE_BYTES, element.type());
+    size_t length = base::checked_cast<size_t>(element.length());
+    size_t bytes_left = length;
+    while (bytes_left > 0) {
+      bytes_written =
+          file.WriteAtCurrentPos(element.bytes() + (length - bytes_left),
+                                 base::saturated_cast<int>(bytes_left));
+      if (bytes_written < 0)
+        break;
+      DCHECK_LE(static_cast<size_t>(bytes_written), bytes_left);
+      bytes_left -= bytes_written;
+    }
+    if (bytes_written < 0)
+      break;
+  }
+  UMA_HISTOGRAM_BOOLEAN("Storage.Blob.PageFileWriteSuccess", bytes_written > 0);
+
+  // Grab our modification time and create our SharedFileReference to manage the

[pwnall, 2016/09/26 13:18:09]

Where is the SharedFileReference created?

[dmurph, 2016/09/29 00:44:22]

Ah, this was moved out.

+  // lifetime of the file.
+  File::Info info;
+  bool success = file.GetInfo(&info);
+  UMA_HISTOGRAM_BOOLEAN("Storage.Blob.PageFileInfoSuccess", success);
+  creation_info.error =
+      bytes_written < 0 || !success ? File::FILE_ERROR_FAILED : File::FILE_OK;
+  creation_info.last_modified = info.last_modified;
+  return creation_info;
+}
+
+std::vector<scoped_refptr<ShareableBlobDataItem>> WrapInRefPtrs(
+    const std::vector<ShareableBlobDataItem*> items_ptrs) {
+  std::vector<scoped_refptr<ShareableBlobDataItem>> result;
+  for (ShareableBlobDataItem* item : items_ptrs) {
+    result.push_back(make_scoped_refptr(item));
+  }
+  return result;
+}
+
+void GetFileSizesAndTotalSize(

[pwnall, 2016/09/26 13:18:09]

Would it make sense turn total_size_output into the return value? In that case,
I suppose you'd rename to GetTotalSizeAndFileSizes().

[dmurph, 2016/09/29 00:44:22]

Sure.

+    const std::vector<ShareableBlobDataItem*>& unreserved_file_items,
+    std::vector<uint64_t>* file_sizes_output,
+    uint64_t* total_size_output) {
+  *total_size_output = 0;
+  base::SmallMap<std::map<uint64_t, uint64_t>> file_sizes;
+  for (ShareableBlobDataItem* item : unreserved_file_items) {
+    const DataElement& element = item->item()->data_element();
+    uint64_t file_id = BlobDataBuilder::GetFutureFileID(element);
+    auto it = file_sizes.find(file_id);
+    if (it != file_sizes.end()) {
+      it->second = std::max(it->second, element.offset() + element.length());
+    } else {
+      file_sizes[file_id] = element.offset() + element.length();
+    }
+    *total_size_output += element.length();
+  }
+  for (const auto& size_pair : file_sizes) {
+    file_sizes_output->push_back(size_pair.second);
+  }
+}
+
+}  // namespace
+
+BlobMemoryController::QuotaAllocationTask::~QuotaAllocationTask() {}
+
+// Both tasks store a list iterator to their current position in the task list,

[pwnall, 2016/09/26 13:18:07]

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? 

[dmurph, 2016/09/29 00:44:20]

Done.

+// and the total size of their allocation.
+template <typename T>
+class BlobMemoryController::BaseQuotaAllocationTask
+    : public BlobMemoryController::QuotaAllocationTask {
+ public:
+  BaseQuotaAllocationTask() : allocation_size_(0) {}

[pwnall, 2016/09/26 13:18:09]

Do you need the default constructor to be able to have a vector of tasks?

[dmurph, 2016/09/29 00:44:20]

No, we need it for FileQuotaAllocationTask, where we don't have the allocation
size right away.

+  explicit BaseQuotaAllocationTask(uint64_t allocation_size)
+      : allocation_size_(allocation_size) {}
+
+  ~BaseQuotaAllocationTask() override {}

[pwnall, 2016/09/26 13:18:08]

would = default work here?

[dmurph, 2016/09/29 00:44:21]

Done.

+
+  void set_my_list_position(typename T::iterator current_position) {

[pwnall, 2016/09/26 13:18:08]

The argument name seems odd, given everything else around it.

[dmurph, 2016/09/29 00:44:22]

Done.

+    my_list_position_ = current_position;
+  }
+  typename T::iterator my_list_position() { return my_list_position_; }
+
+  uint64_t allocation_size() { return allocation_size_; }
+  void set_allocation_size(uint64_t allocation_size) {
+    allocation_size_ = allocation_size;
+  }
+
+ private:
+  uint64_t allocation_size_;
+  typename T::iterator my_list_position_;
+};
+
+class BlobMemoryController::MemoryQuotaAllocationTask
+    : public BlobMemoryController::BaseQuotaAllocationTask<
+          PendingMemoryQuotaTaskList> {
+ public:
+  MemoryQuotaAllocationTask(
+      BlobMemoryController* controller,
+      uint64_t quota_request_size,
+      std::vector<scoped_refptr<ShareableBlobDataItem>> pending_items,
+      MemoryQuotaRequestCallback done_callback)
+      : BaseQuotaAllocationTask(quota_request_size),
+        controller_(controller),
+        pending_items_(std::move(pending_items)),
+        done_callback_(done_callback),
+        ptr_factory_(this) {}
+
+  ~MemoryQuotaAllocationTask() override {}
+
+  void RunSuccessCallback(bool success) const {

[pwnall, 2016/09/26 13:18:08]

RunDoneCallback? RunCallback?

[dmurph, 2016/09/29 00:44:21]

Done.

+    if (success) {
+      for (const auto& item : pending_items_) {
+        item->set_state(ShareableBlobDataItem::QUOTA_GRANTED);
+      }
+    }
+    done_callback_.Run(success);

[pwnall, 2016/09/26 13:18:08]

Does the task not need to remove itself from the pending list here?

[dmurph, 2016/09/29 00:44:21]

No, we do that externally as it'll invalidate the iterators :(

+  }
+
+  void Cancel() override {

[pwnall, 2016/09/26 13:18:08]

Is it worth DCHECK-ing that Cancel() is called on the right thread?

[michaeln, 2016/09/28 01:08:03]

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.

[dmurph, 2016/09/29 00:44:22]

I'll put a comment about IO thread expectations in the header for the whole
class.

+    controller_->pending_memory_quota_total_size_ -= allocation_size();
+    // This call destorys this object.

[pwnall, 2016/09/26 13:18:09]

typo destorys -> destroys

[dmurph, 2016/09/29 00:44:22]

Done.

+    controller_->pending_memory_quota_tasks_.erase(my_list_position());
+  }
+
+  BlobMemoryController* controller_;
+  std::vector<scoped_refptr<ShareableBlobDataItem>> pending_items_;
+  MemoryQuotaRequestCallback done_callback_;
+
+  base::WeakPtrFactory<MemoryQuotaAllocationTask> ptr_factory_;
+  DISALLOW_COPY_AND_ASSIGN(MemoryQuotaAllocationTask);
+};
+
+// This task

[pwnall, 2016/09/26 13:18:09]

Eh?

[dmurph, 2016/09/29 00:44:21]

Done.

+class BlobMemoryController::FileQuotaAllocationTask
+    : public BlobMemoryController::BaseQuotaAllocationTask<
+          PendingFileQuotaTaskList> {
+ public:
+  FileQuotaAllocationTask(
+      BlobMemoryController* memory_controller,
+      std::vector<ShareableBlobDataItem*> unreserved_file_items,
+      const FileQuotaRequestCallback& done_callback)
+      : controller_(memory_controller),
+        done_callback_(done_callback),
+        ptr_factory_(this) {
+    // Get the file sizes and total size.
+    uint64_t total_size = 0;
+    GetFileSizesAndTotalSize(unreserved_file_items, &file_sizes_, &total_size);
+    DCHECK_LE(total_size, controller_->GetAvailableFileSpaceForBlobs());
+    set_allocation_size(total_size);
+
+    // Check our state and set our item states.
+    for (ShareableBlobDataItem* item : unreserved_file_items) {
+      DCHECK_EQ(ShareableBlobDataItem::QUOTA_NEEDED, item->state());
+      DCHECK_EQ(DataElement::TYPE_FILE, item->item()->type());
+      item->set_state(ShareableBlobDataItem::QUOTA_REQUESTED);
+      pending_items_.push_back(make_scoped_refptr(item));
+    }
+
+    // Increment disk usage and create our file references.
+    controller_->disk_used_ += total_size;
+    std::vector<scoped_refptr<ShareableFileReference>> file_refs;
+    for (size_t i = 0; i < file_sizes_.size(); i++) {
+      std::string file_name =

[pwnall, 2016/09/26 13:18:08]

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")

[dmurph, 2016/09/29 00:44:21]

Sounds good. I can't quite do the checks, but I can document that.

+          base::Uint64ToString(controller_->current_file_num_++);
+      file_refs.push_back(ShareableFileReference::GetOrCreate(
+          controller_->blob_storage_dir_.Append(file_name),
+          ShareableFileReference::DELETE_ON_FINAL_RELEASE,
+          controller_->file_runner_.get()));
+    }
+
+    // Send file creation task to file thread.
+    base::PostTaskAndReplyWithResult(
+        controller_->file_runner_.get(), FROM_HERE,
+        base::Bind(&CreateFiles, base::Passed(&file_refs)),
+        base::Bind(&FileQuotaAllocationTask::OnCreateFiles,
+                   ptr_factory_.GetWeakPtr()));
+    controller_->RecordTracingCounters();
+  }
+  ~FileQuotaAllocationTask() override {}
+
+  void RunSuccessCallback(bool success,

[pwnall, 2016/09/26 13:18:08]

RunDoneCallback? RunCallback?

[dmurph, 2016/09/29 00:44:21]

Done.

+                          std::vector<FileCreationInfo> file_info) const {
+    done_callback_.Run(success, std::move(file_info));
+  }
+
+  void Cancel() override {
+    controller_->disk_used_ -= allocation_size();
+    // This call destorys this object.

[pwnall, 2016/09/26 13:18:07]

typo: "destorys"

[dmurph, 2016/09/29 00:44:21]

Done.

+    controller_->pending_file_quota_tasks_.erase(my_list_position());

[pwnall, 2016/09/26 13:18:07]

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.

[dmurph, 2016/09/29 00:44:20]

Yes, this is why I documented that calling 'Cancel' will NOT call the success
callback.

+  }
+
+  void OnCreateFiles(std::vector<FileCreationInfo> result) {

[pwnall, 2016/09/26 13:18:09]

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.

[dmurph, 2016/09/29 00:44:21]

Good point about sequencing. Fixed.

+    if (result.empty()) {
+      for (uint64_t size : file_sizes_) {
+        controller_->disk_used_ -= size;
+      }
+      done_callback_.Run(false, std::vector<FileCreationInfo>());

[pwnall, 2016/09/26 13:18:08]

It seems odd that you have a method wrapping running the callback, and then you
run the callback directly here.

[dmurph, 2016/09/29 00:44:20]

removed.

+      controller_->DisableFilePaging();
+      return;
+    }
+    DCHECK_EQ(file_sizes_.size(), result.size());
+    for (size_t i = 0; i < result.size(); i++) {
+      result[i].file_reference->AddFinalReleaseCallback(
+          base::Bind(&BlobMemoryController::OnBlobFileDelete,
+                     controller_->ptr_factory_.GetWeakPtr(), file_sizes_[i]));
+    }
+    for (const auto& item : pending_items_) {
+      item->set_state(ShareableBlobDataItem::QUOTA_GRANTED);
+    }
+    done_callback_.Run(true, std::move(result));

[pwnall, 2016/09/26 13:18:09]

Again, seems odd that you're not using RunSuccessCallback here.

[dmurph, 2016/09/29 00:44:21]

Done.

+  }
+
+  BlobMemoryController* controller_;
+  std::vector<uint64_t> file_sizes_;
+  std::vector<scoped_refptr<ShareableBlobDataItem>> pending_items_;
+  scoped_refptr<base::TaskRunner> file_runner_;
+  FileQuotaRequestCallback done_callback_;
+
+  base::WeakPtrFactory<FileQuotaAllocationTask> ptr_factory_;
+  DISALLOW_COPY_AND_ASSIGN(FileQuotaAllocationTask);
+};
+
+FileCreationInfo::FileCreationInfo() = default;
+FileCreationInfo::~FileCreationInfo() = default;
+FileCreationInfo::FileCreationInfo(FileCreationInfo&&) = default;
+FileCreationInfo& FileCreationInfo::operator=(FileCreationInfo&&) = default;
+
+BlobMemoryController::BlobMemoryController()
+    : recent_item_cache_(
+          base::MRUCache<uint64_t, ShareableBlobDataItem*>::NO_AUTO_EVICT),
+      ptr_factory_(this) {}
+
+BlobMemoryController::~BlobMemoryController() {}
+
+void BlobMemoryController::EnableFilePaging(
+    const base::FilePath& storage_directory,
+    scoped_refptr<base::TaskRunner> file_runner) {
+  DCHECK(!storage_directory.empty());

[pwnall, 2016/09/26 13:18:07]

Should this also DCHECK that file paging was disabled?

[dmurph, 2016/09/29 00:44:22]

Done.

+  file_runner_ = std::move(file_runner);
+  blob_storage_dir_ = storage_directory;
+  file_paging_enabled_ = true;
+}
+
+void BlobMemoryController::DisableFilePaging() {
+  file_paging_enabled_ = false;
+  blob_memory_used_ += in_flight_memory_used_;
+  in_flight_memory_used_ = 0;
+  items_saving_to_disk_.clear();
+  for (const auto& memory_request : pending_memory_quota_tasks_) {
+    memory_request->RunSuccessCallback(false);

[michaeln, 2016/09/28 01:08:03]

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)

[dmurph, 2016/09/29 00:44:21]

Done.

+  }
+  for (const auto& file_request : pending_file_quota_tasks_) {
+    DCHECK_GE(disk_used_, file_request->allocation_size());
+    disk_used_ -= file_request->allocation_size();
+    file_request->RunSuccessCallback(false, std::vector<FileCreationInfo>());
+  }

[michaeln, 2016/09/28 01:08:03]

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

[dmurph, 2016/09/29 00:44:20]

Done.

+  pending_pagings_ = 0;
+  pending_memory_quota_total_size_ = 0;
+  pending_memory_quota_tasks_.clear();
+  pending_file_quota_tasks_.clear();
+  recent_item_cache_.Clear();
+  recent_item_cache_bytes_ = 0;
+  file_runner_ = nullptr;
+  RecordTracingCounters();
+}
+
+BlobMemoryController::Strategy BlobMemoryController::DetermineStrategy(
+    size_t preemptive_transported_bytes,
+    uint64_t total_transportation_bytes) const {
+  if (total_transportation_bytes == 0) {
+    return Strategy::NONE_NEEDED;
+  }
+  if (!CanReserveQuota(total_transportation_bytes)) {
+    return Strategy::TOO_LARGE;
+  }
+  // Handle the case where we have all the bytes preemptively transported, and
+  // we can also fit them.
+  if (preemptive_transported_bytes == total_transportation_bytes &&
+      pending_memory_quota_tasks_.empty() &&
+      preemptive_transported_bytes < GetAvailableMemoryForBlobs()) {
+    return Strategy::NONE_NEEDED;
+  }
+  if (file_paging_enabled_ &&
+      (total_transportation_bytes > limits_.max_blob_in_memory_space)) {
+    return Strategy::FILE;
+  }
+  if (total_transportation_bytes > limits_.max_ipc_memory_size) {
+    return Strategy::SHARED_MEMORY;
+  }
+  return Strategy::IPC;

[michaeln, 2016/09/28 01:08:04]

short and sweet, thnx for neutering the comments :)

+}
+
+bool BlobMemoryController::CanReserveQuota(uint64_t size) const {
+  // We check each size independently as a blob can't be constructed in both
+  // disk and memory.
+  return size <= GetAvailableMemoryForBlobs() ||
+         size <= GetAvailableFileSpaceForBlobs();
+}
+
+base::WeakPtr<QuotaAllocationTask> BlobMemoryController::ReserveMemoryQuota(
+    std::vector<ShareableBlobDataItem*> unreserved_memory_items,
+    const MemoryQuotaRequestCallback& done_callback) {
+  uint64_t total_bytes_needed = 0;
+  for (ShareableBlobDataItem* item : unreserved_memory_items) {
+    DCHECK_EQ(ShareableBlobDataItem::QUOTA_NEEDED, item->state());
+    DCHECK(item->item()->type() == DataElement::TYPE_BYTES_DESCRIPTION ||
+           item->item()->type() == DataElement::TYPE_BYTES);

[michaeln, 2016/09/28 01:08:04]

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)

[dmurph, 2016/09/29 00:44:21]

Not possible, but good to DCHECK everywhere just in case.

+    total_bytes_needed += item->item()->length();
+    item->set_state(ShareableBlobDataItem::QUOTA_REQUESTED);
+  }
+
+  if (total_bytes_needed == 0) {
+    done_callback.Run(true);
+    return base::WeakPtr<QuotaAllocationTask>();
+  }
+
+  if (!file_paging_enabled_) {

[pwnall, 2016/09/26 13:18:09]

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.

[dmurph, 2016/09/29 00:44:21]

Nice idea, done.

+    blob_memory_used_ += total_bytes_needed;
+    for (ShareableBlobDataItem* item : unreserved_memory_items) {
+      item->set_state(ShareableBlobDataItem::QUOTA_GRANTED);
+    }
+    done_callback.Run(true);
+    return base::WeakPtr<QuotaAllocationTask>();
+  }
+
+  // If we're currently waiting for blobs to page already, then we add
+  // ourselves to the end of the queue. Once paging is complete, we'll schedule
+  // more paging for any more pending blobs.
+  if (!pending_memory_quota_tasks_.empty()) {
+    std::vector<scoped_refptr<ShareableBlobDataItem>> item_handles =
+        WrapInRefPtrs(unreserved_memory_items);
+
+    pending_memory_quota_total_size_ += total_bytes_needed;
+    pending_memory_quota_tasks_.push_back(
+        base::MakeUnique<MemoryQuotaAllocationTask>(this, total_bytes_needed,
+                                                    std::move(item_handles),
+                                                    done_callback));
+    pending_memory_quota_tasks_.back()->set_my_list_position(
+        --pending_memory_quota_tasks_.end());
+
+    return pending_memory_quota_tasks_.back()->ptr_factory_.GetWeakPtr();
+  }
+
+  // Store right away if we can.
+  if (total_bytes_needed <= GetAvailableMemoryForBlobs()) {
+    // If we're past our blob memory limit, then schedule our paging.
+    blob_memory_used_ += total_bytes_needed;
+    for (ShareableBlobDataItem* item : unreserved_memory_items) {
+      item->set_state(ShareableBlobDataItem::QUOTA_GRANTED);
+    }
+    MaybeSchedulePagingUntilSystemHealthy();
+    done_callback.Run(true);
+    return base::WeakPtr<QuotaAllocationTask>();
+  }
+
+  // This means we're too big for memory.

[pwnall, 2016/09/26 13:18:07]

"This means" appears to be redundant here.

[dmurph, 2016/09/29 00:44:22]

Done.

+  DCHECK(pending_memory_quota_tasks_.empty());
+  DCHECK_EQ(0u, pending_memory_quota_total_size_);
+  std::vector<scoped_refptr<ShareableBlobDataItem>> item_handles =
+      WrapInRefPtrs(unreserved_memory_items);
+
+  pending_memory_quota_total_size_ = total_bytes_needed;
+  pending_memory_quota_tasks_.push_back(
+      base::MakeUnique<MemoryQuotaAllocationTask>(
+          this, total_bytes_needed, std::move(item_handles), done_callback));
+  pending_memory_quota_tasks_.back()->set_my_list_position(
+      --pending_memory_quota_tasks_.end());
+
+  auto weak_ptr = pending_memory_quota_tasks_.back()->ptr_factory_.GetWeakPtr();
+  MaybeSchedulePagingUntilSystemHealthy();
+  return weak_ptr;
+}
+
+base::WeakPtr<QuotaAllocationTask> BlobMemoryController::ReserveFileQuota(
+    std::vector<ShareableBlobDataItem*> unreserved_file_items,
+    const FileQuotaRequestCallback& done_callback) {
+  pending_file_quota_tasks_.push_back(base::MakeUnique<FileQuotaAllocationTask>(
+      this, std::move(unreserved_file_items), done_callback));
+  pending_file_quota_tasks_.back()->set_my_list_position(
+      --pending_file_quota_tasks_.end());
+  return pending_file_quota_tasks_.back()->ptr_factory_.GetWeakPtr();
+}
+
+void BlobMemoryController::MaybeFreeMemoryQuotaForItems(
+    const std::vector<scoped_refptr<ShareableBlobDataItem>>& items) {
+  uint64_t memory_to_be_freed = 0;
+  std::unordered_set<uint64_t> visited_items;
+  for (const scoped_refptr<ShareableBlobDataItem>& item : items) {
+    switch (item->state()) {
+      case ShareableBlobDataItem::QUOTA_NEEDED:
+      case ShareableBlobDataItem::QUOTA_REQUESTED:
+      case ShareableBlobDataItem::POPULATED_WITHOUT_QUOTA:
+        continue;
+      case ShareableBlobDataItem::QUOTA_GRANTED:
+      case ShareableBlobDataItem::POPULATED_WITH_QUOTA:
+        break;
+    }
+    // We only care about bytes items that don't have blobs referencing them,
+    // and we remove duplicates.
+    DataElement::Type type = item->item()->type();
+    if (!item->referencing_blobs().empty() ||
+        (type != DataElement::TYPE_BYTES &&
+         type != DataElement::TYPE_BYTES_DESCRIPTION) ||
+        base::ContainsKey(visited_items, item->item_id()))
+      continue;
+    visited_items.insert(item->item_id());
+    memory_to_be_freed += item->item()->length();
+  }
+  if (memory_to_be_freed != 0) {
+    DCHECK_GE(blob_memory_used_, memory_to_be_freed);
+    blob_memory_used_ -= memory_to_be_freed;
+    MaybeGrantPendingQuotaRequests();
+  }
+}
+
+void BlobMemoryController::ForceFreeMemoryQuotaForItem(
+    const scoped_refptr<ShareableBlobDataItem>& item) {
+  CHECK_EQ(item->state(), ShareableBlobDataItem::QUOTA_GRANTED);
+  CHECK_EQ(item->item()->type(), DataElement::TYPE_BYTES_DESCRIPTION);
+  blob_memory_used_ -= item->item()->length();
+  MaybeGrantPendingQuotaRequests();
+}
+
+void BlobMemoryController::UpdateBlobItemInRecents(

[michaeln, 2016/09/28 01:08:04]

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)

[dmurph, 2016/09/29 00:44:21]

Good idea, thanks.

+    ShareableBlobDataItem* item) {
+  DCHECK_EQ(DataElement::TYPE_BYTES, item->item()->type());
+  DCHECK_EQ(ShareableBlobDataItem::POPULATED_WITH_QUOTA, item->state());
+  // We don't want to re-add the item if we're currently paging it to disk.
+  if (items_saving_to_disk_.find(item->item_id()) !=
+      items_saving_to_disk_.end())
+    return;
+  auto iterator = recent_item_cache_.Get(item->item_id());
+  if (iterator == recent_item_cache_.end()) {
+    recent_item_cache_bytes_ += static_cast<size_t>(item->item()->length());
+    recent_item_cache_.Put(item->item_id(), item);
+    MaybeSchedulePagingUntilSystemHealthy();
+  }
+}
+
+void BlobMemoryController::RemoveBlobItemInRecents(
+    const ShareableBlobDataItem& item) {
+  auto iterator = recent_item_cache_.Get(item.item_id());
+  if (iterator != recent_item_cache_.end()) {
+    size_t size = static_cast<size_t>(item.item()->length());
+    DCHECK_GE(recent_item_cache_bytes_, size);
+    recent_item_cache_bytes_ -= size;
+    recent_item_cache_.Erase(iterator);
+  }
+}
+
+void BlobMemoryController::MaybeGrantPendingQuotaRequests() {
+  while (!pending_memory_quota_tasks_.empty() &&
+         limits_.max_blob_in_memory_space - blob_memory_used_ >=
+             pending_memory_quota_tasks_.front()->allocation_size()) {
+    std::unique_ptr<MemoryQuotaAllocationTask> memory_task =
+        std::move(pending_memory_quota_tasks_.front());
+    pending_memory_quota_tasks_.pop_front();
+    size_t request_size = memory_task->allocation_size();
+    pending_memory_quota_total_size_ -= request_size;
+    blob_memory_used_ += request_size;
+    memory_task->RunSuccessCallback(true);
+  }
+  RecordTracingCounters();
+}
+
+void BlobMemoryController::MaybeSchedulePagingUntilSystemHealthy() {
+  // Don't do paging when others are happening, as we don't change our

[pwnall, 2016/09/26 13:18:09]

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.

[dmurph, 2016/09/29 00:44:22]

Done.

+  // blobs_waiting_for_paging_size_ value until after the paging files have
+  // been written.
+  if (pending_pagings_ != 0 || !file_paging_enabled_)
+    return;
+
+  // We try to page items to disk until our current system size + requested
+  // memory is below our size limit.
+  while (pending_memory_quota_total_size_ + blob_memory_used_ >
+         limits_.max_blob_in_memory_space) {
+    // We only page when we have enough items to fill a while page file.

[pwnall, 2016/09/26 13:18:07]

while -> whole

[dmurph, 2016/09/29 00:44:21]

Done.

+    if (recent_item_cache_bytes_ < limits_.min_page_file_size)
+      break;
+    DCHECK_LE(limits_.min_page_file_size,
+              static_cast<uint64_t>(blob_memory_used_));
+    size_t total_items_size = 0;
+    std::unique_ptr<std::vector<scoped_refptr<ShareableBlobDataItem>>>
+        items_for_disk(new std::vector<scoped_refptr<ShareableBlobDataItem>>());
+    // Collect our items.

[pwnall, 2016/09/26 13:18:09]

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);

[dmurph, 2016/09/29 00:44:20]

Done.

+    while (total_items_size < limits_.min_page_file_size &&
+           !recent_item_cache_.empty()) {
+      auto iterator = --recent_item_cache_.end();
+      ShareableBlobDataItem* item = iterator->second;
+      DCHECK(item);
+      DCHECK_EQ(item->item()->type(), DataElement::TYPE_BYTES);
+      recent_item_cache_.Erase(iterator);
+      recent_item_cache_bytes_ -= static_cast<size_t>(item->item()->length());
+      items_saving_to_disk_.insert(item->item_id());
+      size_t size = base::checked_cast<size_t>(item->item()->length());
+      total_items_size += size;
+      items_for_disk->push_back(make_scoped_refptr(item));
+    }
+    if (total_items_size == 0)
+      break;
+
+    // Update our bookkeeping.
+    pending_pagings_++;
+    disk_used_ += total_items_size;
+    DCHECK_GE(blob_memory_used_, total_items_size);
+    blob_memory_used_ -= total_items_size;
+    in_flight_memory_used_ += total_items_size;
+    std::string file_name = base::Uint64ToString(current_file_num_++);
+    // Create our file reference.
+    scoped_refptr<ShareableFileReference> file_ref =
+        ShareableFileReference::GetOrCreate(
+            blob_storage_dir_.Append(file_name),
+            ShareableFileReference::DELETE_ON_FINAL_RELEASE,
+            file_runner_.get());
+    // Add the release callback so we decrement our disk usage on file deletion.
+    file_ref->AddFinalReleaseCallback(
+        base::Bind(&BlobMemoryController::OnBlobFileDelete,
+                   ptr_factory_.GetWeakPtr(), total_items_size));
+    // Post the file writing task.
+    base::PostTaskAndReplyWithResult(
+        file_runner_.get(), FROM_HERE,
+        base::Bind(&WriteItemsToFile, items_for_disk.get(), total_items_size,
+                   base::Passed(&file_ref)),
+        base::Bind(&BlobMemoryController::OnPagingComplete,
+                   ptr_factory_.GetWeakPtr(), base::Passed(&items_for_disk),
+                   total_items_size));
+  }
+  RecordTracingCounters();
+}
+
+void BlobMemoryController::OnPagingComplete(

[pwnall, 2016/09/26 13:18:08]

Again, how about Paging -> Eviction?

[dmurph, 2016/09/29 00:44:21]

Done.

+    std::unique_ptr<std::vector<scoped_refptr<ShareableBlobDataItem>>> items,
+    size_t total_items_size,
+    FileCreationInfo result) {
+  if (!file_paging_enabled_)
+    return;
+  if (result.error != File::FILE_OK) {
+    UMA_HISTOGRAM_ENUMERATION("Storage.Blob.PagingError", -result.error,
+                              -File::FILE_ERROR_MAX);
+    disk_used_ -= total_items_size;
+    DisableFilePaging();
+    return;
+  }
+  DCHECK_LT(0u, pending_pagings_);
+  pending_pagings_--;
+
+  // Switch from the data backing item to a new file backing item.

[pwnall, 2016/09/26 13:18:08]

data-backed items and file-backed items?

[dmurph, 2016/09/29 00:44:21]

Done.

+  uint64_t offset = 0;
+  for (const scoped_refptr<ShareableBlobDataItem>& shareable_item :
+       *items.get()) {
+    scoped_refptr<BlobDataItem> new_item(new BlobDataItem(
+        base::WrapUnique(new DataElement()), result.file_reference));
+    new_item->data_element_ptr()->SetToFilePathRange(
+        result.file_reference->path(), offset, shareable_item->item()->length(),
+        result.last_modified);
+    *(shareable_item->item_mutable()) = new_item;

[pwnall, 2016/09/26 13:18:09]

Why is this better than a set_item()?

[dmurph, 2016/09/29 00:44:22]

Done.

+    items_saving_to_disk_.erase(shareable_item->item_id());
+    offset += shareable_item->item()->length();
+  }
+  in_flight_memory_used_ -= total_items_size;
+
+  // We want callback on blobs up to the amount we've freed.
+  MaybeGrantPendingQuotaRequests();
+
+  // If we still have more blobs waiting and we're not waiting on more paging
+  // operations, schedule more.
+  MaybeSchedulePagingUntilSystemHealthy();
+}
+
+void BlobMemoryController::RecordTracingCounters() const {
+  TRACE_COUNTER2("Blob", "MemoryUsage", "RegularStorage", blob_memory_used_,
+                 "InFlightToDisk", in_flight_memory_used_);
+  TRACE_COUNTER1("Blob", "DiskUsage", disk_used_);
+  TRACE_COUNTER1("Blob", "TranfersPendingOnDisk",
+                 pending_memory_quota_tasks_.size());
+  TRACE_COUNTER1("Blob", "TranfersBytesPendingOnDisk",
+                 pending_memory_quota_total_size_);
+}
+
+size_t BlobMemoryController::GetAvailableMemoryForBlobs() const {
+  if (limits_.total_memory_space() < memory_usage())
+    return 0;
+  return limits_.total_memory_space() - memory_usage();
+}
+
+uint64_t BlobMemoryController::GetAvailableFileSpaceForBlobs() const {
+  if (!file_paging_enabled_)
+    return 0;
+  return limits_.max_blob_disk_space - disk_used_ -
+         pending_memory_quota_total_size_;
+}
+
+void BlobMemoryController::OnBlobFileDelete(uint64_t size,
+                                            const base::FilePath& path) {
+  DCHECK_LE(size, disk_used_);
+  disk_used_ -= size;
+}
+
+}  // namespace storage