[webkit-reviews] review denied: [Bug 174355] [JSC] Optimize Map iteration with intrinsic : [Attachment 318737] 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 174355: [JSC] Optimize Map iteration with intrinsic
https://bugs.webkit.org/show_bug.cgi?id=174355

Attachment 318737: Patch

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




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

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

LGTM, but r- because I think I found a few real bugs. Also other comments to
follow.

> Source/JavaScriptCore/builtins/MapIteratorPrototype.js:44
> +    return { done, value };

I think this is wrong, for example:
```
let x = new Map;
let i = x[Symbol.iterator]();
let o = i.next();
Object.hasOwnProperty(o, "value") // false
```
Let's add a test for this and for the Set version below.

> Source/JavaScriptCore/builtins/MapPrototype.js:29
> +{

"use strict"?

> Source/JavaScriptCore/builtins/MapPrototype.js:50
> +    if (!@isMap(this))

Does this work even when subclassing Map?

> Source/JavaScriptCore/builtins/SetPrototype.js:29
> +{

"use strict"?

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:1059
> +	   forNode(node).setType(m_graph, SpecCell);

Maybe it's worth giving the exact structure here since it's constant?

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:2903
> +	   if (argumentCountIncludingThis != 2)
> +	       return false;

Why isn't this an assert? Don't we only call this from our builtins?

> Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp:762
> +	       setPrediction(SpecCell);

Why not SpecCellOther here?

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:10190
> +    m_jit.loadPtr(MacroAssembler::Address(mapGPR,
HashMapImpl<HashMapBucket<HashMapBucketDataKey>>::offsetOfHead()), bucketGPR);

I think it's worth a static_assert that JSSet::offsetOfHead() ==
JSMap::offsetOfHead() == your offset here.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:10202
> +    m_jit.loadPtr(MacroAssembler::Address(bucketGPR,
HashMapBucket<HashMapBucketDataKeyValue>::offsetOfNext()), resultGPR);

ditto about static assert

> Source/JavaScriptCore/ftl/FTLAbstractHeapRepository.h:120
> +    macro(HashMapImpl_head, 
HashMapImpl<HashMapBucket<HashMapBucketDataKey>>::offsetOfHead()) \
>      macro(HashMapBucket_value,
HashMapBucket<HashMapBucketDataKeyValue>::offsetOfValue()) \
>      macro(HashMapBucket_key,
HashMapBucket<HashMapBucketDataKeyValue>::offsetOfKey()) \
> +    macro(HashMapBucket_next,
HashMapBucket<HashMapBucketDataKeyValue>::offsetOfNext()) \
> +    macro(HashMapBucket_deleted,
HashMapBucket<HashMapBucketDataKeyValue>::offsetOfDeleted()) \

Please add static_asserts that these are the same for JSSet and JSMap and their
respective buckets

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:8285
> +	   if (m_node->bucketOwnerUseKind() == MapObjectUse)
> +	       noBucketResult =
m_out.anchor(weakPointer(vm().sentinelMapBucket.get()));
> +	   else
> +	       noBucketResult =
m_out.anchor(weakPointer(vm().sentinelSetBucket.get()));

This is wrong, you need to branch to something that sets the noBucketResult
here. 
It'd be good if you add a test that catches this bug.

> Source/JavaScriptCore/runtime/JSGlobalObject.cpp:772
> +    createMapIteratorPrivateFunction->putDirect(vm,
vm.propertyNames->prototype, mapIteratorPrototype);

I think this can be putDirectWithoutTransition.

> Source/JavaScriptCore/runtime/JSGlobalObject.cpp:775
> +    createSetIteratorPrivateFunction->putDirect(vm,
vm.propertyNames->prototype, setIteratorPrototype);

ditto

> Source/JavaScriptCore/runtime/JSMapIterator.cpp:34
> +const ClassInfo JSMapIterator::s_info = { "Map Iterator", nullptr, nullptr,
nullptr, CREATE_METHOD_TABLE(JSMapIterator) };

why nullptr instead of JSCell::info?

> Source/JavaScriptCore/runtime/JSSetIterator.cpp:34
> +const ClassInfo JSSetIterator::s_info = { "Set Iterator", nullptr, nullptr,
nullptr, CREATE_METHOD_TABLE(JSSetIterator) };

ditto

> Source/JavaScriptCore/runtime/JSSetIterator.h:34
> +// Now, it is only used for serialization.

what do you mean here?