Implementing Object.assign in JS is not so good compared to C++ version because, 1. JS version allocates JS array for object own keys. And we allocate JSString / Symbol for each key. But basically, they can be handled as UniquedStringImpl in C++. Allocating these cells are wasteful. 2. While implementing builtins in JS offers some good type speculation chances, Object.assign is inherently super polymorphic. So JS's type profile doesn't help well. 3. We have a chance to introduce various fast path for Object.assign in C++.
Created attachment 312972 [details] Patch
Created attachment 312973 [details] Patch
Comment on attachment 312973 [details] Patch Attachment 312973 [details] did not pass jsc-ews (mac): Output: http://webkit-queues.webkit.org/results/3935307 New failing tests: ChakraCore.yaml/ChakraCore/test/es6/object-assign.js.default
Created attachment 312978 [details] Patch
Created attachment 312979 [details] Patch
btw, it would make Speedometer2 Redux one fast since it heavily uses Object.assign. When I previously optimized Object.assign a bit, that shows good improvement.
Comment on attachment 312979 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=312979&action=review r=me > Source/JavaScriptCore/runtime/JSObjectInlines.h:231 > +ALWAYS_INLINE bool JSObject::putInline(ExecState* exec, PropertyName propertyName, JSValue value, PutPropertySlot& slot) > +{ > + auto putMethod = methodTable(exec->vm())->put; > + if (LIKELY(putMethod == JSObject::put)) > + return JSObject::putInlineForJSObject(this, exec, propertyName, value, slot); > + return putMethod(this, exec, propertyName, value, slot); > +} > + JSValue::putInline, which calls this, also performs this same branch. Please remove it from JSValue::putInline since it can just directly call into this now. > Source/JavaScriptCore/runtime/ObjectConstructor.cpp:326 > + RETURN_IF_EXCEPTION(scope, encodedJSValue()); style nit: { } instead of encodedJSValue() (and below in the function) > Source/JavaScriptCore/runtime/ObjectConstructor.cpp:343 > + nit: no need to exception check here given how this is used below.
Comment on attachment 312979 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=312979&action=review Thanks! >> Source/JavaScriptCore/runtime/JSObjectInlines.h:231 >> + > > JSValue::putInline, which calls this, also performs this same branch. Please remove it from JSValue::putInline since it can just directly call into this now. I think we should move this function to JSCell (like, JSCell::putInline) b/c JSValue's JSCell* can include other types like JSString. Fixed. >> Source/JavaScriptCore/runtime/ObjectConstructor.cpp:326 >> + RETURN_IF_EXCEPTION(scope, encodedJSValue()); > > style nit: { } instead of encodedJSValue() > (and below in the function) Changed, thanks :) >> Source/JavaScriptCore/runtime/ObjectConstructor.cpp:343 >> + > > nit: no need to exception check here given how this is used below. Right! Dropped.
Created attachment 312996 [details] Patch for landing
Committed r218348: <http://trac.webkit.org/changeset/218348>
https://perf.webkit.org/v3/#/charts?since=1496979192778&paneList=((18-906)) Win!
(In reply to Yusuke Suzuki from comment #11) > https://perf.webkit.org/v3/#/charts?since=1496979192778&paneList=((18-906)) > > Win! Nice!
(In reply to Sam Weinig from comment #12) > (In reply to Yusuke Suzuki from comment #11) > > https://perf.webkit.org/v3/#/charts?since=1496979192778&paneList=((18-906)) > > > > Win! > > Nice! I really enjoy the timing of this given the following tweet - https://twitter.com/Catfish_Man/status/875127498933129217
(In reply to Sam Weinig from comment #13) > (In reply to Sam Weinig from comment #12) > > (In reply to Yusuke Suzuki from comment #11) > > > https://perf.webkit.org/v3/#/charts?since=1496979192778&paneList=((18-906)) > > > > > > Win! > > > > Nice! > > I really enjoy the timing of this given the following tweet - > https://twitter.com/Catfish_Man/status/875127498933129217 Yeah, implementing builtin in JS or C++ highly depends on the complexity of the target function, opportunity for inlining and Object Allocating Sinking, IC's effectiveness etc. We need to choose one of them (JS or C++) carefully. It will offer great improvement that another cannot achieve! In String.prototype.concat case, it is super simple. And its operation is already highly optimized in JS JIT. It encourages inlining, type speculation, DFG/FTL optimizations etc. It is worth implementing String.prototype.concat in JS. On the other hand, Object.assign is a bit complex and polymorphic. Inlining, type speculation and JS JIT tier's optimization (including IC) do not help much for Object.assign. And by implementing it in C++, we can avoid several allocations. C++ implementation leads to high performance in this case.