Bug 37978 - Unify JSC::IdentifierTable and WebCore::AtomicStringTable implementations.
Summary: Unify JSC::IdentifierTable and WebCore::AtomicStringTable implementations.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Gavin Barraclough
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-04-22 00:34 PDT by Gavin Barraclough
Modified: 2010-04-22 13:55 PDT (History)
3 users (show)

See Also:


Attachments
The patch (41.49 KB, patch)
2010-04-22 00:42 PDT, Gavin Barraclough
no flags Details | Formatted Diff | Diff
Fix review bot issues (41.49 KB, patch)
2010-04-22 00:47 PDT, Gavin Barraclough
ggaren: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Gavin Barraclough 2010-04-22 00:34:43 PDT
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.
Comment 1 Gavin Barraclough 2010-04-22 00:42:19 PDT
Created attachment 54036 [details]
The patch
Comment 2 WebKit Review Bot 2010-04-22 00:44:47 PDT
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.
Comment 3 Gavin Barraclough 2010-04-22 00:47:01 PDT
Created attachment 54038 [details]
Fix review bot issues
Comment 4 Geoffrey Garen 2010-04-22 09:58:45 PDT
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
Comment 5 Gavin Barraclough 2010-04-22 12:39:08 PDT
> 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.
Comment 6 Gavin Barraclough 2010-04-22 13:38:57 PDT
Transmitting file data ................
Committed revision 58114.
Comment 7 WebKit Review Bot 2010-04-22 13:55:29 PDT
http://trac.webkit.org/changeset/58114 might have broken Qt Linux Release