[webkit-reviews] review denied: [Bug 172413] [JSC] Map and Set constructors should have fast path for cloning : [Attachment 310848] Patch

[bugzilla-daemon at webkit.org]

Saam Barati <sbarati at apple.com> has denied Yusuke Suzuki
<utatane.tea at gmail.com>'s request for review:
Bug 172413: [JSC] Map and Set constructors should have fast path for cloning
https://bugs.webkit.org/show_bug.cgi?id=172413

Attachment 310848: Patch

https://bugs.webkit.org/attachment.cgi?id=310848&action=review




--- Comment #15 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 310848
  --> https://bugs.webkit.org/attachment.cgi?id=310848
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=310848&action=review

Mostly LGTM, but I think I found one bug, and have some overall comments. r-
b/c the bug.

> Source/JavaScriptCore/runtime/HashMapImpl.h:348
> +	   if ((Checked<uint32_t>(base->m_keyCount) * 2).unsafeGet() >=
powerOf2)
> +	       powerOf2 = (Checked<uint32_t>(powerOf2) * 2).unsafeGet();

How is this condition ever not true? Is it only when keyCount is 0 or 1?

Also, why *2 here, and not *4? After creating this set, if anybody adds
anything to it, we'll rehash and grow the table. Are we just guessing that
nobody adds to it?

> Source/JavaScriptCore/runtime/HashMapImpl.h:360
> +	   makeAndSetNewBuffer(exec, vm);
> +	   RETURN_IF_EXCEPTION(scope, void());
> +
> +	   m_head.set(vm, this, HashMapBucketType::create(vm));
> +	   m_tail.set(vm, this, HashMapBucketType::create(vm));
> +
> +	   m_head->setNext(vm, m_tail.get());
> +	   m_tail->setPrev(vm, m_head.get());
> +	   m_head->setDeleted(true);
> +	   m_tail->setDeleted(true);

Nit: This is identical to the other finishCreation. It's probably worth having
a helper function.

> Source/JavaScriptCore/runtime/HashMapImpl.h:460
> +    ALWAYS_INLINE void addNormalizedNonExistingForCloning(ExecState* exec,
JSValue key, JSValue value = JSValue())

Why does just calling add not work? This function seems nearly identical to the
regular add, it feels like there should be a way to combine them. Perhaps the
hash lookup loop could take a lambda that does returns a boolean indicating if
we can store to the current bucket.

> Source/JavaScriptCore/runtime/InternalFunction.h:51
> +    ALWAYS_INLINE static Structure* createSubclassStructure(ExecState*,
JSValue newTarget, Structure*);

I thought this needed to be marked ALWAYS_INLINE on the implementation of the
function, not declaration?

> Source/JavaScriptCore/runtime/InternalFunction.h:52
> +    JS_EXPORT_PRIVATE static Structure*
createSubclassStructureSlow(ExecState*, JSValue newTarget, Structure*);

this should be private

> Source/JavaScriptCore/runtime/JSSet.cpp:48
> +bool JSSet::isIteratorProtocolFastAndNonObservable()

The name of this function is misleading since returning true does not mean the
protocol is fast and non observable. Maybe this doesn't even need to be a
function. We can just combine this and "addFastAndNonObservable" into a single
function since they both have just one caller, and it's the same function.
(Same comments for the JSMap variant)

> Source/JavaScriptCore/runtime/JSSet.cpp:81
> +    // This is the fast case. Many sets will be an original set.
> +    if (structure == globalObject->setStructure())
> +	   return true;
> +
> +    if (structure->storedPrototype() != globalObject->jsSetPrototype())
> +	   return false;
> +
> +    return true;

This proof does not look correct. Consider:
```
let x = new Set;
x.add = 42;
```

> Source/JavaScriptCore/runtime/JSSet.h:85
> +template<typename To, typename From>
> +inline typename std::enable_if<std::is_same<typename
std::remove_pointer<To>::type, JSSet>::value, To>::type jsDynamicCast(VM&,
From* from)
> +{
> +    if (LIKELY(from->type() == JSSetType))
> +	   return static_cast<To>(from);
> +    return nullptr;
> +}
> +
> +template<>
> +inline JSSet* jsDynamicCast<JSSet*>(VM&, JSValue from)
> +{
> +    if (LIKELY(from.isCell() && from.asCell()->type() == JSSetType))
> +	   return static_cast<JSSet*>(from.asCell());
> +    return nullptr;
> +}

I'd mark JSSet as "final", and static assert that it's final here, otherwise we
could have a bad time if anybody ever inherits from them. This will future
proof the code with a static assert. (Same for JSMap)