[webkit-reviews] review denied: [Bug 111041] Switch JSStringCache to a StringHash : [Attachment 191648] Patch

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has denied Benjamin Poulain
<benjamin at webkit.org>'s request for review:
Bug 111041: Switch JSStringCache to a StringHash
https://bugs.webkit.org/show_bug.cgi?id=111041

Attachment 191648: Patch
https://bugs.webkit.org/attachment.cgi?id=191648&action=review

------- Additional Comments from Geoffrey Garen <ggaren at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=191648&action=review


r- because I think I spotted some bugs.

That said, I do think it's worth reconsidering the design here. Reading this
patch, and given the bugs I've found / or the misunderstandings I've had, I
think this code will have a high maintenance burden. There's a lot of code here
with tricky edge cases, and some duplicated hash table code. 

The straw man alternative would be:
Step 1: Use WeakGCMap<String, Weak<JSString>>
Step 2: There is no step 2.

Is there a benchmark or some other performance estimate that shows that the
benefits of this solution over the straw man alternative outweigh the costs?

> Source/JavaScriptCore/runtime/WeakJSStringMap.h:79
> +inline WeakJSStringMap::~WeakJSStringMap()

Can this function just call "clear()" instead?

I noticed some subtle differences between this function and clear(). This
function uses ~Weak(), while clear() uses Weak::clear(). This function might
fastFree 0, while clear() won't. This function uses a -1 m_table as a deleted
table sentinel, while clear() uses 0. rehash() assumes it's not necessary to
call ~Weak() on a null value; this function does not. It's not clear to me that
these differences are all intentional. Some of them seem like bugs. Typically,
sharing code can help eliminate bugs like these -- or at least eliminate reader
confusion, if that is the problem here! :)

> Source/JavaScriptCore/runtime/WeakJSStringMap.h:89
> +static inline unsigned doubleHash(unsigned key)

Please use WTF::doubleHash instead of duplicating here. (We should fix
VisitedLinkTable.cpp to do the same.)

> Source/JavaScriptCore/runtime/WeakJSStringMap.h:99
> +ALWAYS_INLINE JSString* WeakJSStringMap::jsStringForString(ExecState* exec,
const String& wtfString)

I would just call this function "add", in keeping with the hash table function
name.

> Source/JavaScriptCore/runtime/WeakJSStringMap.h:102
> +    // Enforcing non-empty string simplify the implementation. The caller
should use jsEmptyString
> +    // anyway in those cases.

Grammar: should be "simplifies".

> Source/JavaScriptCore/runtime/WeakJSStringMap.h:150
> +    if (m_table) {

Please change this to early return to reduce the indentation level of the rest
of the code.

> Source/JavaScriptCore/runtime/WeakJSStringMap.h:178
> +    m_table =
static_cast<Weak<JSString>*>(fastZeroedMalloc(sizeof(Weak<JSString>) *
m_tableSize));

It's not so great to hard-code the assumption that the null Weak is zero in
memory. If somebody changes that, how will they know to update this code? Can
we add an ASSERT of some sort or, even better, a COMPILE_ASSERT?

> Source/JavaScriptCore/runtime/WeakJSStringMap.h:183
> +    for (unsigned i = 0; i < oldTableSize; ++i) {
> +	   if (!oldTable[i].isNull())
> +	       reinsert(oldTable[i]);
> +    }

This function will retain and reinsert Weaks that have been garbage collected.
Is that intentional? It seems needlessly inefficient.

reinsert() seems to ASSERT that we do not insert a null or empty value. It
seems like the ASSERT could fire here. This looks like a bug.

> Source/JavaScriptCore/runtime/WeakJSStringMap.h:235
> +	   // loadMax and bring the ratio close to 2/6. This give us a load in
the bounds [3/12, 5/12[.

Typo: should be "]".