[webkit-reviews] review granted: [Bug 190047] [JSC] Optimize Object.keys by caching own keys results in StructureRareData : [Attachment 357427] Patch

[bugzilla-daemon at webkit.org]

Saam Barati <sbarati at apple.com> has granted Yusuke Suzuki
<yusukesuzuki at slowstart.org>'s request for review:
Bug 190047: [JSC] Optimize Object.keys by caching own keys results in
StructureRareData
https://bugs.webkit.org/show_bug.cgi?id=190047

Attachment 357427: Patch

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




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

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

r=me. Nice!

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:2591
> +		       auto* immutableButterfly =
rareData->cachedOwnKeysConcurrently();
> +		       if (immutableButterfly && immutableButterfly !=
StructureRareData::cachedOwnKeysSentinel()) {

This should now just be:
if (auto* immutableButterfly = rareData->cachedOwnKeysConcurrently()) { ... }

> Source/JavaScriptCore/dfg/DFGConstantFoldingPhase.cpp:776
> +			       auto* immutableButterfly =
rareData->cachedOwnKeysConcurrently();
> +			       if (immutableButterfly && immutableButterfly !=
StructureRareData::cachedOwnKeysSentinel()) {

ditto

> Source/JavaScriptCore/runtime/JSImmutableButterfly.h:126
> +	   if (hasContiguous(indexingType())) {
> +	       for (unsigned index = 0; index < length; ++index)
> +		   toButterfly()->contiguous().at(this,
index).setStartingValue(JSValue());
> +	   }

Why not only do this for Object.keys? Seems like this might affect other
callers too? Seems like there should be some kind of ::create that has this in
its name?

> Source/JavaScriptCore/runtime/ObjectConstructor.cpp:939
> +			       // Cache the immutable butterfly!

nit: This comment has no purpose.

> Source/JavaScriptCore/runtime/StructureRareDataInlines.h:85
> +    WTF::storeStoreFence();

nit: Only needs to be done on the "else"