Bug 172798 - [JSC] Implement String.prototype.concat in JS builtins
Summary: [JSC] Implement String.prototype.concat in JS builtins
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-05-31 21:55 PDT by Yusuke Suzuki
Modified: 2017-06-01 09:54 PDT (History)
8 users (show)

See Also:


Attachments
Patch (8.57 KB, patch)
2017-05-31 21:58 PDT, Yusuke Suzuki
sam: review+
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-05-31 21:55:53 PDT
[JSC] Implement String.prototype.concat in JS builtins
Comment 1 Yusuke Suzuki 2017-05-31 21:58:03 PDT
Created attachment 311683 [details]
Patch
Comment 2 Sam Weinig 2017-05-31 22:05:00 PDT
Comment on attachment 311683 [details]
Patch

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

> Source/JavaScriptCore/builtins/StringPrototype.js:311
> +function stringConcatSlowPath()
> +{
> +    "use strict";

Is using arguments faster than using rest parameters? (I asked Saam recently and he was on the fence I think, or maybe just hadn't benchmarked it).
Comment 3 Yusuke Suzuki 2017-05-31 22:15:19 PDT
Comment on attachment 311683 [details]
Patch

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

Thanks!

>> Source/JavaScriptCore/builtins/StringPrototype.js:311
>> +    "use strict";
> 
> Is using arguments faster than using rest parameters? (I asked Saam recently and he was on the fence I think, or maybe just hadn't benchmarked it).

One stupid problem is that our builtin JS system currently cannot accept default and rest parameters :(
We should extend our builtin JS generators (python scripts) to accept these forms.
It should be easy & simple task.

But, even if we do that, for builtin JS, I think we can introduce & use much better one: extending `@argument()` to accept arbitrary runtime integers instead of constant integers.
By doing so, we can avoid allocation for arguments / rest parameter arrays completely for builtin JS code.
So, personally, rather than extending builtin JS generators, I think we should do this thing.

So, I think we do not need to use rest parameters & arguments in JS builtin code when we do the above optimization.
Comment 4 Yusuke Suzuki 2017-05-31 22:18:33 PDT
Committed r217648: <http://trac.webkit.org/changeset/217648>
Comment 5 Yusuke Suzuki 2017-05-31 23:52:02 PDT
(In reply to Yusuke Suzuki from comment #4)
> Committed r217648: <http://trac.webkit.org/changeset/217648>

Need to rebaseline error messages. WIP...
Comment 6 Yusuke Suzuki 2017-06-01 00:27:14 PDT
Committed r217649: <http://trac.webkit.org/changeset/217649>
Comment 7 Saam Barati 2017-06-01 09:47:03 PDT
(In reply to Yusuke Suzuki from comment #3)
> Comment on attachment 311683 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=311683&action=review
> 
> Thanks!
> 
> >> Source/JavaScriptCore/builtins/StringPrototype.js:311
> >> +    "use strict";
> > 
> > Is using arguments faster than using rest parameters? (I asked Saam recently and he was on the fence I think, or maybe just hadn't benchmarked it).
> 
> One stupid problem is that our builtin JS system currently cannot accept
> default and rest parameters :(
> We should extend our builtin JS generators (python scripts) to accept these
> forms.
> It should be easy & simple task.
Out of curiosity, why does this break the script? Does it try to validate syntax somehow?

> 
> But, even if we do that, for builtin JS, I think we can introduce & use much
> better one: extending `@argument()` to accept arbitrary runtime integers
> instead of constant integers.
> By doing so, we can avoid allocation for arguments / rest parameter arrays
> completely for builtin JS code.
> So, personally, rather than extending builtin JS generators, I think we
> should do this thing.
I agree.

> 
> So, I think we do not need to use rest parameters & arguments in JS builtin
> code when we do the above optimization.
Comment 8 Yusuke Suzuki 2017-06-01 09:54:26 PDT
Comment on attachment 311683 [details]
Patch

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

>>>> Source/JavaScriptCore/builtins/StringPrototype.js:311
>>>> +    "use strict";
>>> 
>>> Is using arguments faster than using rest parameters? (I asked Saam recently and he was on the fence I think, or maybe just hadn't benchmarked it).
>> 
>> One stupid problem is that our builtin JS system currently cannot accept default and rest parameters :(
>> We should extend our builtin JS generators (python scripts) to accept these forms.
>> It should be easy & simple task.
>> 
>> But, even if we do that, for builtin JS, I think we can introduce & use much better one: extending `@argument()` to accept arbitrary runtime integers instead of constant integers.
>> By doing so, we can avoid allocation for arguments / rest parameter arrays completely for builtin JS code.
>> So, personally, rather than extending builtin JS generators, I think we should do this thing.
>> 
>> So, I think we do not need to use rest parameters & arguments in JS builtin code when we do the above optimization.
> 
> Out of curiosity, why does this break the script? Does it try to validate syntax somehow?

It's just a missing regexp in builtin generator python. We should extend the existing regexp to accept parameters including default and rest params.