Bug 176654 – [JSC] Optimize Object.keys by using careful array allocation

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Yusuke Suzuki]

[JSC] Optimize Object.keys by using careful array allocation

[Yusuke Suzuki]

Created attachment 320344 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 320344 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:13
> +        but it rarely appears.

Then, ProxyObject case goes to the generic path.

[Yusuke Suzuki]

Created attachment 320345 [details]
Patch

[Yusuke Suzuki]

Created attachment 320346 [details]
Patch

[Yusuke Suzuki]

Created attachment 320347 [details]
Patch

[Darin Adler]

Comment on attachment 320347 [details]
Patch

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

> Source/JavaScriptCore/runtime/ObjectConstructor.h:125
> -JSArray* ownPropertyKeys(ExecState*, JSObject*, PropertyNameMode, DontEnumPropertiesMode);
> +JSValue ownPropertyKeys(ExecState*, JSObject*, PropertyNameMode, DontEnumPropertiesMode);

Why this change? Nothing in the change log explains it. The function still seems to always return a JSArray*. Is it a performance optimization to move the conversion from JSArray* to JSValue inside the function?

If the change isn’t needed, then it seems useful to follow the general C++ coding style rule of not widening types unnecessarily in case the narrower type can be of use for performance optimization or otherwise useful in the future.

[Yusuke Suzuki]

Comment on attachment 320347 [details]
Patch

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

>> Source/JavaScriptCore/runtime/ObjectConstructor.h:125
>> +JSValue ownPropertyKeys(ExecState*, JSObject*, PropertyNameMode, DontEnumPropertiesMode);
> 
> Why this change? Nothing in the change log explains it. The function still seems to always return a JSArray*. Is it a performance optimization to move the conversion from JSArray* to JSValue inside the function?
> 
> If the change isn’t needed, then it seems useful to follow the general C++ coding style rule of not widening types unnecessarily in case the narrower type can be of use for performance optimization or otherwise useful in the future.

I thought it is not so good that JSValue::encode(nullptr as JSCell*) happens if the error occurrs. But either is ok to me.

[Yusuke Suzuki]

Committed r221853: <http://trac.webkit.org/changeset/221853>

[Yusuke Suzuki]

https://arewefastyet.com/#machine=29&view=single&suite=six-speed&subtest=for-of-object-es6
https://arewefastyet.com/#machine=29&view=single&suite=six-speed&subtest=object-assign-es5

Good results. I think we should set up enumeration cache for Object.keys case.

[Saam Barati]

Comment on attachment 320347 [details]
Patch

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

> Source/JavaScriptCore/runtime/ObjectConstructor.cpp:402
> +        values->putDirectIndex(exec, index++, value);

Isn't this an observable change in JS semantics?

[Darin Adler]

Comment on attachment 320347 [details]
Patch

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

>>> Source/JavaScriptCore/runtime/ObjectConstructor.h:125
>>> +JSValue ownPropertyKeys(ExecState*, JSObject*, PropertyNameMode, DontEnumPropertiesMode);
>> 
>> Why this change? Nothing in the change log explains it. The function still seems to always return a JSArray*. Is it a performance optimization to move the conversion from JSArray* to JSValue inside the function?
>> 
>> If the change isn’t needed, then it seems useful to follow the general C++ coding style rule of not widening types unnecessarily in case the narrower type can be of use for performance optimization or otherwise useful in the future.
> 
> I thought it is not so good that JSValue::encode(nullptr as JSCell*) happens if the error occurrs. But either is ok to me.

Not different from returning "JSValue { }"; the JSValue(JSCell*) constructor handles nullptr in exactly that way. If we wanted to optimize by avoiding the branch, I don’t think returning JSValue accomplishes that.

[Yusuke Suzuki]

Comment on attachment 320347 [details]
Patch

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

>> Source/JavaScriptCore/runtime/ObjectConstructor.cpp:402
>> +        values->putDirectIndex(exec, index++, value);
> 
> Isn't this an observable change in JS semantics?

I changed these code to align them to the spec (previously, our implementation does not follow the spec.)
I'll add the test for this.

[Saam Barati]

(In reply to Yusuke Suzuki from comment #12)
> Comment on attachment 320347 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=320347&action=review
> 
> >> Source/JavaScriptCore/runtime/ObjectConstructor.cpp:402
> >> +        values->putDirectIndex(exec, index++, value);
> > 
> > Isn't this an observable change in JS semantics?
> 
> I changed these code to align them to the spec (previously, our
> implementation does not follow the spec.)
> I'll add the test for this.

👍🏽

[Radar WebKit Bug Importer]

<rdar://problem/34693450>