blink/bindings: Keep compressed ParkableStrings when unparking, and reuse it.

When a ParkableString is unparked, keep the compressed data around, allowing to
drop the uncompressed data at no cost later. As decompression is much (>10x)
cheaper than compression and compressed data smaller, this is usually a good
tradeoff. This also allows to recover memory when a string is unparked once, and
its content is not touched again soon.

Concretely, we introduce:
- "synchronous parking" when parking is merely dropping the decompressed
  representation.
- "synchronous only" string parking in ParkableStringManager
- Drop all parkable strings when receiving a OnPurgeMemory() notification.

Also, improve and simplify testing, with fewer "friend" function and tighter
compression test.

Bug: 877044
Change-Id: I4b66ca5ddc67b2a6e49bfd1790e1788870642b04
Reviewed-on: https://chromium-review.googlesource.com/c/1329975Reviewed-by: Benoit L <lizeb@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Commit-Queue: Benoit L <lizeb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#610024}

third_party/blink/renderer/platform/bindings/DEPS

@@ -11,6 +11,7 @@ include_rules = [
     "+third_party/blink/renderer/platform/heap",
     "+third_party/blink/renderer/platform/instance_counters.h",
     "+third_party/blink/renderer/platform/instrumentation",
+    "+third_party/blink/renderer/platform/memory_coordinator.h",
     "+third_party/blink/renderer/platform/platform_export.h",
     "+third_party/blink/renderer/platform/runtime_enabled_features.h",
     "+third_party/blink/renderer/platform/scheduler",

third_party/blink/renderer/platform/bindings/parkable_string.cc

@@ -199,21 +199,32 @@ unsigned ParkableStringImpl::CharactersSizeInBytes() const {
   return length_ * (is_8bit() ? sizeof(LChar) : sizeof(UChar));
 }
 
-bool ParkableStringImpl::Park() {
+bool ParkableStringImpl::Park(ParkingMode mode) {
   AssertOnValidThread();
   MutexLocker locker(mutex_);
   DCHECK(may_be_parked_);
   if (state_ == State::kUnparked && CanParkNow()) {
-    // |string_|'s data should not be touched except in the compression task.
-    AsanPoisonString(string_);
-    // |params| keeps |this| alive until |OnParkingCompleteOnMainThread()|.
-    auto params = std::make_unique<CompressionTaskParams>(
-        this, string_.Bytes(), string_.CharactersSizeInBytes(),
-        Thread::Current()->GetTaskRunner());
-    background_scheduler::PostOnBackgroundThread(
-        FROM_HERE, CrossThreadBind(&ParkableStringImpl::CompressInBackground,
-                                   WTF::Passed(std::move(params))));
-    state_ = State::kParkingInProgress;
+    // Parking can proceed synchronously.
+    if (has_compressed_data()) {
+      RecordParkingAction(ParkingAction::kParkedInBackground);
+      state_ = State::kParked;
+      ParkableStringManager::Instance().OnParked(this, string_.Impl());
+
+      // Must unpoison the memory before releasing it.
+      AsanUnpoisonString(string_);
+      string_ = String();
+    } else if (mode == ParkingMode::kAlways) {
+      // |string_|'s data should not be touched except in the compression task.
+      AsanPoisonString(string_);
+      // |params| keeps |this| alive until |OnParkingCompleteOnMainThread()|.
+      auto params = std::make_unique<CompressionTaskParams>(
+          this, string_.Bytes(), string_.CharactersSizeInBytes(),
+          Thread::Current()->GetTaskRunner());
+      background_scheduler::PostOnBackgroundThread(
+          FROM_HERE, CrossThreadBind(&ParkableStringImpl::CompressInBackground,
+                                     WTF::Passed(std::move(params))));
+      state_ = State::kParkingInProgress;
+    }
   }
 
   return state_ == State::kParked || state_ == State::kParkingInProgress;
@@ -271,7 +282,6 @@ void ParkableStringImpl::Unpark() {
   // the string we need, nothing else to do than to abort.
   CHECK(compression::GzipUncompress(compressed_string_piece,
                                     uncompressed_string_piece));
-  compressed_ = nullptr;
   string_ = uncompressed;
   state_ = State::kUnparked;
   ParkableStringManager::Instance().OnUnparked(this, string_.Impl());
@@ -303,9 +313,9 @@ void ParkableStringImpl::OnParkingCompleteOnMainThread(
   if (CanParkNow() && compressed) {
     RecordParkingAction(ParkingAction::kParkedInBackground);
     state_ = State::kParked;
+    compressed_ = std::move(compressed);
     ParkableStringManager::Instance().OnParked(this, string_.Impl());
 
-    compressed_ = std::move(compressed);
     // Must unpoison the memory before releasing it.
     AsanUnpoisonString(string_);
     string_ = String();

third_party/blink/renderer/platform/bindings/parkable_string.h

@@ -49,6 +49,7 @@ class PLATFORM_EXPORT ParkableStringImpl final
   };
 
   enum class ParkableState { kParkable, kNotParkable };
+  enum class ParkingMode { kIfCompressedDataExists, kAlways };
 
   // Not all parkable strings can actually be parked. If |parkable| is
   // kNotParkable, then one cannot call |Park()|, and the underlying StringImpl
@@ -69,19 +70,26 @@ class PLATFORM_EXPORT ParkableStringImpl final
   unsigned CharactersSizeInBytes() const;
 
   // A parked string cannot be accessed until it has been |Unpark()|-ed.
-  // Returns true iff the string has been parked.
-  bool Park();
+  //
+  // Parking may be synchronous, and will be if compressed data is already
+  // available. If |mode| is |kIfCompressedDataExists|, then parking will always
+  // be synchronous.
+  //
+  // Returns true if the string is being parked or has been parked.
+  bool Park(ParkingMode mode);
   // Returns true iff the string can be parked. This does not mean that the
   // string can be parked now, merely that it is eligible to be parked at some
   // point.
   bool may_be_parked() const { return may_be_parked_; }
   // Returns true if the string is parked.
   bool is_parked() const;
-  // Returns the compressed size, must not be called unless the string is
-  // compressed.
+  // Returns whether synchronous parking is possible, that is the string was
+  // parked in the past.
+  bool has_compressed_data() const { return !!compressed_; }
+  // Returns the compressed size, must not be called unless the string has a
+  // compressed representation.
   size_t compressed_size() const {
-    DCHECK(is_parked());
-    DCHECK(compressed_);
+    DCHECK(has_compressed_data());
     return compressed_->size();
   }
 
@@ -124,15 +132,8 @@ class PLATFORM_EXPORT ParkableStringImpl final
 #endif
   }
 
-  FRIEND_TEST_ALL_PREFIXES(ParkableStringTest, Park);
-  FRIEND_TEST_ALL_PREFIXES(ParkableStringTest, AbortParking);
-  FRIEND_TEST_ALL_PREFIXES(ParkableStringTest, Unpark);
   FRIEND_TEST_ALL_PREFIXES(ParkableStringTest, LockUnlock);
   FRIEND_TEST_ALL_PREFIXES(ParkableStringTest, LockParkedString);
-  FRIEND_TEST_ALL_PREFIXES(ParkableStringTest, TableSimple);
-  FRIEND_TEST_ALL_PREFIXES(ParkableStringTest, TableMultiple);
-  FRIEND_TEST_ALL_PREFIXES(ParkableStringTest, AsanPoisoning);
-  FRIEND_TEST_ALL_PREFIXES(ParkableStringTest, Compression);
   DISALLOW_COPY_AND_ASSIGN(ParkableStringImpl);
 };
 

third_party/blink/renderer/platform/bindings/parkable_string_manager.cc

@@ -12,22 +12,35 @@
 #include "base/single_thread_task_runner.h"
 #include "third_party/blink/public/platform/platform.h"
 #include "third_party/blink/renderer/platform/bindings/parkable_string.h"
+#include "third_party/blink/renderer/platform/memory_coordinator.h"
 #include "third_party/blink/renderer/platform/scheduler/public/thread.h"
 #include "third_party/blink/renderer/platform/scheduler/public/thread_scheduler.h"
 #include "third_party/blink/renderer/platform/wtf/text/wtf_string.h"
+#include "third_party/blink/renderer/platform/wtf/vector.h"
 #include "third_party/blink/renderer/platform/wtf/wtf.h"
 
 namespace blink {
 
+class OnPurgeMemoryListener : public GarbageCollected<OnPurgeMemoryListener>,
+                              public MemoryCoordinatorClient {
+  USING_GARBAGE_COLLECTED_MIXIN(OnPurgeMemoryListener);
+
+  void OnPurgeMemory() override {
+    ParkableStringManager::Instance().ParkAllIfRendererBackgrounded(
+        ParkableStringImpl::ParkingMode::kAlways);
+  }
+};
+
 ParkableStringManager& ParkableStringManager::Instance() {
   DCHECK(IsMainThread());
   DEFINE_STATIC_LOCAL(ParkableStringManager, instance, ());
   return instance;
 }
 
-ParkableStringManager::~ParkableStringManager() {}
+ParkableStringManager::~ParkableStringManager() = default;
 
 void ParkableStringManager::SetRendererBackgrounded(bool backgrounded) {
+  DCHECK(IsMainThread());
   backgrounded_ = backgrounded;
 
   if (backgrounded_) {
@@ -37,7 +50,8 @@ void ParkableStringManager::SetRendererBackgrounded(bool backgrounded) {
     task_runner->PostDelayedTask(
         FROM_HERE,
         base::BindOnce(&ParkableStringManager::ParkAllIfRendererBackgrounded,
-                       base::Unretained(this)),
+                       base::Unretained(this),
+                       ParkableStringImpl::ParkingMode::kAlways),
         base::TimeDelta::FromSeconds(kParkingDelayInSeconds));
     // We only want to record statistics in the following case: a foreground tab
     // goes to background, and stays in background until the stats are recorded,
@@ -46,11 +60,15 @@ void ParkableStringManager::SetRendererBackgrounded(bool backgrounded) {
     // To that end:
     // 1. Don't post a recording task if one has been posted and hasn't run yet.
     // 2. Any background -> foreground transition between now and the
-    //    recording task running cancels stats recording.
+    //    recording task running cancels the task.
+    //
+    // Also drop strings that can be dropped cheaply in this task, to prevent
+    // used-once strings from increasing memory usage.
     if (!waiting_to_record_stats_) {
       task_runner->PostDelayedTask(
           FROM_HERE,
-          base::BindOnce(&ParkableStringManager::RecordStatistics,
+          base::BindOnce(&ParkableStringManager::
+                             DropStringsWithCompressedDataAndRecordStatistics,
                          base::Unretained(this)),
           base::TimeDelta::FromSeconds(kParkingDelayInSeconds +
                                        kStatisticsRecordingDelayInSeconds));
@@ -66,6 +84,7 @@ void ParkableStringManager::SetRendererBackgrounded(bool backgrounded) {
 }
 
 bool ParkableStringManager::IsRendererBackgrounded() const {
+  DCHECK(IsMainThread());
   return backgrounded_;
 }
 
@@ -79,6 +98,7 @@ bool ParkableStringManager::ShouldPark(const StringImpl& string) {
 
 scoped_refptr<ParkableStringImpl> ParkableStringManager::Add(
     scoped_refptr<StringImpl>&& string) {
+  DCHECK(IsMainThread());
   StringImpl* raw_ptr = string.get();
   auto it = unparked_strings_.find(raw_ptr);
   if (it != unparked_strings_.end())
@@ -125,8 +145,10 @@ void ParkableStringManager::OnUnparked(ParkableStringImpl* was_parked_string,
   unparked_strings_.insert(new_unparked_string, was_parked_string);
 }
 
-void ParkableStringManager::ParkAllIfRendererBackgrounded() {
+void ParkableStringManager::ParkAllIfRendererBackgrounded(
+    ParkableStringImpl::ParkingMode mode) {
   DCHECK(IsMainThread());
+
   if (!IsRendererBackgrounded())
     return;
 
@@ -137,22 +159,39 @@ void ParkableStringManager::ParkAllIfRendererBackgrounded() {
   for (ParkableStringImpl* str : parked_strings_)
     total_size += str->CharactersSizeInBytes();
 
-  for (ParkableStringImpl* str : unparked_strings_.Values()) {
-    str->Park();  // Parking is asynchronous.
+  // Parking may be synchronous, need to copy values first.
+  // In case of synchronous parking, |ParkableStringImpl::Park()| calls
+  // |OnParked()|, which moves the string from |unparked_strings_|
+  // to |parked_strings_|, hence the need to copy values first.
+  //
+  // Efficiency: In practice, either we are parking strings for the first time,
+  // and |unparked_strings_| can contain a few 10s of strings (and we will
+  // trigger expensive compression), or this is a subsequent one, and
+  // |unparked_strings_| will have few entries.
+  WTF::Vector<ParkableStringImpl*> unparked;
+  unparked.ReserveCapacity(unparked_strings_.size());
+  for (ParkableStringImpl* str : unparked_strings_.Values())
+    unparked.push_back(str);
+
+  for (ParkableStringImpl* str : unparked) {
+    str->Park(mode);
     total_size += str->CharactersSizeInBytes();
   }
 
-  size_t total_size_kb = total_size / 1000;
-  UMA_HISTOGRAM_COUNTS_100000("Memory.MovableStringsTotalSizeKb",
-                              total_size_kb);
-  UMA_HISTOGRAM_COUNTS_1000("Memory.MovableStringsCount", Size());
+  // Only collect stats for "full" parking calls.
+  if (mode == ParkableStringImpl::ParkingMode::kAlways) {
+    size_t total_size_kb = total_size / 1000;
+    UMA_HISTOGRAM_COUNTS_100000("Memory.MovableStringsTotalSizeKb",
+                                total_size_kb);
+    UMA_HISTOGRAM_COUNTS_1000("Memory.MovableStringsCount", Size());
+  }
 }
 
 size_t ParkableStringManager::Size() const {
   return parked_strings_.size() + unparked_strings_.size();
 }
 
-void ParkableStringManager::RecordStatistics() {
+void ParkableStringManager::DropStringsWithCompressedDataAndRecordStatistics() {
   DCHECK(IsMainThread());
   DCHECK(waiting_to_record_stats_);
   waiting_to_record_stats_ = false;
@@ -162,6 +201,11 @@ void ParkableStringManager::RecordStatistics() {
   // renderer is still backgrounded_.
   DCHECK(IsRendererBackgrounded());
 
+  // We are in the background, drop all the ParkableStrings we can without
+  // costing any CPU (as we already have the compressed representation).
+  ParkAllIfRendererBackgrounded(
+      ParkableStringImpl::ParkingMode::kIfCompressedDataExists);
+
   size_t total_size = 0, total_before_compression_size = 0;
   size_t total_compressed_size = 0;
   for (ParkableStringImpl* str : parked_strings_) {
@@ -202,6 +246,10 @@ ParkableStringManager::ParkableStringManager()
       waiting_to_record_stats_(false),
       should_record_stats_(false),
       unparked_strings_(),
-      parked_strings_() {}
+      parked_strings_() {
+  // No need to ever unregister, as the only ParkableStringManager instance
+  // lives forever.
+  MemoryCoordinator::Instance().RegisterClient(new OnPurgeMemoryListener());
+}
 
 }  // namespace blink

third_party/blink/renderer/platform/bindings/parkable_string_manager.h

@@ -8,6 +8,7 @@
 #include "base/feature_list.h"
 #include "base/macros.h"
 #include "base/memory/scoped_refptr.h"
+#include "third_party/blink/renderer/platform/bindings/parkable_string.h"
 #include "third_party/blink/renderer/platform/platform_export.h"
 #include "third_party/blink/renderer/platform/wtf/allocator.h"
 #include "third_party/blink/renderer/platform/wtf/hash_functions.h"
@@ -18,7 +19,6 @@
 namespace blink {
 
 class ParkableString;
-class ParkableStringImpl;
 
 const base::Feature kCompressParkableStringsInBackground{
     "CompressParkableStringsInBackground", base::FEATURE_DISABLED_BY_DEFAULT};
@@ -37,7 +37,6 @@ class PLATFORM_EXPORT ParkableStringManager {
   bool IsRendererBackgrounded() const;
   // Number of parked and unparked strings. Public for testing.
   size_t Size() const;
-  void ResetForTesting();
 
   // Whether a string is parkable or not. Can be called from any thread.
   static bool ShouldPark(const StringImpl& string);
@@ -49,6 +48,7 @@ class PLATFORM_EXPORT ParkableStringManager {
  private:
   friend class ParkableString;
   friend class ParkableStringImpl;
+  friend class OnPurgeMemoryListener;
 
   scoped_refptr<ParkableStringImpl> Add(scoped_refptr<StringImpl>&&);
   void Remove(ParkableStringImpl*, StringImpl*);
@@ -56,8 +56,9 @@ class PLATFORM_EXPORT ParkableStringManager {
   void OnParked(ParkableStringImpl*, StringImpl*);
   void OnUnparked(ParkableStringImpl*, StringImpl*);
 
-  void ParkAllIfRendererBackgrounded();
-  void RecordStatistics();
+  void ParkAllIfRendererBackgrounded(ParkableStringImpl::ParkingMode mode);
+  void DropStringsWithCompressedDataAndRecordStatistics();
+  void ResetForTesting();
 
   ParkableStringManager();
 
@@ -68,6 +69,8 @@ class PLATFORM_EXPORT ParkableStringManager {
       unparked_strings_;
   HashSet<ParkableStringImpl*, PtrHash<ParkableStringImpl>> parked_strings_;
 
+  friend class ParkableStringTest;
+  FRIEND_TEST_ALL_PREFIXES(ParkableStringTest, SynchronousCompression);
   DISALLOW_COPY_AND_ASSIGN(ParkableStringManager);
 };