[webkit-reviews] review denied: [Bug 86024] Enh: Hash Const JSString in Backing Stores to Save Memory : [Attachment 150197] Patch with fix for string hash ASSERT failures

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has denied Michael Saboff
<msaboff at apple.com>'s request for review:
Bug 86024: Enh: Hash Const JSString in Backing Stores to Save Memory
https://bugs.webkit.org/show_bug.cgi?id=86024

Attachment 150197: Patch with fix for string hash ASSERT failures
https://bugs.webkit.org/attachment.cgi?id=150197&action=review

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


r- because of the changes suggested below, and because we need data to figure
out if this patch slows down GC.

> Source/JavaScriptCore/heap/MarkStack.cpp:532
> +    do {

Should be:

unsigned currentFlags = m_flags;
unsigned newFlags = currentFlags | s_hashConstLock;
if (!WTF::weakCompareAndSwap(&m_flags, currentFlags, newFlags))
    return false;
WTF::memoryBarrierAfterLock();
return true;

(tryLock should only try once; weak memory ordering platforms like ARM require
fences for locks.)

> Source/JavaScriptCore/heap/MarkStack.cpp:552
> +ALWAYS_INLINE void MarkStack::internalAppend(JSValue* slot)

Let's put a comment here explaining that we're specifically excluding all
visits except for visits to object and array backing stores. That way, if
someone refactors this code, they'll maintain that requirement.

> Source/JavaScriptCore/heap/MarkStack.cpp:560
> +    if (value.isString()) {

Let's cast to JSCell* before doing this test, to avoid testing isCell() twice.
Maybe the compiler will optimize that out for us, but let's not rely on that.

> Source/JavaScriptCore/heap/MarkStack.cpp:562
> +	   if ((string->length() > 1) && !string->isRope() &&
string->tryHashConstLock()) {

Let's use a helper function named shouldTryHashConst, which does all our tests
for us:
- length > 1
- !isRope
- !isHashConstSingleton

> Source/JavaScriptCore/runtime/JSString.h:179
> +	   void releaseHashConstLock() { m_flags &= ~s_hashConstLock; }

This needs WTF::memoryBarrierBeforeUnlock().