Bug 37978 – Unify JSC::IdentifierTable and WebCore::AtomicStringTable implementations.

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Template Framework

[Gavin Barraclough]

These two classes both implement a HashSet of uniqued StringImpls, with translator classes to avoid unnecessary object creation.
The only difference between the classes is which flag (isIdentifier or inTable) is set.  Combine the two classes using a template predicated on which flag to use.

[Gavin Barraclough]

Created attachment 54036 [details]
The patch

[WebKit Review Bot]

Attachment 54036 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
JavaScriptCore/wtf/text/AtomicStringTable.h:32:  Alphabetical sorting problem.  [build/include_order] [4]
JavaScriptCore/wtf/text/AtomicStringTable.h:53:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 2 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Gavin Barraclough]

Created attachment 54038 [details]
Fix review bot issues

[Geoffrey Garen]

Comment on attachment 54038 [details]
Fix review bot issues

Looks good. 

A few hashtable comments:


+    LiteralTable* literalTable = globalData->literalTable;
+    const LiteralTable::iterator& iter = literalTable->find(c);
+    if (iter != literalTable->end())
         return iter->second;
 
-    pair<HashSet<UString::Rep*>::iterator, bool> addResult = identifierTable.add<const char*, IdentifierCStringTranslator>(c);
-
-    // If the string is newly-translated, then we need to adopt it.
-    // The boolean in the pair tells us if that is so.
-    RefPtr<UString::Rep> addedString = addResult.second ? adoptRef(*addResult.first) : *addResult.first;
-
-    literalIdentifierTable.add(c, addedString.get());
-
+    RefPtr<StringImpl> addedString = globalData->identifierTable->add(c);
+    literalTable->add(c, addedString.get());
     return addedString.release();

I'm surprised that we have a separate table for char*. Weird. Maybe it's just legacy code that can go away?

Anyhoo, "literalTable->find" followed by "literalTable->add" is a double-lookup. The idiom for avoiding double-lookup is:

pair<iterator, bool> result = literalTable->add(c, 0);
if (!result.second) // pre-existing entry
    return result.first->second;
...
result.first->second = addedString.get();


+template<bool isIdentifierTable>
+IdentifierOrAtomicStringTable<isIdentifierTable>::~IdentifierOrAtomicStringTable()
+{
+    HashSet<StringImpl*>::iterator end = m_table.end();
+    for (HashSet<StringImpl*>::iterator iter = m_table.begin(); iter != end; ++iter) {
+        if (isIdentifierTable)
+            (*iter)->setIsIdentifier(false);
+        else
+            (*iter)->setIsAtomic(false);
+    }
+}

This would probably be more efficient if you wrote two copies of the loop, and moved the isIdentifierTable test out of the loop. We know that isIdentifierTable is constant inside the loop, but GCC probably doesn't.

r=me

[Gavin Barraclough]

> I'm surprised that we have a separate table for char*. Weird. Maybe it's just
> legacy code that can go away?

This table holds RefPtrs rather than StringImpl*s, I think the idea is to prevernt thrash creating & destroying string objects for literal strings in JSC's source.  I'm not sure if it's still necessary, but would rather not change in this patch.

> Anyhoo, "literalTable->find" followed by "literalTable->add" is a
> double-lookup. The idiom for avoiding double-lookup is:

*nod, will do.

> This would probably be more efficient if you wrote two copies of the loop, and
> moved the isIdentifierTable test out of the loop. We know that
> isIdentifierTable is constant inside the loop, but GCC probably doesn't.

If you think about it, GCC absolutely must know all template parameter values to code generate to be able to code specialize, which is the whole purpose of templates.  This is more obvious in the case where the template parameter is a type, also holds where the template parameter is a value.

Still, happy to rejig the check out the loop to make it more obvious that no redundant checking is going on.

[Gavin Barraclough]

Transmitting file data ................
Committed revision 58114.

[WebKit Review Bot]

http://trac.webkit.org/changeset/58114 might have broken Qt Linux Release