Bug 173414 - [JSC] Implement Object.assign in C++
Summary: [JSC] Implement Object.assign in C++
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords:
Depends on:
Blocks: 173416
  Show dependency treegraph
 
Reported: 2017-06-15 05:09 PDT by Yusuke Suzuki
Modified: 2017-06-16 10:32 PDT (History)
7 users (show)

See Also:


Attachments
Patch (9.04 KB, patch)
2017-06-15 05:41 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (9.29 KB, patch)
2017-06-15 05:43 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (13.71 KB, patch)
2017-06-15 07:01 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (13.54 KB, patch)
2017-06-15 07:01 PDT, Yusuke Suzuki
sbarati: review+
Details | Formatted Diff | Diff
Patch for landing (14.62 KB, patch)
2017-06-15 12:27 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2017-06-15 05:09:33 PDT
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++.
Comment 1 Yusuke Suzuki 2017-06-15 05:41:37 PDT
Created attachment 312972 [details]
Patch
Comment 2 Yusuke Suzuki 2017-06-15 05:43:25 PDT
Created attachment 312973 [details]
Patch
Comment 3 Build Bot 2017-06-15 06:23:38 PDT
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
Comment 4 Yusuke Suzuki 2017-06-15 07:01:12 PDT
Created attachment 312978 [details]
Patch
Comment 5 Yusuke Suzuki 2017-06-15 07:01:53 PDT
Created attachment 312979 [details]
Patch
Comment 6 Yusuke Suzuki 2017-06-15 11:36:06 PDT
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 7 Saam Barati 2017-06-15 12:10:56 PDT
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 8 Yusuke Suzuki 2017-06-15 12:21:36 PDT
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.
Comment 9 Yusuke Suzuki 2017-06-15 12:27:07 PDT
Created attachment 312996 [details]
Patch for landing
Comment 10 Yusuke Suzuki 2017-06-15 12:37:26 PDT
Committed r218348: <http://trac.webkit.org/changeset/218348>
Comment 12 Sam Weinig 2017-06-16 10:15:03 PDT
(In reply to Yusuke Suzuki from comment #11)
> https://perf.webkit.org/v3/#/charts?since=1496979192778&paneList=((18-906))
> 
> Win!

Nice!
Comment 13 Sam Weinig 2017-06-16 10:16:09 PDT
(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
Comment 14 Yusuke Suzuki 2017-06-16 10:32:02 PDT
(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.