[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..cc8d0ab3a7633103ac5bc674bbf5ce0c891dcb05
--- /dev/null
+++ b/storage/browser/blob/blob_memory_controller.h
@@ -0,0 +1,224 @@
+// 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's main responsability is deciding how blob data gets stored.
+// This encompasses:
+// * Keeping track of memory & file quota,
+// * How to transport the blob data from the renderer (DetermineStrategy),
+// * Allocating memory & file quota (ReserveMemoryQuota, ReserveFileQuota)
+// * Freeing quota (MaybeFreeQuotaForItems)
+// * Paging memory quota to disk when we're nearing our memory limit, and
+// * Maintaining an LRU of memory items to choose candidates to page to disk
+//   (UpdateBlobItemInRecents, RemoveBlobItemInRecents).
+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 true if we successfully received memory quota.
+  using MemoryQuotaRequestCallback = base::Callback<void(bool)>;
+  // The bool argument is true if we successfully received file quota, and the
+  // vector argument provides the file info.
+  using FileQuotaRequestCallback =
+      base::Callback<void(bool, std::vector<FileCreationInfo>)>;
+
+  class QuotaAllocationTask {

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

thnx for adding this abstraction

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

Acknowledged.

+   public:
+    virtual ~QuotaAllocationTask();

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

looks like the caller should never delete these, if thats right, it'd be good to
make the dtor private

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

Done. Making protected so subclasses can call it, as well as unique_ptr.

+    // Operation is cancelled and the callback will not be called. This object
+    // will be destroyed by this call.
+    virtual void Cancel() = 0;
+  };
+
+  BlobMemoryController();
+  virtual ~BlobMemoryController();
+
+  void EnableFilePaging(const base::FilePath& storage_directory,

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

I mentioned this on the larger cl too, but it got lost in the noise. I think
it'd be an improvement if this class internally managed the creation of the
backing directory. The class could lazily create the dir on demand as the amount
of storage being called for increased. I see two runtime benefits to that (plus
it shrinks the public interface a little).

1. The dir may not have to be created at all depending on blob usage. Compared
to always having to incur file system interactions during browser startup.

2. Consider the case when the very first request to reserve quota occurs prior
to EnableFilePaging being called, and its too large to fit in memory. With the
interface as it is, the controller would have to deny the requests. If the class
managed the directory internally, it could asyncly setup the dir as part of
satisfying that large request.

Wdyt?

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

Sounds good, I put these into the constructor, and to have paging disabled then
you put a nullptr as the task runner.

+                        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;
+
+  // Reserves quota for the given |unreserved_memory_items|. The items must 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.
+  // Returns a task handle if the request is asynchronous for cancellation.
+  // NOTE: We don't inspect quota limits and assume the user checked
+  //       CanReserveQuota before calling this.
+  base::WeakPtr<QuotaAllocationTask> ReserveMemoryQuota(
+      std::vector<ShareableBlobDataItem*> unreserved_memory_items,
+      const MemoryQuotaRequestCallback& success_callback);
+
+  // 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|.
+  // Returns a task handle for cancellation.
+  // NOTE: We don't inspect quota limits and assume the user checked
+  //       CanReserveQuota before calling this.
+  base::WeakPtr<QuotaAllocationTask> ReserveFileQuota(
+      std::vector<ShareableBlobDataItem*> unreserved_file_items,

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

should these be vectors of scoped_refptrs?

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

Hmm.. Maybe we should, as we grab a reference anyways. Does that mean we should
make the above method match? That one usually won't be async, so the refptrs
aren't usually needed (and then we grab one if we need the async version).

+      const FileQuotaRequestCallback& success_callback);
+
+  // This frees quota for items that don't have any blob references.

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

the comment confused me a little, i think you mean "for items that are not
referenced by any blobs"

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

Done.

+  void MaybeFreeMemoryQuotaForItems(
+      const std::vector<scoped_refptr<ShareableBlobDataItem>>& items);
+  // Forcibly frees memory quota for the given item.
+  void ForceFreeMemoryQuotaForItem(
+      const scoped_refptr<ShareableBlobDataItem>& item);
+
+  // This adds or updates a bytes item in our LRU table used for paging items
+  // to disk. The item must have state POPULATED_WITH_QUOTA.
+  void UpdateBlobItemInRecents(ShareableBlobDataItem* item);
+  void RemoveBlobItemInRecents(const ShareableBlobDataItem& item);

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

is the remove call needed or could this happen internally as an artifact of the
call to Free stuff?

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

Yes it can! woo!

+
+  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:
+  template <typename T>
+  class BaseQuotaAllocationTask;
+  class FileQuotaAllocationTask;
+  class MemoryQuotaAllocationTask;
+
+  using PendingMemoryQuotaTaskList =
+      std::list<std::unique_ptr<MemoryQuotaAllocationTask>>;
+  using PendingFileQuotaTaskList =
+      std::list<std::unique_ptr<FileQuotaAllocationTask>>;
+
+  void MaybeGrantPendingQuotaRequests();
+
+  // Schedule paging until our memory usage is below our memory limit.
+  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);
+
+  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);
+
+  void RecordTracingCounters() const;

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

From the perspective of code review / maintenance, it'd be good to document when
this is supposed to get called.

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

Done.

+
+  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;
+
+  uint64_t current_file_num_ = 0;
+
+  size_t pending_memory_quota_total_size_ = 0;
+  PendingMemoryQuotaTaskList pending_memory_quota_tasks_;
+  PendingFileQuotaTaskList pending_file_quota_tasks_;
+
+  size_t pending_pagings_ = 0;
+
+  scoped_refptr<base::TaskRunner> file_runner_;
+
+  bool file_paging_enabled_ = false;
+  base::FilePath blob_storage_dir_;
+
+  // 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_;

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

consistency, typically in storage/browser/blob, this kind of data member is
called weak_factory_

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

Done.

+
+  DISALLOW_COPY_AND_ASSIGN(BlobMemoryController);
+};
+}  // namespace storage
+#endif  // STORAGE_BROWSER_BLOB_BLOB_MEMORY_CONTROLLER_H_