blink/bindings: Reduce the overhead of ParkableString.

The overhead of ParkableString is:
- sizeof(ParkableString)
- sizeof(ParkableStringImpl) + digest + compressed data (if compressed)

The first one is small (sizeof(scoped_refptr<>), which is
sizeof(void*)), but the second one is non-trivial. This is due to:
- Need to cache metadata about a string when it is parked, for instance
  Is8Bit(), or its initial length.
- SHA256 hash used to deduplicate parked and unparked strings
- WTF::Mutex, which includes a pthread_mutex_t, which is large on most
  platforms.
- Handling of recursive locking, plus ParkableStringImpl being a
  refcounted class itself.

This is problematic, since it means that callers must know whether the
string is large, and either use WTF::String for small ones, or
ParkableString for large ones. It leads to more complex and duplicated
code, or to potential memory regressions.

As ParkableString is starting to be used in more places (e.g. for
trusted scripts), having to handle this becomes cumbersome and more
error-prone. On the other hand, all the metadata outlined above is not
necessary if the string is never parked, which is the case for small
ones. So the desired state is: short ParkableStrings are cheap enough so
that they can be used no matter the string length.

There are two simple ways to handle this:
- Have ParkableString hold the StringImpl directly for short strings
- Separate the metadata in ParkableStringImpl.

The first approach seems simpler, but would introduce non trivial
complexity in V8's value cache for strings (since it caches a
ref-counted impl for strings, and would need to know about the short and
long ParkableStrings). So the second one is selected here.

The new overhead is:
- Short ParkableStrings:
  - sizeof(ParkableStringImpl) == 3 * sizeof(void*) compared to
    WTF::String
  - savings of sizeof(ParkableStringImpl::Metadata) compared to
    ParkableString
- Long ParkableStrings: 0 vs the previous overhead (by replacing a
  std::unique_ptr<SecureDigest> with a
  std::unique_ptr<ParkableStringImpl::ParkableMetadata>)

Given that client code had to keep a ParkableString *and* a String to
handle short ones, the net cost is 2*sizeof(void*) for short strings,
and no code duplication.

As a consequence, ParkableString can now be used for short and long
strings without any custom code. CharacterData is converted
accordingly. A forthcoming CL will evaluate using ParkableString for
*all* CharacterData instances.

Also some cleanup:
- "git cl lint" warnings: include-what-you-use and explicit constructor
- Move mutex runtime assertions to compile-time checks.
- Make the ordering in parkable_string.cc consistent with the .h

Bug: 1059268, 877044
Change-Id: Ic397751970c6beeaf9d524102f50d5d71ec4a0ec
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2091494
Commit-Queue: Benoit L <lizeb@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#748199}

third_party/blink/renderer/core/dom/character_data.cc

@@ -38,19 +38,13 @@
 
 namespace blink {
 
-void CharacterData::MakeParkableOrAtomize() {
+void CharacterData::MakeParkable() {
   if (is_parkable_)
     return;
 
-  // ParkableStrings have some overhead, don't pay it if we're not going to
-  // park a string at all.
-  if (ParkableStringManager::ShouldPark(*data_.Impl())) {
-    parkable_data_ = ParkableString(data_.ReleaseImpl());
-    data_ = String();
-    is_parkable_ = true;
-  } else {
-    data_ = AtomicString(data_);
-  }
+  parkable_data_ = ParkableString(data_.ReleaseImpl());
+  data_ = String();
+  is_parkable_ = true;
 }
 
 void CharacterData::setData(const String& data) {

third_party/blink/renderer/core/dom/character_data.h

@@ -38,9 +38,8 @@ class CORE_EXPORT CharacterData : public Node {
   DEFINE_WRAPPERTYPEINFO();
 
  public:
-  // Makes the data either Parkable or Atomic. This enables de-duplication in
-  // both cases, the first one allowing compression as well.
-  void MakeParkableOrAtomize();
+  // Makes the data Parkable. This enables de-duplication and compression.
+  void MakeParkable();
   const String& data() const {
     return is_parkable_ ? parkable_data_.ToString() : data_;
   }

third_party/blink/renderer/core/dom/element.cc

@@ -26,8 +26,11 @@
 
 #include "third_party/blink/renderer/core/dom/element.h"
 
+#include <algorithm>
 #include <bitset>
+#include <limits>
 #include <memory>
+#include <utility>
 
 #include "cc/input/snap_selection_strategy.h"
 #include "third_party/blink/public/mojom/scroll/scroll_into_view_params.mojom-blink.h"
@@ -171,7 +174,7 @@ namespace {
 
 class DisplayLockStyleScope {
  public:
-  DisplayLockStyleScope(Element* element) : element_(element) {
+  explicit DisplayLockStyleScope(Element* element) : element_(element) {
     // Note that we don't store context as a member of this scope, since it may
     // get created as part of element self style recalc.
     auto* context = element->GetDisplayLockContext();
@@ -4946,7 +4949,7 @@ String Element::TextFromChildren() {
     return g_empty_string;
 
   if (first_text_node && !found_multiple_text_nodes) {
-    first_text_node->MakeParkableOrAtomize();
+    first_text_node->MakeParkable();
     return first_text_node->data();
   }
 

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

@@ -72,9 +72,29 @@ class PLATFORM_EXPORT ParkableStringImpl final
   const String& ToString();
 
   // See the matching String methods.
-  bool is_8bit() const { return is_8bit_; }
-  unsigned length() const { return length_; }
+  bool is_8bit() const {
+    if (!may_be_parked())
+      return string_.Is8Bit();
+
+    return metadata_->is_8bit_;
+  }
+  unsigned length() const {
+    if (!may_be_parked())
+      return string_.length();
+
+    return metadata_->length_;
+  }
   unsigned CharactersSizeInBytes() const;
+  size_t MemoryFootprintForDump() const;
+
+  // 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 !!metadata_; }
+
+  // Note: Public member functions below must only be called on strings for
+  // which |may_be_parked()| returns true. Otherwise, these will either trigger
+  // a DCHECK() or crash.
 
   // Tries to either age or park a string:
   //
@@ -98,36 +118,32 @@ class PLATFORM_EXPORT ParkableStringImpl final
   //
   // 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 !!digest_; }
+
   // Returns true if the string is parked.
   bool is_parked() const;
+
   // Returns whether synchronous parking is possible, that is the string was
   // parked in the past.
-  bool has_compressed_data() const { return !!compressed_; }
+  bool has_compressed_data() const { return !!metadata_->compressed_; }
+
   // Returns the compressed size, must not be called unless the string has a
   // compressed representation.
   size_t compressed_size() const {
     DCHECK(has_compressed_data());
-    return compressed_->size();
+    return metadata_->compressed_->size();
   }
 
   bool is_young_for_testing() {
-    MutexLocker locker(mutex_);
-    return is_young_;
+    MutexLocker locker(metadata_->mutex_);
+    return metadata_->is_young_;
   }
 
-  // Must only be called on parkable ParkableStringImpls.
-  SecureDigest* digest() const {
+  const SecureDigest* digest() const {
     AssertOnValidThread();
-    DCHECK(digest_);
-    return digest_.get();
+    DCHECK(metadata_);
+    return &metadata_->digest_;
   }
 
-  size_t MemoryFootprintForDump() const;
-
  private:
   enum class State : uint8_t;
   enum class Status : uint8_t;
@@ -138,20 +154,25 @@ class PLATFORM_EXPORT ParkableStringImpl final
   ParkableStringImpl(scoped_refptr<StringImpl>&& impl,
                      std::unique_ptr<SecureDigest> digest);
 
+  // Note: Private member  functions below must only be called on strings for
+  // which |may_be_parked()| returns true. Otherwise, these will either trigger
+  // a DCHECK() or crash.
+
 #if defined(ADDRESS_SANITIZER)
   // See |CompressInBackground()|. Doesn't make the string young.
   // May be called from any thread.
   void LockWithoutMakingYoung();
 #endif  // defined(ADDRESS_SANITIZER)
   // May be called from any thread.
-  void MakeYoung() EXCLUSIVE_LOCKS_REQUIRED(mutex_);
+  void MakeYoung() EXCLUSIVE_LOCKS_REQUIRED(metadata_->mutex_);
   // Whether the string is referenced or locked. The return value is valid as
   // long as |mutex_| is held.
-  Status CurrentStatus() const EXCLUSIVE_LOCKS_REQUIRED(mutex_);
-  bool CanParkNow() const EXCLUSIVE_LOCKS_REQUIRED(mutex_);
-  void ParkInternal(ParkingMode mode);
-  void Unpark();
-  String UnparkInternal() const;
+  Status CurrentStatus() const EXCLUSIVE_LOCKS_REQUIRED(metadata_->mutex_);
+  bool CanParkNow() const EXCLUSIVE_LOCKS_REQUIRED(metadata_->mutex_);
+  void ParkInternal(ParkingMode mode)
+      EXCLUSIVE_LOCKS_REQUIRED(metadata_->mutex_);
+  void Unpark() EXCLUSIVE_LOCKS_REQUIRED(metadata_->mutex_);
+  String UnparkInternal() const EXCLUSIVE_LOCKS_REQUIRED(metadata_->mutex_);
   // Called on the main thread after compression is done.
   // |params| is the same as the one passed to |CompressInBackground()|,
   // |compressed| is the compressed data, nullptr if compression failed.
@@ -166,31 +187,40 @@ class PLATFORM_EXPORT ParkableStringImpl final
   static void CompressInBackground(std::unique_ptr<CompressionTaskParams>);
 
   int lock_depth_for_testing() {
-    MutexLocker locker_(mutex_);
-    return lock_depth_;
+    MutexLocker locker_(metadata_->mutex_);
+    return metadata_->lock_depth_;
   }
 
-  Mutex mutex_;
-  int lock_depth_ GUARDED_BY(mutex_);
+  // Metadata only used for parkable ParkableStrings.
+  struct ParkableMetadata {
+    ParkableMetadata(String string, std::unique_ptr<SecureDigest> digest);
+
+    Mutex mutex_;
+    int lock_depth_ GUARDED_BY(mutex_);
+
+    // Main thread only.
+    State state_;
+    std::unique_ptr<Vector<uint8_t>> compressed_;
+    const SecureDigest digest_;
+
+    // A string can either be "young" or "old". It starts young, and transitions
+    // are:
+    // Young -> Old: By calling |MaybeAgeOrParkString()|.
+    // Old -> Young: When the string is accessed, either by |Lock()|-ing it or
+    //               calling |ToString()|.
+    //
+    // Thread safety: it is typically not safe to guard only one part of a
+    // bitfield with a mutex, but this is correct here, as the other members are
+    // const (and never change).
+    bool is_young_ : 1 GUARDED_BY(mutex_);
+    const bool is_8bit_ : 1;
+    const unsigned length_;
+
+    DISALLOW_COPY_AND_ASSIGN(ParkableMetadata);
+  };
 
-  // Main thread only.
-  State state_;
   String string_;
-  std::unique_ptr<Vector<uint8_t>> compressed_;
-  const std::unique_ptr<SecureDigest> digest_;
-
-  // A string can either be "young" or "old". It starts young, and transitions
-  // are:
-  // Young -> Old: By calling |MaybeAgeOrParkString()|.
-  // Old -> Young: When the string is accessed, either by |Lock()|-ing it or
-  //               calling |ToString()|.
-  //
-  // Thread safety: it is typically not safe to guard only one part of a
-  // bitfield with a mutex, but this is correct here, as the other members are
-  // const (and never change).
-  bool is_young_ : 1 GUARDED_BY(mutex_);
-  const bool is_8bit_ : 1;
-  const unsigned length_;
+  const std::unique_ptr<ParkableMetadata> metadata_;
 
 #if DCHECK_IS_ON()
   const base::PlatformThreadId owning_thread_;
@@ -202,13 +232,26 @@ class PLATFORM_EXPORT ParkableStringImpl final
 #endif
   }
 
-  FRIEND_TEST_ALL_PREFIXES(ParkableStringTest, LockUnlock);
-  FRIEND_TEST_ALL_PREFIXES(ParkableStringTest, LockParkedString);
+ public:
   FRIEND_TEST_ALL_PREFIXES(ParkableStringTest, Equality);
   FRIEND_TEST_ALL_PREFIXES(ParkableStringTest, EqualityNoUnparking);
+  FRIEND_TEST_ALL_PREFIXES(ParkableStringTest, LockUnlock);
+  FRIEND_TEST_ALL_PREFIXES(ParkableStringTest, LockParkedString);
+  FRIEND_TEST_ALL_PREFIXES(ParkableStringTest, ReportMemoryDump);
+  FRIEND_TEST_ALL_PREFIXES(ParkableStringTest, MemoryFootprintForDump);
+
   DISALLOW_COPY_AND_ASSIGN(ParkableStringImpl);
 };
 
+#if !DCHECK_IS_ON()
+// 3 pointers:
+// - vtable (from RefCounted)
+// - string_.Impl()
+// - metadata_
+static_assert(sizeof(ParkableStringImpl) == 3 * sizeof(void*),
+              "ParkableStringImpl should not be too large");
+#endif
+
 class PLATFORM_EXPORT ParkableString final {
   DISALLOW_NEW();
 
@@ -251,6 +294,9 @@ class PLATFORM_EXPORT ParkableString final {
   scoped_refptr<ParkableStringImpl> impl_;
 };
 
+static_assert(sizeof(ParkableString) == sizeof(void*),
+              "ParkableString should be small");
+
 }  // namespace blink
 
 #endif  // THIRD_PARTY_BLINK_RENDERER_PLATFORM_BINDINGS_PARKABLE_STRING_H_

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

@@ -68,13 +68,14 @@ const char* ParkableStringManager::kAllocatorDumpName = "parkable_strings";
 struct ParkableStringManager::SecureDigestHash {
   STATIC_ONLY(SecureDigestHash);
 
-  static unsigned GetHash(ParkableStringImpl::SecureDigest* const digest) {
+  static unsigned GetHash(
+      const ParkableStringImpl::SecureDigest* const digest) {
     // The first bytes of the hash are as good as anything else.
     return *reinterpret_cast<const unsigned*>(digest->data());
   }
 
-  static inline bool Equal(ParkableStringImpl::SecureDigest* const a,
-                           ParkableStringImpl::SecureDigest* const b) {
+  static inline bool Equal(const ParkableStringImpl::SecureDigest* const a,
+                           const ParkableStringImpl::SecureDigest* const b) {
     return a == b ||
            std::equal(a->data(), a->data() + ParkableStringImpl::kDigestSize,
                       b->data());
@@ -377,7 +378,7 @@ ParkableStringManager::Statistics ParkableStringManager::ComputeStatistics()
   // The digest has an inline capacity set to the digest size, hence sizeof() is
   // accurate.
   constexpr size_t kParkableStringImplActualSize =
-      sizeof(ParkableStringImpl) + sizeof(ParkableStringImpl::SecureDigest);
+      sizeof(ParkableStringImpl) + sizeof(ParkableStringImpl::ParkableMetadata);
 
   for (const auto& kv : unparked_strings_) {
     ParkableStringImpl* str = kv.value;

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

@@ -104,11 +104,11 @@ class PLATFORM_EXPORT ParkableStringManager {
 
   // Relies on secure hash equality for deduplication. If one day SHA256 becomes
   // insecure, then this would need to be updated to a more robust hash.
-  WTF::HashMap<ParkableStringImpl::SecureDigest*,
+  WTF::HashMap<const ParkableStringImpl::SecureDigest*,
                ParkableStringImpl*,
                SecureDigestHash>
       unparked_strings_;
-  WTF::HashMap<ParkableStringImpl::SecureDigest*,
+  WTF::HashMap<const ParkableStringImpl::SecureDigest*,
                ParkableStringImpl*,
                SecureDigestHash>
       parked_strings_;

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

@@ -611,6 +611,9 @@ TEST_F(ParkableStringTest, ReportMemoryDump) {
   using testing::Contains;
   using testing::Eq;
 
+  constexpr size_t kActualSize =
+      sizeof(ParkableStringImpl) + sizeof(ParkableStringImpl::ParkableMetadata);
+
   auto& manager = ParkableStringManager::Instance();
   ParkableString parkable1(MakeLargeString('a').ReleaseImpl());
   ParkableString parkable2(MakeLargeString('b').ReleaseImpl());
@@ -647,20 +650,20 @@ TEST_F(ParkableStringTest, ReportMemoryDump) {
                                       kCompressedSize);
   EXPECT_THAT(dump->entries(), Contains(Eq(ByRef(overhead))));
 
-  constexpr size_t kParkableStringImplActualSize =
-      sizeof(ParkableStringImpl) + sizeof(ParkableStringImpl::SecureDigest);
   MemoryAllocatorDump::Entry metadata("metadata_size", "bytes",
-                                      2 * kParkableStringImplActualSize);
+                                      2 * kActualSize);
   EXPECT_THAT(dump->entries(), Contains(Eq(ByRef(metadata))));
 
   MemoryAllocatorDump::Entry savings(
       "savings_size", "bytes",
-      2 * kStringSize - (kStringSize + 2 * kCompressedSize +
-                         2 * kParkableStringImplActualSize));
+      2 * kStringSize - (kStringSize + 2 * kCompressedSize + 2 * kActualSize));
   EXPECT_THAT(dump->entries(), Contains(Eq(ByRef(savings))));
 }
 
 TEST_F(ParkableStringTest, MemoryFootprintForDump) {
+  constexpr size_t kActualSize =
+      sizeof(ParkableStringImpl) + sizeof(ParkableStringImpl::ParkableMetadata);
+
   size_t memory_footprint;
   ParkableString parkable1(MakeLargeString('a').ReleaseImpl());
   ParkableString parkable2(MakeLargeString('b').ReleaseImpl());
@@ -670,19 +673,15 @@ TEST_F(ParkableStringTest, MemoryFootprintForDump) {
   parkable1.ToString();
 
   // Compressed and uncompressed data.
-  memory_footprint = sizeof(ParkableStringImpl) +
-                     sizeof(ParkableStringImpl::SecureDigest) +
-                     parkable1.Impl()->compressed_size() +
+  memory_footprint = kActualSize + parkable1.Impl()->compressed_size() +
                      parkable1.Impl()->CharactersSizeInBytes();
   EXPECT_EQ(memory_footprint, parkable1.Impl()->MemoryFootprintForDump());
 
   // Compressed uncompressed data only.
-  memory_footprint = sizeof(ParkableStringImpl) +
-                     sizeof(ParkableStringImpl::SecureDigest) +
-                     parkable2.Impl()->compressed_size();
+  memory_footprint = kActualSize + parkable2.Impl()->compressed_size();
   EXPECT_EQ(memory_footprint, parkable2.Impl()->MemoryFootprintForDump());
 
-  // Short string, no digest.
+  // Short string, no metadata.
   memory_footprint =
       sizeof(ParkableStringImpl) + parkable3.Impl()->CharactersSizeInBytes();
   EXPECT_EQ(memory_footprint, parkable3.Impl()->MemoryFootprintForDump());
@@ -694,10 +693,10 @@ TEST_F(ParkableStringTest, CompressionDisabled) {
 
   ParkableString parkable(MakeLargeString().ReleaseImpl());
   WaitForDelayedParking();
-  EXPECT_FALSE(parkable.Impl()->is_parked());
+  EXPECT_FALSE(parkable.Impl()->may_be_parked());
 
   MemoryPressureListenerRegistry::Instance().OnPurgeMemory();
-  EXPECT_FALSE(parkable.Impl()->is_parked());
+  EXPECT_FALSE(parkable.Impl()->may_be_parked());
 }
 
 TEST_F(ParkableStringTest, Aging) {