[BlobStorage] BlobMemoryController & tests

storage/browser/blob/blob_memory_controller.h

Index: storage/browser/blob/blob_memory_controller.h
diff --git a/storage/browser/blob/blob_memory_controller.h b/storage/browser/blob/blob_memory_controller.h
new file mode 100644
index 0000000000000000000000000000000000000000..fe0c2263e6bf904da16d3254e2bb6bcc024c192f
--- /dev/null
+++ b/storage/browser/blob/blob_memory_controller.h
@@ -0,0 +1,244 @@
+// 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.
+
+#ifndef STORAGE_BROWSER_BLOB_BLOB_MEMORY_CONTROLLER_H_
+#define STORAGE_BROWSER_BLOB_BLOB_MEMORY_CONTROLLER_H_
+
+#include <stdint.h>
+
+#include <list>
+#include <map>
+#include <memory>
+#include <string>
+#include <unordered_map>
+#include <unordered_set>
+#include <utility>
+#include <vector>
+
+#include "base/callback.h"
+#include "base/containers/mru_cache.h"
+#include "base/files/file.h"
+#include "base/files/file_path.h"
+#include "base/macros.h"
+#include "base/memory/ref_counted.h"
+#include "base/memory/weak_ptr.h"
+#include "base/optional.h"
+#include "base/time/time.h"
+#include "storage/browser/storage_browser_export.h"
+#include "storage/common/blob_storage/blob_storage_constants.h"
+
+namespace base {
+class TaskRunner;
+}
+
+namespace storage {
+class DataElement;
+class ShareableBlobDataItem;
+class ShareableFileReference;
+
+// This class is responsible for file & memory quota bookkeeping, creating files

[pwnall, 2016/09/21 22:56:58]

It might be easier to reason about what's in this class if you can come up with
one phrase that describes its responsibility. This paragraph could be a bullet
list, which makes me wonder whether the class violates SRP.

It seems to me that the class' main responsibility is deciding where blob
contents is stored. If this is true, it follows that the memory controller would
decide how a blob's items are transferred from the renderer to the browser, and
when/which items move between disk and RAM. So, the bullet list gets to become a
2nd paragraph.

WDYT?

[dmurph, 2016/09/23 20:15:14]

MAkes sense. I tried to fix my language, let me know if it makes sense.

+// and paging old blob items to disk, and keeping track of an LRU of blob items.
+//
+// See ReserveMemoryQuotaForItems and ReserveFileQuotaForItems for reserving
+// memory and file quota respectively, and use MaybeFreeQuotaForItems to free
+// quota.
+//
+// Use UpdateBlobItemInRecents & RemoveBlobItemInRecents for modifying the LRU
+// of blob items.
+class STORAGE_EXPORT BlobMemoryController {
+ public:
+  enum class Strategy {
+    // We don't have enough memory for this blob.
+    TOO_LARGE,
+    // There isn't any memory that needs transporting.
+    NONE_NEEDED,
+    // Transportation strategies.
+    IPC,
+    SHARED_MEMORY,
+    FILE
+  };
+
+  struct FileCreationInfo {
+    FileCreationInfo();
+    ~FileCreationInfo();
+    FileCreationInfo(FileCreationInfo&& other);
+    FileCreationInfo& operator=(FileCreationInfo&&);
+
+    base::File::Error error = base::File::FILE_ERROR_FAILED;
+    scoped_refptr<ShareableFileReference> file_reference;
+    base::File file;
+    base::Time last_modified;
+  };
+
+  // The bool argument is if we were able to successfuly receive quota.

[pwnall, 2016/09/21 22:56:59]

is if -> is true if?

Please feel free to discard this feedback (and let me know) if this shorthand is
common in the codebase.

[dmurph, 2016/09/23 20:15:14]

Fixed.

+  using FileQuotaRequestCallback =
+      base::Callback<void(bool, std::vector<FileCreationInfo>)>;
+  using PendingFileQuotaRequest = uint64_t;
+  static const uint64_t kInvalidFileQuotaRequest = 0;
+
+  // The bool argument is if we were able to successfuly receive quota.
+  using MemoryQuotaRequestCallback = base::Callback<void(bool)>;
+  using PendingBlobConstructionList =
+      std::list<std::pair<size_t, MemoryQuotaRequestCallback>>;
+  using PendingMemoryQuotaRequest = PendingBlobConstructionList::iterator;
+  PendingMemoryQuotaRequest GetInvalidMemoryQuotaRequest();
+
+  BlobMemoryController();
+  virtual ~BlobMemoryController();
+
+  void EnableFilePaging(const base::FilePath& storage_directory,
+                        scoped_refptr<base::TaskRunner> file_runner);
+
+  // Disables the disk. This cancels all pending file creations and paging
+  // operations.
+  void DisableFilePaging();
+
+  bool file_paging_enabled() const { return file_paging_enabled_; }
+
+  // Returns the strategy the transportation layer should use to transport the
+  // given memory. |preemptive_transported_bytes| are the number of transport
+  // bytes that are already populated for us, so we don't haved to request them
+  // from the renderer.
+  Strategy DetermineStrategy(size_t preemptive_transported_bytes,
+                             uint64_t total_transportation_bytes) const;
+
+  // Checks to see if we can reserve quota (disk or memory) for the given size.
+  bool CanReserveQuota(uint64_t size) const;
+
+  // This reserves quota for the given |unreserved_memory_items|. The items must

[pwnall, 2016/09/21 22:56:59]

"This" seems unnecessary here, and diverges from the cadence established above.
Also, in method comments, I'd expect "this" to refer to the C++ this, e.g. "this
memory controller".

If you agree, please also update the comments below.

[dmurph, 2016/09/23 20:15:14]

Done.

+  // be bytes items in QUOTA_NEEDED state, which we change to QUOTA_REQUESTED.
+  // After we reserve memory quota we change their state to QUOTA_GRANTED and
+  // call |success_callback|.  This can happen synchronously.
+  // NOTE: We don't inspect quota limits and assume the user checked
+  //       CanReserveQuota before calling this.

[pwnall, 2016/09/21 22:56:58]

Would it make sense to have a stronger requirement here, like "DetermineStrategy
must have indicated that the shared memory strategy should be used for this
blob"?

I'm specifically troubled by the fact that the CanReserveQuota() implementation
returns true if the blob fits into memory _or_ into file storage. So It seems
like it's possible to have that memory return true, but still not have enough
quota here.

+  // Returns a value if we're async for use with CancelMemoryQuotaReservation.
+  PendingMemoryQuotaRequest ReserveMemoryQuota(
+      std::vector<ShareableBlobDataItem*> unreserved_memory_items,
+      const MemoryQuotaRequestCallback& success_callback);
+
+  void CancelMemoryQuotaReservation(const PendingMemoryQuotaRequest& entry);
+
+  // This reserves quota for the given |unreserved_file_items|. The items must
+  // be temporary file items (BlobDataBuilder::IsTemporaryFileItem returns true)
+  // in QUOTA_NEEDED state, which we change to QUOTA_REQUESTED. After we reserve
+  // file quota we change their state to QUOTA_GRANTED and call
+  // |success_callback|.
+  // NOTE: We don't inspect quota limits and assume the user checked

[pwnall, 2016/09/21 22:56:59]

nit: not sure if "user" is common, but I think "caller" is a bit more precise
here

+  //       CanReserveQuota before calling this.
+  // The return value can be used with CancelFileQuotaReservation to cancel.
+  PendingFileQuotaRequest ReserveFileQuota(
+      std::vector<ShareableBlobDataItem*> unreserved_file_items,
+      const FileQuotaRequestCallback& success_callback);
+
+  void CancelFileQuotaReservation(const PendingFileQuotaRequest& entry);
+
+  // This frees quota for items that don't have any blob references.
+  void MaybeFreeQuotaForItems(
+      const std::vector<scoped_refptr<ShareableBlobDataItem>>& items);
+
+  // This is used to release quota for an item that was supposed to be filled
+  // with a data copy from an item in another blob, but by the time the copy
+  // was meant to be performed the original item had been paged to file. This
+  // functionally just decrements the memory by the item length, but we do
+  // extra checks to make sure our state is correct.
+  void FreeQuotaForPagedItemReference(
+      const scoped_refptr<ShareableBlobDataItem>& item);
+
+  // This adds or updates a bytes item in our LRU table used for paging items

[pwnall, 2016/09/21 22:56:58]

Who is responsible for updating the LRU cache/list?

+  // to disk. The item must have state POPULATED_WITH_QUOTA.
+  void UpdateBlobItemInRecents(ShareableBlobDataItem* item);
+  void RemoveBlobItemInRecents(const ShareableBlobDataItem& item);
+
+  size_t memory_usage() const {
+    return blob_memory_used_ + in_flight_memory_used_;
+  }
+  uint64_t disk_usage() const { return disk_used_; }
+
+  const BlobStorageLimits& limits() const { return limits_; }
+  void SetLimitsForTesting(BlobStorageLimits limits) { limits_ = limits; }
+
+ private:
+  // Must be class method because state_ is private in ShareableBlobDataItem.
+  void SetStateQuotaGrantedAndCallback(
+      std::vector<scoped_refptr<ShareableBlobDataItem>> items,
+      const BlobMemoryController::MemoryQuotaRequestCallback& final_callback,
+      bool success);
+
+  // Called when we've finished creating files for ReserveFileQuota. We make

[pwnall, 2016/09/21 22:56:59]

Everything after the first sentence seems like an implementation detail, which
makes me think it belongs to the body of the function. AFAIK, the comments in
the header should indicate how one would use the function.

[dmurph, 2016/09/23 20:15:14]

removed.

+  // sure to handle the case where the user had cancelled the operation, and if

[pwnall, 2016/09/21 22:56:58]

I find "user" mildly confusing here. I'm guessing it doesn't mean the browser's
user. "the case where the quota request has been canceled" seems a tiny bit
easier to understand.

+  // successful we add the callback to the file reference to decrement our disk
+  // usage on destruction. Then we foward the files and result to the
+  // |file_callback|.
+  void OnCreateFiles(
+      std::vector<uint64_t> file_sizes,
+      std::vector<scoped_refptr<ShareableBlobDataItem>> pending_items,
+      uint64_t disk_quota_entry,
+      const FileQuotaRequestCallback& file_callback,
+      std::vector<FileCreationInfo> result);
+
+  void MaybeGrantPendingQuotaRequests();
+
+  // We schedule paging until our memory usage is below our memory limit. We use

[pwnall, 2016/09/21 22:56:58]

"We" seems redundant. The first sentence seems to re-state the method name. The
following sentences appear to describe the implementation (and thus belong in
the definition).

[dmurph, 2016/09/23 20:15:14]

Done.

+  // the lru table to find old items, and we combine them until we reach the
+  // min_page_file_size(), and don't schedule paging until we have at least that
+  // amount of memory to save to disk.
+  void MaybeSchedulePagingUntilSystemHealthy();
+
+  // Called when we've completed paging a list of items to disk. This is where
+  // we swap the bytes items for file items, and and update our bookkeeping.
+  void OnPagingComplete(
+      std::unique_ptr<std::vector<scoped_refptr<ShareableBlobDataItem>>> items,
+      size_t total_items_size,
+      FileCreationInfo result);
+
+  void RecordTracingCounters();
+
+  size_t GetAvailableMemoryForBlobs() const;
+  uint64_t GetAvailableFileSpaceForBlobs() const;
+
+  // This is registered as a callback for file deletions on the file reference
+  // of our paging files. We decrement the disk space used.
+  void OnBlobFileDelete(uint64_t size, const base::FilePath& path);
+
+  BlobStorageLimits limits_;
+
+  // Memory bookkeeping. These numbers are all disjoint.
+  // This is the amount of memory we're using for blobs in RAM.
+  size_t blob_memory_used_ = 0;
+  // This is memory we're temporarily using while we try to write blob items to
+  // disk.
+  size_t in_flight_memory_used_ = 0;
+  // This is the amount of memory we're using on disk.
+  uint64_t disk_used_ = 0;
+
+  size_t pending_pagings_ = 0;
+  PendingBlobConstructionList blobs_waiting_for_paging_;
+  size_t blobs_waiting_for_paging_size_ = 0;
+
+  // We use the same data storage as above to keep the PendingEntry API
+  // consistent.
+  uint64_t curr_disk_save_entry_ = 0;
+  std::unordered_map<uint64_t, uint64_t> pending_file_request_sizes_;
+
+  scoped_refptr<base::TaskRunner> file_runner_;
+
+  bool file_paging_enabled_ = false;
+  base::FilePath blob_storage_dir_;
+  uint64_t current_file_num_ = 0;
+
+  // Lifetime of the ShareableBlobDataItem objects is handled externally in the
+  // BlobStorageContext class.
+  base::MRUCache<uint64_t, ShareableBlobDataItem*> recent_item_cache_;
+  size_t recent_item_cache_bytes_ = 0;
+  // We need to keep track of items currently being paged to disk so that if
+  // another blob successfully grabs a ref, we can prevent it from adding the
+  // item to the recent_item_cache_ above.
+  std::unordered_set<uint64_t> items_saving_to_disk_;
+
+  base::WeakPtrFactory<BlobMemoryController> ptr_factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(BlobMemoryController);
+};
+}  // namespace storage
+#endif  // STORAGE_BROWSER_BLOB_BLOB_MEMORY_CONTROLLER_H_