<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.1//EN"
"http://www.w3.org/TR/xhtml11/DTD/xhtml11.dtd">
<html xmlns="http://www.w3.org/1999/xhtml">
<head><meta http-equiv="content-type" content="text/html; charset=utf-8" />
<title>[210230] trunk</title>
</head>
<body>
<style type="text/css"><!--
#msg dl.meta { border: 1px #006 solid; background: #369; padding: 6px; color: #fff; }
#msg dl.meta dt { float: left; width: 6em; font-weight: bold; }
#msg dt:after { content:':';}
#msg dl, #msg dt, #msg ul, #msg li, #header, #footer, #logmsg { font-family: verdana,arial,helvetica,sans-serif; font-size: 10pt; }
#msg dl a { font-weight: bold}
#msg dl a:link { color:#fc3; }
#msg dl a:active { color:#ff0; }
#msg dl a:visited { color:#cc6; }
h3 { font-family: verdana,arial,helvetica,sans-serif; font-size: 10pt; font-weight: bold; }
#msg pre { overflow: auto; background: #ffc; border: 1px #fa0 solid; padding: 6px; }
#logmsg { background: #ffc; border: 1px #fa0 solid; padding: 1em 1em 0 1em; }
#logmsg p, #logmsg pre, #logmsg blockquote { margin: 0 0 1em 0; }
#logmsg p, #logmsg li, #logmsg dt, #logmsg dd { line-height: 14pt; }
#logmsg h1, #logmsg h2, #logmsg h3, #logmsg h4, #logmsg h5, #logmsg h6 { margin: .5em 0; }
#logmsg h1:first-child, #logmsg h2:first-child, #logmsg h3:first-child, #logmsg h4:first-child, #logmsg h5:first-child, #logmsg h6:first-child { margin-top: 0; }
#logmsg ul, #logmsg ol { padding: 0; list-style-position: inside; margin: 0 0 0 1em; }
#logmsg ul { text-indent: -1em; padding-left: 1em; }#logmsg ol { text-indent: -1.5em; padding-left: 1.5em; }
#logmsg > ul, #logmsg > ol { margin: 0 0 1em 0; }
#logmsg pre { background: #eee; padding: 1em; }
#logmsg blockquote { border: 1px solid #fa0; border-left-width: 10px; padding: 1em 1em 0 1em; background: white;}
#logmsg dl { margin: 0; }
#logmsg dt { font-weight: bold; }
#logmsg dd { margin: 0; padding: 0 0 0.5em 0; }
#logmsg dd:before { content:'\00bb';}
#logmsg table { border-spacing: 0px; border-collapse: collapse; border-top: 4px solid #fa0; border-bottom: 1px solid #fa0; background: #fff; }
#logmsg table th { text-align: left; font-weight: normal; padding: 0.2em 0.5em; border-top: 1px dotted #fa0; }
#logmsg table td { text-align: right; border-top: 1px dotted #fa0; padding: 0.2em 0.5em; }
#logmsg table thead th { text-align: center; border-bottom: 1px solid #fa0; }
#logmsg table th.Corner { text-align: left; }
#logmsg hr { border: none 0; border-top: 2px dashed #fa0; height: 1px; }
#header, #footer { color: #fff; background: #636; border: 1px #300 solid; padding: 6px; }
#patch { width: 100%; }
#patch h4 {font-family: verdana,arial,helvetica,sans-serif;font-size:10pt;padding:8px;background:#369;color:#fff;margin:0;}
#patch .propset h4, #patch .binary h4 {margin:0;}
#patch pre {padding:0;line-height:1.2em;margin:0;}
#patch .diff {width:100%;background:#eee;padding: 0 0 10px 0;overflow:auto;}
#patch .propset .diff, #patch .binary .diff {padding:10px 0;}
#patch span {display:block;padding:0 10px;}
#patch .modfile, #patch .addfile, #patch .delfile, #patch .propset, #patch .binary, #patch .copfile {border:1px solid #ccc;margin:10px 0;}
#patch ins {background:#dfd;text-decoration:none;display:block;padding:0 10px;}
#patch del {background:#fdd;text-decoration:none;display:block;padding:0 10px;}
#patch .lines, .info {color:#888;background:#fff;}
--></style>
<div id="msg">
<dl class="meta">
<dt>Revision</dt> <dd><a href="http://trac.webkit.org/projects/webkit/changeset/210230">210230</a></dd>
<dt>Author</dt> <dd>utatane.tea@gmail.com</dd>
<dt>Date</dt> <dd>2017-01-02 18:40:45 -0800 (Mon, 02 Jan 2017)</dd>
</dl>
<h3>Log Message</h3>
<pre>Leverage Substring to create new AtomicStringImpl for StaticStringImpl and SymbolImpl
https://bugs.webkit.org/show_bug.cgi?id=166636
Reviewed by Darin Adler.
Source/WTF:
Previously we always create the full atomic string if we need to create the same string
based on the given value. For example, when generating AtomicStringImpl from the SymbolImpl,
we need to create a new AtomicStringImpl since SymbolImpl never becomes `isAtomic() == true`.
But it is costly.
This patch leverages the substring system of StringImpl. Instead of allocating the completely
duplicate string, we create a substring StringImpl that shares the same content with the
base string.
* wtf/text/AtomicStringImpl.cpp:
(WTF::stringTable):
(WTF::addToStringTable):
(WTF::addSubstring):
(WTF::AtomicStringImpl::addSlowCase):
(WTF::AtomicStringImpl::remove):
(WTF::AtomicStringImpl::lookUpSlowCase):
* wtf/text/StringImpl.h:
(WTF::StringImpl::StaticStringImpl::operator StringImpl&):
Tools:
* TestWebKitAPI/Tests/WTF/StringImpl.cpp:
(TestWebKitAPI::TEST):</pre>
<h3>Modified Paths</h3>
<ul>
<li><a href="#trunkSourceWTFChangeLog">trunk/Source/WTF/ChangeLog</a></li>
<li><a href="#trunkSourceWTFwtftextAtomicStringImplcpp">trunk/Source/WTF/wtf/text/AtomicStringImpl.cpp</a></li>
<li><a href="#trunkSourceWTFwtftextStringImplh">trunk/Source/WTF/wtf/text/StringImpl.h</a></li>
<li><a href="#trunkToolsChangeLog">trunk/Tools/ChangeLog</a></li>
<li><a href="#trunkToolsTestWebKitAPITestsWTFStringImplcpp">trunk/Tools/TestWebKitAPI/Tests/WTF/StringImpl.cpp</a></li>
</ul>
</div>
<div id="patch">
<h3>Diff</h3>
<a id="trunkSourceWTFChangeLog"></a>
<div class="modfile"><h4>Modified: trunk/Source/WTF/ChangeLog (210229 => 210230)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/WTF/ChangeLog 2017-01-03 01:57:40 UTC (rev 210229)
+++ trunk/Source/WTF/ChangeLog 2017-01-03 02:40:45 UTC (rev 210230)
</span><span class="lines">@@ -1,5 +1,31 @@
</span><span class="cx"> 2017-01-02 Yusuke Suzuki <utatane.tea@gmail.com>
</span><span class="cx">
</span><ins>+ Leverage Substring to create new AtomicStringImpl for StaticStringImpl and SymbolImpl
+ https://bugs.webkit.org/show_bug.cgi?id=166636
+
+ Reviewed by Darin Adler.
+
+ Previously we always create the full atomic string if we need to create the same string
+ based on the given value. For example, when generating AtomicStringImpl from the SymbolImpl,
+ we need to create a new AtomicStringImpl since SymbolImpl never becomes `isAtomic() == true`.
+ But it is costly.
+
+ This patch leverages the substring system of StringImpl. Instead of allocating the completely
+ duplicate string, we create a substring StringImpl that shares the same content with the
+ base string.
+
+ * wtf/text/AtomicStringImpl.cpp:
+ (WTF::stringTable):
+ (WTF::addToStringTable):
+ (WTF::addSubstring):
+ (WTF::AtomicStringImpl::addSlowCase):
+ (WTF::AtomicStringImpl::remove):
+ (WTF::AtomicStringImpl::lookUpSlowCase):
+ * wtf/text/StringImpl.h:
+ (WTF::StringImpl::StaticStringImpl::operator StringImpl&):
+
+2017-01-02 Yusuke Suzuki <utatane.tea@gmail.com>
+
</ins><span class="cx"> Use StaticStringImpl instead of StaticASCIILiteral
</span><span class="cx"> https://bugs.webkit.org/show_bug.cgi?id=166586
</span><span class="cx">
</span></span></pre></div>
<a id="trunkSourceWTFwtftextAtomicStringImplcpp"></a>
<div class="modfile"><h4>Modified: trunk/Source/WTF/wtf/text/AtomicStringImpl.cpp (210229 => 210230)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/WTF/wtf/text/AtomicStringImpl.cpp 2017-01-03 01:57:40 UTC (rev 210229)
+++ trunk/Source/WTF/wtf/text/AtomicStringImpl.cpp 2017-01-03 02:40:45 UTC (rev 210230)
</span><span class="lines">@@ -68,19 +68,18 @@
</span><span class="cx">
</span><span class="cx"> #endif // USE(WEB_THREAD)
</span><span class="cx">
</span><del>-static ALWAYS_INLINE HashSet<StringImpl*>& stringTable()
</del><ins>+using StringTableImpl = HashSet<StringImpl*>;
+
+static ALWAYS_INLINE StringTableImpl& stringTable()
</ins><span class="cx"> {
</span><span class="cx"> return wtfThreadData().atomicStringTable()->table();
</span><span class="cx"> }
</span><span class="cx">
</span><span class="cx"> template<typename T, typename HashTranslator>
</span><del>-static inline Ref<AtomicStringImpl> addToStringTable(const T& value)
</del><ins>+static inline Ref<AtomicStringImpl> addToStringTable(AtomicStringTableLocker&, StringTableImpl& atomicStringTable, const T& value)
</ins><span class="cx"> {
</span><del>- AtomicStringTableLocker locker;
</del><ins>+ auto addResult = atomicStringTable.add<HashTranslator>(value);
</ins><span class="cx">
</span><del>- HashSet<StringImpl*>& atomicStringTable = stringTable();
- HashSet<StringImpl*>::AddResult addResult = atomicStringTable.add<HashTranslator>(value);
-
</del><span class="cx"> // If the string is newly-translated, then we need to adopt it.
</span><span class="cx"> // The boolean in the pair tells us if that is so.
</span><span class="cx"> if (addResult.isNewEntry)
</span><span class="lines">@@ -88,6 +87,13 @@
</span><span class="cx"> return *static_cast<AtomicStringImpl*>(*addResult.iterator);
</span><span class="cx"> }
</span><span class="cx">
</span><ins>+template<typename T, typename HashTranslator>
+static inline Ref<AtomicStringImpl> addToStringTable(const T& value)
+{
+ AtomicStringTableLocker locker;
+ return addToStringTable<T, HashTranslator>(locker, stringTable(), value);
+}
+
</ins><span class="cx"> struct CStringTranslator {
</span><span class="cx"> static unsigned hash(const LChar* c)
</span><span class="cx"> {
</span><span class="lines">@@ -400,16 +406,30 @@
</span><span class="cx"> return addToStringTable<CharBuffer, CharBufferFromLiteralDataTranslator>(buffer);
</span><span class="cx"> }
</span><span class="cx">
</span><ins>+static inline Ref<AtomicStringImpl> addSubstring(AtomicStringTableLocker& locker, StringTableImpl& atomicStringTable, StringImpl& base)
+{
+ ASSERT(base.length());
+ ASSERT(base.isSymbol() || base.isStatic());
+
+ SubstringLocation buffer = { &base, 0, base.length() };
+ if (base.is8Bit())
+ return addToStringTable<SubstringLocation, SubstringTranslator8>(locker, atomicStringTable, buffer);
+ return addToStringTable<SubstringLocation, SubstringTranslator16>(locker, atomicStringTable, buffer);
+}
+
+static inline Ref<AtomicStringImpl> addSubstring(StringImpl& base)
+{
+ AtomicStringTableLocker locker;
+ return addSubstring(locker, stringTable(), base);
+}
+
</ins><span class="cx"> Ref<AtomicStringImpl> AtomicStringImpl::addSlowCase(StringImpl& string)
</span><span class="cx"> {
</span><span class="cx"> if (!string.length())
</span><span class="cx"> return *static_cast<AtomicStringImpl*>(StringImpl::empty());
</span><span class="cx">
</span><del>- if (string.isSymbol() || string.isStatic()) {
- if (string.is8Bit())
- return *add(string.characters8(), string.length());
- return *add(string.characters16(), string.length());
- }
</del><ins>+ if (string.isSymbol() || string.isStatic())
+ return addSubstring(string);
</ins><span class="cx">
</span><span class="cx"> ASSERT_WITH_MESSAGE(!string.isAtomic(), "AtomicStringImpl should not hit the slow case if the string is already atomic.");
</span><span class="cx">
</span><span class="lines">@@ -430,9 +450,8 @@
</span><span class="cx"> return *static_cast<AtomicStringImpl*>(StringImpl::empty());
</span><span class="cx">
</span><span class="cx"> if (string.isSymbol() || string.isStatic()) {
</span><del>- if (string.is8Bit())
- return *add(string.characters8(), string.length());
- return *add(string.characters16(), string.length());
</del><ins>+ AtomicStringTableLocker locker;
+ return addSubstring(locker, stringTable.table(), string);
</ins><span class="cx"> }
</span><span class="cx">
</span><span class="cx"> ASSERT_WITH_MESSAGE(!string.isAtomic(), "AtomicStringImpl should not hit the slow case if the string is already atomic.");
</span><span class="lines">@@ -452,8 +471,8 @@
</span><span class="cx"> {
</span><span class="cx"> ASSERT(string->isAtomic());
</span><span class="cx"> AtomicStringTableLocker locker;
</span><del>- HashSet<StringImpl*>& atomicStringTable = stringTable();
- HashSet<StringImpl*>::iterator iterator = atomicStringTable.find(string);
</del><ins>+ auto& atomicStringTable = stringTable();
+ auto iterator = atomicStringTable.find(string);
</ins><span class="cx"> ASSERT_WITH_MESSAGE(iterator != atomicStringTable.end(), "The string being removed is atomic in the string table of an other thread!");
</span><span class="cx"> ASSERT(string == *iterator);
</span><span class="cx"> atomicStringTable.remove(iterator);
</span><span class="lines">@@ -466,14 +485,8 @@
</span><span class="cx"> if (!string.length())
</span><span class="cx"> return static_cast<AtomicStringImpl*>(StringImpl::empty());
</span><span class="cx">
</span><del>- if (string.isSymbol() || string.isStatic()) {
- if (string.is8Bit())
- return lookUpInternal(string.characters8(), string.length());
- return lookUpInternal(string.characters16(), string.length());
- }
-
</del><span class="cx"> AtomicStringTableLocker locker;
</span><del>- HashSet<StringImpl*>& atomicStringTable = stringTable();
</del><ins>+ auto& atomicStringTable = stringTable();
</ins><span class="cx"> auto iterator = atomicStringTable.find(&string);
</span><span class="cx"> if (iterator != atomicStringTable.end())
</span><span class="cx"> return static_cast<AtomicStringImpl*>(*iterator);
</span></span></pre></div>
<a id="trunkSourceWTFwtftextStringImplh"></a>
<div class="modfile"><h4>Modified: trunk/Source/WTF/wtf/text/StringImpl.h (210229 => 210230)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/WTF/wtf/text/StringImpl.h 2017-01-03 01:57:40 UTC (rev 210229)
+++ trunk/Source/WTF/wtf/text/StringImpl.h 2017-01-03 02:40:45 UTC (rev 210230)
</span><span class="lines">@@ -537,6 +537,7 @@
</span><span class="cx"> }
</span><span class="cx">
</span><span class="cx"> class StaticStringImpl {
</span><ins>+ WTF_MAKE_NONCOPYABLE(StaticStringImpl);
</ins><span class="cx"> public:
</span><span class="cx"> // Used to construct static strings, which have an special refCount that can never hit zero.
</span><span class="cx"> // This means that the static string will never be destroyed, which is important because
</span><span class="lines">@@ -559,6 +560,11 @@
</span><span class="cx"> {
</span><span class="cx"> }
</span><span class="cx">
</span><ins>+ operator StringImpl&()
+ {
+ return *reinterpret_cast<StringImpl*>(this);
+ }
+
</ins><span class="cx"> // These member variables must match the layout of StringImpl.
</span><span class="cx"> unsigned m_refCount;
</span><span class="cx"> unsigned m_length;
</span></span></pre></div>
<a id="trunkToolsChangeLog"></a>
<div class="modfile"><h4>Modified: trunk/Tools/ChangeLog (210229 => 210230)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Tools/ChangeLog 2017-01-03 01:57:40 UTC (rev 210229)
+++ trunk/Tools/ChangeLog 2017-01-03 02:40:45 UTC (rev 210230)
</span><span class="lines">@@ -1,3 +1,13 @@
</span><ins>+2017-01-02 Yusuke Suzuki <utatane.tea@gmail.com>
+
+ Leverage Substring to create new AtomicStringImpl for StaticStringImpl and SymbolImpl
+ https://bugs.webkit.org/show_bug.cgi?id=166636
+
+ Reviewed by Darin Adler.
+
+ * TestWebKitAPI/Tests/WTF/StringImpl.cpp:
+ (TestWebKitAPI::TEST):
+
</ins><span class="cx"> 2017-01-02 Manuel Rego Casasnovas <rego@igalia.com>
</span><span class="cx">
</span><span class="cx"> [GTK] WebCore/CSSParser unit test is not being built
</span></span></pre></div>
<a id="trunkToolsTestWebKitAPITestsWTFStringImplcpp"></a>
<div class="modfile"><h4>Modified: trunk/Tools/TestWebKitAPI/Tests/WTF/StringImpl.cpp (210229 => 210230)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Tools/TestWebKitAPI/Tests/WTF/StringImpl.cpp 2017-01-03 01:57:40 UTC (rev 210229)
+++ trunk/Tools/TestWebKitAPI/Tests/WTF/StringImpl.cpp 2017-01-03 02:40:45 UTC (rev 210230)
</span><span class="lines">@@ -554,11 +554,17 @@
</span><span class="cx"> ASSERT_TRUE(reference->isSymbol());
</span><span class="cx"> ASSERT_FALSE(reference->isAtomic());
</span><span class="cx">
</span><ins>+ auto result = AtomicStringImpl::lookUp(reference.ptr());
+ ASSERT_FALSE(result);
+
</ins><span class="cx"> auto atomic = AtomicStringImpl::add(reference.ptr());
</span><span class="cx"> ASSERT_TRUE(atomic->isAtomic());
</span><span class="cx"> ASSERT_FALSE(atomic->isSymbol());
</span><span class="cx"> ASSERT_TRUE(reference->isSymbol());
</span><span class="cx"> ASSERT_FALSE(reference->isAtomic());
</span><ins>+
+ auto result2 = AtomicStringImpl::lookUp(reference.ptr());
+ ASSERT_TRUE(result2);
</ins><span class="cx"> }
</span><span class="cx">
</span><span class="cx"> TEST(WTF, StringImplNullSymbolToAtomicString)
</span><span class="lines">@@ -567,13 +573,45 @@
</span><span class="cx"> ASSERT_TRUE(reference->isSymbol());
</span><span class="cx"> ASSERT_FALSE(reference->isAtomic());
</span><span class="cx">
</span><ins>+ // Because the substring of the reference is the empty string which is already interned.
+ auto result = AtomicStringImpl::lookUp(reference.ptr());
+ ASSERT_TRUE(result);
+
</ins><span class="cx"> auto atomic = AtomicStringImpl::add(reference.ptr());
</span><span class="cx"> ASSERT_TRUE(atomic->isAtomic());
</span><span class="cx"> ASSERT_FALSE(atomic->isSymbol());
</span><span class="cx"> ASSERT_TRUE(reference->isSymbol());
</span><span class="cx"> ASSERT_FALSE(reference->isAtomic());
</span><ins>+ ASSERT_EQ(atomic.get(), StringImpl::empty());
+
+ auto result2 = AtomicStringImpl::lookUp(reference.ptr());
+ ASSERT_TRUE(result2);
</ins><span class="cx"> }
</span><span class="cx">
</span><ins>+static StringImpl::StaticStringImpl staticString {"Cocoa"};
+
+TEST(WTF, StringImplStaticToAtomicString)
+{
+ StringImpl& original = staticString;
+ ASSERT_FALSE(original.isSymbol());
+ ASSERT_FALSE(original.isAtomic());
+ ASSERT_TRUE(original.isStatic());
+
+ auto result = AtomicStringImpl::lookUp(&original);
+ ASSERT_FALSE(result);
+
+ auto atomic = AtomicStringImpl::add(&original);
+ ASSERT_TRUE(atomic->isAtomic());
+ ASSERT_FALSE(atomic->isSymbol());
+ ASSERT_FALSE(atomic->isStatic());
+ ASSERT_FALSE(original.isSymbol());
+ ASSERT_FALSE(original.isAtomic());
+ ASSERT_TRUE(original.isStatic());
+
+ auto result2 = AtomicStringImpl::lookUp(&original);
+ ASSERT_TRUE(result2);
+}
+
</ins><span class="cx"> TEST(WTF, StringImplConstexprHasher)
</span><span class="cx"> {
</span><span class="cx"> ASSERT_EQ(stringFromUTF8("")->hash(), StringHasher::computeLiteralHashAndMaskTop8Bits(""));
</span></span></pre>
</div>
</div>
</body>
</html>