[JSC] Optimize Object.keys by using careful array allocation
Created attachment 320344 [details] Patch
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.
Created attachment 320345 [details] Patch
Created attachment 320346 [details] Patch
Created attachment 320347 [details] Patch
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.
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.
Committed r221853: <http://trac.webkit.org/changeset/221853>
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.
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?
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.
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.
(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. 👍🏽
<rdar://problem/34693450>