blink/bindings: Park ParkableStrings in a separate task.

When a ParkableString is parked, do it in a separate task on the requester
thread. This is required to introduce compression.

This asynchronous parking means that a string can be in 3 states: unparked,
parking in progress or parked.

This CL adds:
- Asynchronous parking.
- Tracking of parked strings in ParkableStringManager
- Actual string parking with DCHECK_IS_ON()

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

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

@@ -4,9 +4,13 @@
 
 #include "third_party/blink/renderer/platform/bindings/parkable_string.h"
 
+#include "base/bind.h"
 #include "base/logging.h"
 #include "base/metrics/histogram_macros.h"
+#include "base/single_thread_task_runner.h"
+#include "third_party/blink/public/platform/platform.h"
 #include "third_party/blink/renderer/platform/bindings/parkable_string_manager.h"
+#include "third_party/blink/renderer/platform/scheduler/public/thread.h"
 #include "third_party/blink/renderer/platform/wtf/address_sanitizer.h"
 #include "third_party/blink/renderer/platform/wtf/thread_specific.h"
 
@@ -18,31 +22,55 @@ void RecordParkingAction(ParkableStringImpl::ParkingAction action) {
   UMA_HISTOGRAM_ENUMERATION("Memory.MovableStringParkingAction", action);
 }
 
-#if defined(ADDRESS_SANITIZER)
 void AsanPoisonString(const String& string) {
+#if defined(ADDRESS_SANITIZER)
+  // Since |string| is not deallocated, it remains in the per-thread
+  // AtomicStringTable, where its content can be accessed for equality
+  // comparison for instance, triggering a poisoned memory access.
+  // See crbug.com/883344 for an example.
+  if (string.Impl()->IsAtomic())
+    return;
+
   const void* start = string.Is8Bit()
                           ? static_cast<const void*>(string.Characters8())
                           : static_cast<const void*>(string.Characters16());
   ASAN_POISON_MEMORY_REGION(start, string.CharactersSizeInBytes());
+#endif  // defined(ADDRESS_SANITIZER)
 }
 
 void AsanUnpoisonString(const String& string) {
+#if defined(ADDRESS_SANITIZER)
   const void* start = string.Is8Bit()
                           ? static_cast<const void*>(string.Characters8())
                           : static_cast<const void*>(string.Characters16());
   ASAN_UNPOISON_MEMORY_REGION(start, string.CharactersSizeInBytes());
-}
 #endif  // defined(ADDRESS_SANITIZER)
+}
 
 }  // namespace
 
+// Valid transitions are:
+// 1. kUnparked -> kParkingInProgress: Parking started asynchronously
+// 2. kParkingInProgress -> kUnparked: Parking did not complete
+// 3. kParkingInProgress -> kParked: Parking completed normally
+// 4. kParked -> kUnparked: String has been unparked.
+//
+// See |Park()| for (1), |OnParkingCompleteOnMainThread()| for 2-3, and
+// |Unpark()| for (4).
+enum class ParkableStringImpl::State { kUnparked, kParkingInProgress, kParked };
+
 ParkableStringImpl::ParkableStringImpl(scoped_refptr<StringImpl>&& impl,
                                        ParkableState parkable)
     : mutex_(),
       lock_depth_(0),
+      state_(State::kUnparked),
       string_(std::move(impl)),
-      is_parked_(false),
-      is_parkable_(parkable == ParkableState::kParkable)
+#if DCHECK_IS_ON()
+      parked_string_(),
+#endif
+      may_be_parked_(parkable == ParkableState::kParkable),
+      is_8bit_(string_.Is8Bit()),
+      length_(string_.length())
 #if DCHECK_IS_ON()
       ,
       owning_thread_(CurrentThread())
@@ -53,11 +81,17 @@ ParkableStringImpl::ParkableStringImpl(scoped_refptr<StringImpl>&& impl,
 
 ParkableStringImpl::~ParkableStringImpl() {
   AssertOnValidThread();
-#if defined(ADDRESS_SANITIZER)
-  AsanUnpoisonString(string_);
+#if DCHECK_IS_ON()
+  {
+    MutexLocker locker(mutex_);
+    DCHECK_EQ(0, lock_depth_);
+  }
 #endif
-  if (is_parkable_)
-    ParkableStringManager::Instance().Remove(string_.Impl());
+  AsanUnpoisonString(string_);
+  DCHECK(state_ == State::kParked || state_ == State::kUnparked);
+
+  if (may_be_parked_)
+    ParkableStringManager::Instance().Remove(this, string_.Impl());
 }
 
 void ParkableStringImpl::Lock() {
@@ -78,36 +112,16 @@ void ParkableStringImpl::Unlock() {
   // the owning thread may concurrently call |ToString()|. It is then allowed
   // to use the string until the end of the current owning thread task.
   // Requires DCHECK_IS_ON() for the |owning_thread_| check.
-  if (lock_depth_ == 0 && is_parkable_ && string_.Impl()->HasOneRef() &&
-      owning_thread_ == CurrentThread()) {
-    // Since |string_| is not deallocated, it remains in the per-thread
-    // AtomicStringTable, where its content can be accessed for equality
-    // comparison for instance, triggering a poisoned memory access.
-    // See crbug.com/883344 for an example.
-    // TODO(lizeb): park the string in |Unlock()| when ASAN is enabled, removing
-    // this check.
-    if (!string_.Impl()->IsAtomic())
-      AsanPoisonString(string_);
+  if (CanParkNow() && owning_thread_ == CurrentThread()) {
+    AsanPoisonString(string_);
   }
 #endif  // defined(ADDRESS_SANITIZER) && DCHECK_IS_ON()
 }
 
-bool ParkableStringImpl::Is8Bit() const {
-  AssertOnValidThread();
-  return string_.Is8Bit();
-}
-
-bool ParkableStringImpl::IsNull() const {
-  AssertOnValidThread();
-  return string_.IsNull();
-}
-
 const String& ParkableStringImpl::ToString() {
   AssertOnValidThread();
   MutexLocker locker(mutex_);
-#if defined(ADDRESS_SANITIZER)
   AsanUnpoisonString(string_);
-#endif
 
   Unpark();
   return string_;
@@ -115,41 +129,92 @@ const String& ParkableStringImpl::ToString() {
 
 unsigned ParkableStringImpl::CharactersSizeInBytes() const {
   AssertOnValidThread();
-  return string_.CharactersSizeInBytes();
+  return length_ * (is_8bit() ? sizeof(LChar) : sizeof(UChar));
 }
 
 bool ParkableStringImpl::Park() {
   AssertOnValidThread();
   MutexLocker locker(mutex_);
-  DCHECK(is_parkable_);
+  DCHECK(may_be_parked_);
+  if (state_ == State::kUnparked && CanParkNow()) {
+    AsanPoisonString(string_);
+    auto task_runner = Platform::Current()->CurrentThread()->GetTaskRunner();
+    task_runner->PostTask(
+        FROM_HERE,
+        base::BindOnce(&ParkableStringImpl::OnParkingCompleteOnMainThread,
+                       this));
+    state_ = State::kParkingInProgress;
+  }
 
-  if (lock_depth_ != 0)
-    return false;
+  return state_ == State::kParked || state_ == State::kParkingInProgress;
+}
 
-  // Cannot park strings with several references.
-  if (string_.Impl()->HasOneRef()) {
-#if defined(ADDRESS_SANITIZER)
-    // See comment in |Unlock()| for this restriction.
-    if (!string_.Impl()->IsAtomic())
-      AsanPoisonString(string_);
-#endif
-    RecordParkingAction(ParkingAction::kParkedInBackground);
-    is_parked_ = true;
-  }
-  return is_parked_;
+bool ParkableStringImpl::is_parked() const {
+  return state_ == State::kParked;
+}
+
+bool ParkableStringImpl::CanParkNow() const {
+  mutex_.AssertAcquired();
+  // Can park iff:
+  // - the string is eligible to parking
+  // - There are no external reference to |string_|. Since |this| holds a
+  //   reference to it, then we are the only one.
+  // - |this| is not locked.
+  return may_be_parked_ && string_.Impl()->HasOneRef() && lock_depth_ == 0;
 }
 
 void ParkableStringImpl::Unpark() {
   AssertOnValidThread();
   mutex_.AssertAcquired();
-  if (!is_parked_)
+  if (state_ != State::kParked)
     return;
 
+#if DCHECK_IS_ON()
+  string_ = parked_string_;
+  parked_string_ = String();
+#endif
+  state_ = State::kUnparked;
+
   bool backgrounded =
       ParkableStringManager::Instance().IsRendererBackgrounded();
   RecordParkingAction(backgrounded ? ParkingAction::kUnparkedInBackground
                                    : ParkingAction::kUnparkedInForeground);
-  is_parked_ = false;
+  auto& manager = ParkableStringManager::Instance();
+  manager.OnUnparked(this, string_.Impl());
+}
+
+void ParkableStringImpl::OnParkingCompleteOnMainThread() {
+  MutexLocker locker(mutex_);
+  DCHECK_EQ(State::kParkingInProgress, state_);
+  // Between |Park()| and now, things may have happened:
+  // 1. |ToString()| or
+  // 2. |Lock()| may have been called.
+  //
+  // We only care about "surviving" calls, that is iff the string returned by
+  // |ToString()| is still alive, or whether we are still locked. Since this
+  // function is protected by the lock, no concurrent modifications can occur.
+  //
+  // Finally, since this is a distinct task from any one that can call
+  // |ToString()|, the invariant that the pointer stays valid until the next
+  // task is preserved.
+  if (CanParkNow()) {
+    RecordParkingAction(ParkingAction::kParkedInBackground);
+    state_ = State::kParked;
+    ParkableStringManager::Instance().OnParked(this, string_.Impl());
+#if DCHECK_IS_ON()
+    AsanUnpoisonString(string_);  // Will touch the string below.
+    parked_string_ = string_.IsolatedCopy();
+    // Poison the previous allocation.
+    // |string_| is kept to make sure the memory is not reused.
+    const void* data = is_8bit()
+                           ? static_cast<const void*>(string_.Characters8())
+                           : static_cast<const void*>(string_.Characters16());
+    memset(const_cast<void*>(data), 0xcc, string_.CharactersSizeInBytes());
+#endif  // DCHECK_IS_ON()
+    AsanPoisonString(string_);
+  } else {
+    state_ = State::kUnparked;
+  }
 }
 
 ParkableString::ParkableString(scoped_refptr<StringImpl>&& impl) {
@@ -180,7 +245,7 @@ void ParkableString::Unlock() const {
 }
 
 bool ParkableString::Is8Bit() const {
-  return impl_->Is8Bit();
+  return impl_->is_8bit();
 }
 
 String ParkableString::ToString() const {

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

@@ -64,28 +64,42 @@ class PLATFORM_EXPORT ParkableStringImpl final
   const String& ToString();
 
   // See the matching String methods.
-  bool Is8Bit() const;
-  bool IsNull() const;
-  unsigned length() const { return string_.length(); }
+  bool is_8bit() const { return is_8bit_; }
+  unsigned length() const { return length_; }
   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();
-  // Returns true iff the string can be parked.
-  bool is_parkable() const { return is_parkable_; }
+  // 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;
 
  private:
+  enum class State;
+
+  // Whether the string can be parked now. Must be called with |mutex_| held,
+  // and the return value is valid as long as the mutex is held.
+  bool CanParkNow() const;
   void Unpark();
-  // Returns true if the string is parked.
-  bool is_parked() const { return is_parked_; }
+  void OnParkingCompleteOnMainThread();
 
-  Mutex mutex_;  // protects the variables below.
+  Mutex mutex_;  // protects lock_depth_.
   int lock_depth_;
+
+  // Main thread only.
+  State state_;
   String string_;
-  bool is_parked_;
+#if DCHECK_IS_ON()
+  String parked_string_;
+#endif
 
-  const bool is_parkable_;
+  const bool may_be_parked_;
+  const bool is_8bit_;
+  const unsigned length_;
 
 #if DCHECK_IS_ON()
   const ThreadIdentifier owning_thread_;
@@ -98,11 +112,13 @@ class PLATFORM_EXPORT ParkableStringImpl final
   }
 
   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);
   DISALLOW_COPY_AND_ASSIGN(ParkableStringImpl);
 };
 
@@ -126,7 +142,7 @@ class PLATFORM_EXPORT ParkableString final {
   bool Is8Bit() const;
   bool IsNull() const { return !impl_; }
   unsigned length() const { return impl_ ? impl_->length() : 0; }
-  bool is_parkable() const { return impl_ && impl_->is_parkable(); }
+  bool may_be_parked() const { return impl_ && impl_->may_be_parked(); }
 
   ParkableStringImpl* Impl() const { return impl_ ? impl_.get() : nullptr; }
   // Returns an unparked version of the string.

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

@@ -9,6 +9,7 @@
 #include "base/bind.h"
 #include "base/macros.h"
 #include "base/metrics/histogram_macros.h"
+#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/scheduler/public/thread.h"
@@ -32,6 +33,7 @@ void ParkableStringManager::SetRendererBackgrounded(bool backgrounded) {
   if (backgrounded_) {
     scoped_refptr<base::SingleThreadTaskRunner> task_runner =
         Platform::Current()->CurrentThread()->GetTaskRunner();
+    DCHECK(task_runner);
     task_runner->PostDelayedTask(
         FROM_HERE,
         base::BindOnce(&ParkableStringManager::ParkAllIfRendererBackgrounded,
@@ -55,22 +57,49 @@ bool ParkableStringManager::ShouldPark(const StringImpl& string) {
 scoped_refptr<ParkableStringImpl> ParkableStringManager::Add(
     scoped_refptr<StringImpl>&& string) {
   StringImpl* raw_ptr = string.get();
-  auto it = table_.find(raw_ptr);
-  if (it != table_.end())
+  auto it = unparked_strings_.find(raw_ptr);
+  if (it != unparked_strings_.end())
     return it->value;
 
   auto new_parkable_string = base::MakeRefCounted<ParkableStringImpl>(
       std::move(string), ParkableStringImpl::ParkableState::kParkable);
-  table_.insert(raw_ptr, new_parkable_string.get());
+  unparked_strings_.insert(raw_ptr, new_parkable_string.get());
   return new_parkable_string;
 }
 
-void ParkableStringManager::Remove(StringImpl* string) {
-  DCHECK(string);
-  DCHECK(ShouldPark(*string));
-  auto it = table_.find(string);
-  DCHECK(it != table_.end());
-  table_.erase(it);
+void ParkableStringManager::Remove(ParkableStringImpl* string,
+                                   StringImpl* maybe_unparked_string) {
+  DCHECK(IsMainThread());
+  if (string->is_parked()) {
+    auto it = parked_strings_.find(string);
+    DCHECK(it != parked_strings_.end());
+    parked_strings_.erase(it);
+  } else {
+    DCHECK(maybe_unparked_string);
+    auto it = unparked_strings_.find(maybe_unparked_string);
+    DCHECK(it != unparked_strings_.end());
+    unparked_strings_.erase(it);
+  }
+}
+
+void ParkableStringManager::OnParked(ParkableStringImpl* newly_parked_string,
+                                     StringImpl* previous_unparked_string) {
+  DCHECK(IsMainThread());
+  DCHECK(newly_parked_string->is_parked());
+  auto it = unparked_strings_.find(previous_unparked_string);
+  DCHECK(it != unparked_strings_.end());
+  unparked_strings_.erase(it);
+  parked_strings_.insert(newly_parked_string);
+}
+
+void ParkableStringManager::OnUnparked(ParkableStringImpl* was_parked_string,
+                                       StringImpl* new_unparked_string) {
+  DCHECK(IsMainThread());
+  DCHECK(!was_parked_string->is_parked());
+  auto it = parked_strings_.find(was_parked_string);
+  DCHECK(it != parked_strings_.end());
+  parked_strings_.erase(it);
+  unparked_strings_.insert(new_unparked_string, was_parked_string);
 }
 
 void ParkableStringManager::ParkAllIfRendererBackgrounded() {
@@ -81,20 +110,26 @@ void ParkableStringManager::ParkAllIfRendererBackgrounded() {
   if (!base::FeatureList::IsEnabled(kCompressParkableStringsInBackground))
     return;
 
-  size_t total_size = 0, count = 0;
-  for (ParkableStringImpl* str : table_.Values()) {
-    str->Park();
+  size_t total_size = 0;
+  for (ParkableStringImpl* str : parked_strings_)
+    total_size += str->CharactersSizeInBytes();
+
+  for (ParkableStringImpl* str : unparked_strings_.Values()) {
+    str->Park();  // Parking is asynchronous.
     total_size += str->CharactersSizeInBytes();
-    count += 1;
   }
 
   size_t total_size_kb = total_size / 1000;
   UMA_HISTOGRAM_COUNTS_100000("Memory.MovableStringsTotalSizeKb",
                               total_size_kb);
-  UMA_HISTOGRAM_COUNTS_1000("Memory.MovableStringsCount", table_.size());
+  UMA_HISTOGRAM_COUNTS_1000("Memory.MovableStringsCount", Size());
+}
+
+size_t ParkableStringManager::Size() const {
+  return parked_strings_.size() + unparked_strings_.size();
 }
 
 ParkableStringManager::ParkableStringManager()
-    : backgrounded_(false), table_() {}
+    : backgrounded_(false), unparked_strings_(), parked_strings_() {}
 
 }  // namespace blink

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

@@ -8,11 +8,11 @@
 #include "base/feature_list.h"
 #include "base/macros.h"
 #include "base/memory/scoped_refptr.h"
-#include "base/single_thread_task_runner.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"
 #include "third_party/blink/renderer/platform/wtf/hash_map.h"
+#include "third_party/blink/renderer/platform/wtf/hash_set.h"
 #include "third_party/blink/renderer/platform/wtf/text/wtf_string.h"
 
 namespace blink {
@@ -44,17 +44,23 @@ class PLATFORM_EXPORT ParkableStringManager {
   friend class ParkableStringImpl;
 
   scoped_refptr<ParkableStringImpl> Add(scoped_refptr<StringImpl>&&);
-  void Remove(StringImpl*);
+  void Remove(ParkableStringImpl*, StringImpl*);
+
+  void OnParked(ParkableStringImpl*, StringImpl*);
+  void OnUnparked(ParkableStringImpl*, StringImpl*);
 
   void ParkAllIfRendererBackgrounded();
+  size_t Size() const;
 
   ParkableStringManager();
 
   bool backgrounded_;
-  HashMap<StringImpl*, ParkableStringImpl*, PtrHash<StringImpl>> table_;
+  HashMap<StringImpl*, ParkableStringImpl*, PtrHash<StringImpl>>
+      unparked_strings_;
+  HashSet<ParkableStringImpl*, PtrHash<ParkableStringImpl>> parked_strings_;
 
-  FRIEND_TEST_ALL_PREFIXES(ParkableStringTest, TableSimple);
-  FRIEND_TEST_ALL_PREFIXES(ParkableStringTest, TableMultiple);
+  FRIEND_TEST_ALL_PREFIXES(ParkableStringTest, ManagerSimple);
+  FRIEND_TEST_ALL_PREFIXES(ParkableStringTest, ManagerMultipleStrings);
   DISALLOW_COPY_AND_ASSIGN(ParkableStringManager);
 };
 

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

@@ -164,7 +164,9 @@ class ParkableStringResource16 final
     DCHECK(!parkable_string_.Is8Bit());
   }
 
-  bool IsCacheable() const override { return !parkable_string_.is_parkable(); }
+  bool IsCacheable() const override {
+    return !parkable_string_.may_be_parked();
+  }
 
   void Lock() const override { parkable_string_.Lock(); }
 
@@ -188,7 +190,9 @@ class ParkableStringResource8 final
     DCHECK(parkable_string_.Is8Bit());
   }
 
-  bool IsCacheable() const override { return !parkable_string_.is_parkable(); }
+  bool IsCacheable() const override {
+    return !parkable_string_.may_be_parked();
+  }
 
   void Lock() const override { parkable_string_.Lock(); }