Bug 173414 – [JSC] Implement Object.assign in C++

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Yusuke Suzuki]

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++.

[Yusuke Suzuki]

Created attachment 312972 [details]
Patch

[Yusuke Suzuki]

Created attachment 312973 [details]
Patch

[Build Bot]

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

[Yusuke Suzuki]

Created attachment 312978 [details]
Patch

[Yusuke Suzuki]

Created attachment 312979 [details]
Patch

[Yusuke Suzuki]

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.

[Saam Barati]

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.

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

Created attachment 312996 [details]
Patch for landing

[Yusuke Suzuki]

Committed r218348: <http://trac.webkit.org/changeset/218348>

[Yusuke Suzuki]

https://perf.webkit.org/v3/#/charts?since=1496979192778&paneList=((18-906))

Win!

[Sam Weinig]

(In reply to Yusuke Suzuki from comment #11)
> https://perf.webkit.org/v3/#/charts?since=1496979192778&paneList=((18-906))
> 
> Win!

Nice!

[Sam Weinig]

(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

[Yusuke Suzuki]

(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.