Changeset 255112 in webkit

Timestamp:

Jan 24, 2020, 5:35:58 PM (5 years ago)

Author:

mark.lam@apple.com

Message:

IntlObject's cached strings should be immortal and safe for concurrent access.
​https://bugs.webkit.org/show_bug.cgi?id=206779
<rdar://problem/58831763>

Reviewed by Yusuke Suzuki.

JSTests:

• stress/numberingSystemsForLocale-cached-strings-should-be-immortal-and-safe-for-concurrent-access.js: Added.

Source/JavaScriptCore:

In IntlObject's numberingSystemsForLocale(), we have a never destroyed
cachedNumberingSystems which is a singleton vector of Strings which are shared
multiple VMs. Hence, the strings in this vector should be a StaticStringImpl
so that it will be immortal, and can be access concurrently from multiple VMs
on different threads without any ref/deref'ing race issues.

• runtime/IntlObject.cpp:

(JSC::numberingSystemsForLocale):

Source/WTF:

Add a factory for creating a dynamically allocated StaticStringImpl.

Note: StaticStringImpl is guaranteed to have the same shape as StringImpl.
The only difference is that s_refCountFlagIsStaticString is set on the refCount
for StaticStringImpl. Since the client will use the StaticStringImpl as a
StringImpl, we implement the factory by using StringImpl::createInternal() for
simplicity, and set the s_refCountFlagIsStaticString flag thereafter.

• wtf/text/StringImpl.cpp:

(WTF::StringImpl::createStaticStringImpl):

• wtf/text/StringImpl.h:

Location:

trunk

Files:

• JSTests/ChangeLog (modified) (1 diff)
• JSTests/stress/numberingSystemsForLocale-cached-strings-should-be-immortal-and-safe-for-concurrent-access.js (added)
• Source/JavaScriptCore/ChangeLog (modified) (1 diff)
• Source/JavaScriptCore/runtime/IntlObject.cpp (modified) (2 diffs)
• Source/WTF/ChangeLog (modified) (1 diff)
• Source/WTF/wtf/text/StringImpl.cpp (modified) (2 diffs)
• Source/WTF/wtf/text/StringImpl.h (modified) (2 diffs)

trunk/JSTests/ChangeLog

@@ +1 @@
+2020-01-24  Mark Lam  <mark.lam@apple.com>
+
+        IntlObject's cached strings should be immortal and safe for concurrent access.
+        https://bugs.webkit.org/show_bug.cgi?id=206779
+        <rdar://problem/58831763>
+
+        Reviewed by Yusuke Suzuki.
+
+        * stress/numberingSystemsForLocale-cached-strings-should-be-immortal-and-safe-for-concurrent-access.js: Added.
+
 2020-01-24  Yusuke Suzuki  <ysuzuki@apple.com>
 

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+2020-01-24  Mark Lam  <mark.lam@apple.com>
+
+        IntlObject's cached strings should be immortal and safe for concurrent access.
+        https://bugs.webkit.org/show_bug.cgi?id=206779
+        <rdar://problem/58831763>
+
+        Reviewed by Yusuke Suzuki.
+
+        In IntlObject's numberingSystemsForLocale(), we have a never destroyed
+        cachedNumberingSystems which is a singleton vector of Strings which are shared
+        multiple VMs.  Hence, the strings in this vector should be a StaticStringImpl
+        so that it will be immortal, and can be access concurrently from multiple VMs
+        on different threads without any ref/deref'ing race issues.
+
+        * runtime/IntlObject.cpp:
+        (JSC::numberingSystemsForLocale):
+
 2020-01-24  Caio Lima  <ticaiolima@gmail.com>
 

trunk/Source/JavaScriptCore/runtime/IntlObject.cpp

@@ -2 +2 @@
  * Copyright (C) 2015 Andy VanWagoner (andy@vanwagoner.family)
  * Copyright (C) 2015 Sukolsak Sakshuwong (sukolsak@gmail.com)
- * Copyright (C) 2016-2019 Apple Inc. All rights reserved.
+ * Copyright (C) 2016-2020 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
@@ -892 +892 @@
     Vector<String>& availableNumberingSystems = cachedNumberingSystems.get();
 
-    if (UNLIKELY(availableNumberingSystems.isEmpty())) {
-        static Lock cachedNumberingSystemsMutex;
-        std::lock_guard<Lock> lock(cachedNumberingSystemsMutex);
-        if (availableNumberingSystems.isEmpty()) {
-            UErrorCode status = U_ZERO_ERROR;
-            UEnumeration* numberingSystemNames = unumsys_openAvailableNames(&status);
+    static std::once_flag initializeOnce;
+    std::call_once(initializeOnce, [&] {
+        ASSERT(availableNumberingSystems.isEmpty());
+        UErrorCode status = U_ZERO_ERROR;
+        UEnumeration* numberingSystemNames = unumsys_openAvailableNames(&status);
+        ASSERT(U_SUCCESS(status));
+
+        int32_t resultLength;
+        // Numbering system names are always ASCII, so use char[].
+        while (const char* result = uenum_next(numberingSystemNames, &resultLength, &status)) {
             ASSERT(U_SUCCESS(status));
-
-            int32_t resultLength;
-            // Numbering system names are always ASCII, so use char[].
-            while (const char* result = uenum_next(numberingSystemNames, &resultLength, &status)) {
-                ASSERT(U_SUCCESS(status));
-                auto numsys = unumsys_openByName(result, &status);
-                ASSERT(U_SUCCESS(status));
-                // Only support algorithmic if it is the default fot the locale, handled below.
-                if (!unumsys_isAlgorithmic(numsys))
-                    availableNumberingSystems.append(String(result, resultLength));
-                unumsys_close(numsys);
-            }
-            uenum_close(numberingSystemNames);
-        }
-    }
+            auto numsys = unumsys_openByName(result, &status);
+            ASSERT(U_SUCCESS(status));
+            // Only support algorithmic if it is the default fot the locale, handled below.
+            if (!unumsys_isAlgorithmic(numsys))
+                availableNumberingSystems.append(String(StringImpl::createStaticStringImpl(result, resultLength)));
+            unumsys_close(numsys);
+        }
+        uenum_close(numberingSystemNames);
+    });
 
     UErrorCode status = U_ZERO_ERROR;

trunk/Source/WTF/ChangeLog

@@ +1 @@
+2020-01-24  Mark Lam  <mark.lam@apple.com>
+
+        IntlObject's cached strings should be immortal and safe for concurrent access.
+        https://bugs.webkit.org/show_bug.cgi?id=206779
+        <rdar://problem/58831763>
+
+        Reviewed by Yusuke Suzuki.
+
+        Add a factory for creating a dynamically allocated StaticStringImpl.
+
+        Note: StaticStringImpl is guaranteed to have the same shape as StringImpl.
+        The only difference is that s_refCountFlagIsStaticString is set on the refCount
+        for StaticStringImpl.  Since the client will use the StaticStringImpl as a
+        StringImpl, we implement the factory by using StringImpl::createInternal() for
+        simplicity, and set the s_refCountFlagIsStaticString flag thereafter.
+
+        * wtf/text/StringImpl.cpp:
+        (WTF::StringImpl::createStaticStringImpl):
+        * wtf/text/StringImpl.h:
+
 2020-01-24  Keith Rollin  <krollin@apple.com>
 

trunk/Source/WTF/wtf/text/StringImpl.cpp

@@ -3 +3 @@
  *           (C) 1999 Antti Koivisto (koivisto@kde.org)
  *           (C) 2001 Dirk Mueller ( mueller@kde.org )
- * Copyright (C) 2003-2018 Apple Inc. All rights reserved.
+ * Copyright (C) 2003-2020 Apple Inc. All rights reserved.
  * Copyright (C) 2006 Andrew Wellington (proton@wiretapped.net)
  *
@@ -280 +280 @@
 {
     return createInternal(characters, length);
+}
+
+Ref<StringImpl> StringImpl::createStaticStringImpl(const char* characters, unsigned length)
+{
+    ASSERT(charactersAreAllASCII<LChar>(reinterpret_cast<const LChar*>(characters), length));
+    Ref<StringImpl> result = createInternal(reinterpret_cast<const LChar*>(characters), length);
+    result->m_refCount |= s_refCountFlagIsStaticString;
+    return result;
 }
 

trunk/Source/WTF/wtf/text/StringImpl.h

@@ -1 +1 @@
 /*
  * Copyright (C) 1999 Lars Knoll (knoll@kde.org)
- * Copyright (C) 2005-2019 Apple Inc. All rights reserved.
+ * Copyright (C) 2005-2020 Apple Inc. All rights reserved.
  * Copyright (C) 2009 Google Inc. All rights reserved.
  *
@@ -255 +255 @@
     WTF_EXPORT_PRIVATE static Ref<StringImpl> createUninitialized(unsigned length, UChar*&);
     template<typename CharacterType> static RefPtr<StringImpl> tryCreateUninitialized(unsigned length, CharacterType*&);
+
+    WTF_EXPORT_PRIVATE static Ref<StringImpl> createStaticStringImpl(const char*, unsigned length);
 
     // Reallocate the StringImpl. The originalString must be only owned by the Ref,