Add Name::EnsureRawHash()

to get rid of the pattern
```
EnsureHash();
uint32_t field = raw_hash_field();
```
which requires an additional load and might be misleading in the
presence of forwarding indices for shared strings, as raw_hash_field()
can return a forwarding index, whereas EnsureRawHash() will always
return a computed hash value.

Bug: v8:12957
Change-Id: I33426fef433f774fb323d4381e784c1037fb6fbb
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3829469Reviewed-by: Igor Sheludko <ishell@chromium.org>
Commit-Queue: Patrick Thier <pthier@chromium.org>
Cr-Commit-Position: refs/heads/main@{#82441}

src/objects/name-inl.h

@@ -112,20 +112,27 @@ bool Name::HasForwardingIndex() const {
   return IsForwardingIndex(raw_hash_field(kAcquireLoad));
 }
 
-uint32_t Name::EnsureHash() {
+uint32_t Name::EnsureRawHash() {
   // Fast case: has hash code already been computed?
   uint32_t field = raw_hash_field();
-  if (IsHashFieldComputed(field)) return HashBits::decode(field);
+  if (IsHashFieldComputed(field)) return field;
   // Slow case: compute hash code and set it. Has to be a string.
-  return String::cast(*this).ComputeAndSetHash();
+  return String::cast(*this).ComputeAndSetRawHash();
 }
 
-uint32_t Name::EnsureHash(const SharedStringAccessGuardIfNeeded& access_guard) {
+uint32_t Name::EnsureRawHash(
+    const SharedStringAccessGuardIfNeeded& access_guard) {
   // Fast case: has hash code already been computed?
   uint32_t field = raw_hash_field();
-  if (IsHashFieldComputed(field)) return HashBits::decode(field);
+  if (IsHashFieldComputed(field)) return field;
   // Slow case: compute hash code and set it. Has to be a string.
-  return String::cast(*this).ComputeAndSetHash(access_guard);
+  return String::cast(*this).ComputeAndSetRawHash(access_guard);
+}
+
+uint32_t Name::EnsureHash() { return HashBits::decode(EnsureRawHash()); }
+
+uint32_t Name::EnsureHash(const SharedStringAccessGuardIfNeeded& access_guard) {
+  return HashBits::decode(EnsureRawHash(access_guard));
 }
 
 void Name::set_raw_hash_field_if_empty(uint32_t hash) {

src/objects/name.h

@@ -32,14 +32,6 @@ class Name : public TorqueGeneratedName<Name, PrimitiveHeapObject> {
   // in the string forwarding table.
   inline bool HasForwardingIndex() const;
 
-  // Returns a hash value used for the property table. Ensures that the hash
-  // value is computed.
-  //
-  // The overload without SharedStringAccessGuardIfNeeded can only be called on
-  // the main thread.
-  inline uint32_t EnsureHash();
-  inline uint32_t EnsureHash(const SharedStringAccessGuardIfNeeded&);
-
   inline uint32_t raw_hash_field() const {
     return RELAXED_READ_UINT32_FIELD(*this, kRawHashFieldOffset);
   }
@@ -177,6 +169,18 @@ class Name : public TorqueGeneratedName<Name, PrimitiveHeapObject> {
        << ArrayIndexLengthBits::kShift) |
       HashFieldTypeBits::kMask;
 
+  // Returns a hash value used for the property table. Ensures that the hash
+  // value is computed.
+  //
+  // The overload without SharedStringAccessGuardIfNeeded can only be called on
+  // the main thread.
+  inline uint32_t EnsureHash();
+  inline uint32_t EnsureHash(const SharedStringAccessGuardIfNeeded&);
+  // The value returned is always a computed hash, even if the value stored is
+  // a forwarding index.
+  inline uint32_t EnsureRawHash();
+  inline uint32_t EnsureRawHash(const SharedStringAccessGuardIfNeeded&);
+
   static inline bool IsHashFieldComputed(uint32_t raw_hash_field);
   static inline bool IsHash(uint32_t raw_hash_field);
   static inline bool IsIntegerIndex(uint32_t raw_hash_field);

src/objects/string-table.cc

@@ -482,7 +482,7 @@ Handle<String> StringTable::LookupString(Isolate* isolate,
   //  - String::Flatten is not threadsafe but is only called on non-shared
   //    strings, since non-flat strings are not shared.
   //
-  //  - String::ComputeAndSetHash is threadsafe on flat strings. This is safe
+  //  - String::ComputeAndSetRawHash is threadsafe on flat strings. This is safe
   //    because the characters are immutable and the same hash will be
   //    computed. The hash field is set with relaxed memory order. A thread that
   //    doesn't see the hash may do redundant work but will not be incorrect.
@@ -500,7 +500,6 @@ Handle<String> StringTable::LookupString(Isolate* isolate,
   // delay the transition into a ThinString to the next stop-the-world GC.
   Handle<String> result = String::Flatten(isolate, string);
   if (!result->IsInternalizedString()) {
-    result->EnsureHash();
     uint32_t raw_hash_field = result->raw_hash_field(kAcquireLoad);
 
     if (String::IsForwardingIndex(raw_hash_field)) {
@@ -509,6 +508,9 @@ Handle<String> StringTable::LookupString(Isolate* isolate,
           isolate->string_forwarding_table()->GetForwardString(isolate, index),
           isolate);
     } else {
+      if (!Name::IsHashFieldComputed(raw_hash_field)) {
+        raw_hash_field = result->EnsureRawHash();
+      }
       InternalizedStringKey key(result, raw_hash_field);
       result = LookupKey(isolate, &key);
     }

src/objects/string.cc

@@ -1495,17 +1495,18 @@ uint32_t HashString(String string, size_t start, int length, uint64_t seed,
 
 }  // namespace
 
-uint32_t String::ComputeAndSetHash() {
+uint32_t String::ComputeAndSetRawHash() {
   DCHECK(!SharedStringAccessGuardIfNeeded::IsNeeded(*this));
-  return ComputeAndSetHash(SharedStringAccessGuardIfNeeded::NotNeeded());
+  return ComputeAndSetRawHash(SharedStringAccessGuardIfNeeded::NotNeeded());
 }
-uint32_t String::ComputeAndSetHash(
+
+uint32_t String::ComputeAndSetRawHash(
     const SharedStringAccessGuardIfNeeded& access_guard) {
   DisallowGarbageCollection no_gc;
   // Should only be called if hash code has not yet been computed.
   //
   // If in-place internalizable strings are shared, there may be calls to
-  // ComputeAndSetHash in parallel. Since only flat strings are in-place
+  // ComputeAndSetRawHash in parallel. Since only flat strings are in-place
   // internalizable and their contents do not change, the result hash is the
   // same. The raw hash field is stored with relaxed ordering.
   DCHECK_IMPLIES(!FLAG_shared_string_table, !HasHashCode());
@@ -1519,7 +1520,7 @@ uint32_t String::ComputeAndSetHash(
         isolate, forward_index);
     uint32_t hash = internalized.raw_hash_field();
     DCHECK(IsHashFieldComputed(hash));
-    return HashBits::decode(hash);
+    return hash;
   }
 
   // Store the hash code in the object.
@@ -1542,8 +1543,10 @@ uint32_t String::ComputeAndSetHash(
     string = ThinString::cast(string).actual(cage_base);
     shape = StringShape(string, cage_base);
     if (length() == string.length()) {
-      set_raw_hash_field(string.raw_hash_field());
-      return hash();
+      uint32_t raw_hash = string.raw_hash_field();
+      DCHECK(IsHashFieldComputed(raw_hash));
+      set_raw_hash_field(raw_hash);
+      return raw_hash;
     }
   }
   uint32_t raw_hash_field =
@@ -1553,21 +1556,19 @@ uint32_t String::ComputeAndSetHash(
           : HashString<uint16_t>(string, start, length(), seed, cage_base,
                                  access_guard);
   set_raw_hash_field_if_empty(raw_hash_field);
-
   // Check the hash code is there (or a forwarding index if the string was
   // internalized in parallel).
   DCHECK(HasHashCode() || HasForwardingIndex());
-  uint32_t result = HashBits::decode(raw_hash_field);
-  DCHECK_NE(result, 0);  // Ensure that the hash value of 0 is never computed.
-  return result;
+  // Ensure that the hash value of 0 is never computed.
+  DCHECK_NE(HashBits::decode(raw_hash_field), 0);
+  return raw_hash_field;
 }
 
 bool String::SlowAsArrayIndex(uint32_t* index) {
   DisallowGarbageCollection no_gc;
   int length = this->length();
   if (length <= kMaxCachedArrayIndexLength) {
-    EnsureHash();  // Force computation of hash code.
-    uint32_t field = raw_hash_field();
+    uint32_t field = EnsureRawHash();  // Force computation of hash code.
     if (!IsIntegerIndex(field)) return false;
     *index = ArrayIndexValueBits::decode(field);
     return true;
@@ -1581,8 +1582,7 @@ bool String::SlowAsIntegerIndex(size_t* index) {
   DisallowGarbageCollection no_gc;
   int length = this->length();
   if (length <= kMaxCachedArrayIndexLength) {
-    EnsureHash();  // Force computation of hash code.
-    uint32_t field = raw_hash_field();
+    uint32_t field = EnsureRawHash();  // Force computation of hash code.
     if (!IsIntegerIndex(field)) return false;
     *index = ArrayIndexValueBits::decode(field);
     return true;

src/objects/string.h

@@ -648,9 +648,11 @@ class String : public TorqueGeneratedString<String, Name> {
   V8_EXPORT_PRIVATE bool SlowAsIntegerIndex(size_t* index);
 
   // Compute and set the hash code.
-  V8_EXPORT_PRIVATE uint32_t ComputeAndSetHash();
+  // The value returned is always a computed hash, even if the value stored is
+  // a forwarding index.
+  V8_EXPORT_PRIVATE uint32_t ComputeAndSetRawHash();
   V8_EXPORT_PRIVATE uint32_t
-  ComputeAndSetHash(const SharedStringAccessGuardIfNeeded&);
+  ComputeAndSetRawHash(const SharedStringAccessGuardIfNeeded&);
 
   TQ_OBJECT_CONSTRUCTORS(String)
 };

test/cctest/test-strings.cc

@@ -1768,8 +1768,7 @@ void TestString(i::Isolate* isolate, const IndexData& data) {
     size_t index;
     CHECK(s->AsIntegerIndex(&index));
     CHECK_EQ(data.integer_index, index);
-    s->EnsureHash();
-    CHECK(String::IsIntegerIndex(s->raw_hash_field()));
+    CHECK(String::IsIntegerIndex(s->EnsureRawHash()));
     CHECK(s->HasHashCode());
   }
   if (!s->HasHashCode()) s->EnsureHash();