Bug 158020 – [ESNext] Support trailing commas in function param lists

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[GSkachkov]

Trailing commas in function param lists is part ESNext specs(https://jeffmo.github.io/es-trailing-function-commas/).
Currently it on the stage 3, so I think it is safe to implement it in JSC now. Also some browser already implemented it.

[GSkachkov]

Created attachment 279670 [details]
Patch

Patch with only success cases. Later will provide more fail cases

[GSkachkov]

Created attachment 279675 [details]
Patch

Patch with more tests

[Build Bot]

Comment on attachment 279675 [details]
Patch

Attachment 279675 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/1375991

New failing tests:
js/parser-syntax-check.html
sputnik/Conformance/13_Function_Definition/S13_A5.html

[Build Bot]

Created attachment 279679 [details]
Archive of layout-test-results from ews102 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 279675 [details]
Patch

Attachment 279675 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/1376059

New failing tests:
js/parser-syntax-check.html
sputnik/Conformance/13_Function_Definition/S13_A5.html

[Build Bot]

Created attachment 279681 [details]
Archive of layout-test-results from ews105 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 279675 [details]
Patch

Attachment 279675 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/1376060

New failing tests:
js/parser-syntax-check.html
sputnik/Conformance/13_Function_Definition/S13_A5.html

[Build Bot]

Created attachment 279683 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.4

[Build Bot]

Comment on attachment 279675 [details]
Patch

Attachment 279675 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1376065

New failing tests:
js/parser-syntax-check.html
sputnik/Conformance/13_Function_Definition/S13_A5.html

[Build Bot]

Created attachment 279684 [details]
Archive of layout-test-results from ews116 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[GSkachkov]

Comment on attachment 279675 [details]
Patch

Ohh, I forgot to check already existed tests

[GSkachkov]

Created attachment 279787 [details]
Patch

Fixed tests

[GSkachkov]

Created attachment 279792 [details]
Patch

Rebase

[GSkachkov]

Created attachment 279795 [details]
Patch

Wrong patch was loaded

[Keith Miller]

Comment on attachment 279795 [details]
Patch

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

r=me with comments.

> Source/JavaScriptCore/tests/stress/trailing-comma-in-function-paramters.js:54
> +test(eval('(function(a, b, c){ return a + b + c; })("A", "B", "C",)'), 'ABC');

I think it would also be useful to add the following tests:

test((function(a,) { return arguments.length; })(), 1);
test((function(a,) { }).length, 1);

[GSkachkov]

Created attachment 279960 [details]
Patch

Check fixes

[GSkachkov]

Landed in:
http://trac.webkit.org/changeset/201488

[WebKit Commit Bot]

Re-opened since this is blocked by bug 158268

[Ryosuke Niwa]

This patch caused 23% regression on JetStream's crypto-md5 test.

[GSkachkov]

Created attachment 280527 [details]
Patch

trying to fix performance issue

[Saam Barati]

Comment on attachment 280527 [details]
Patch

Let's try landing it. Can you also run sunspider locally to see if you see any perf regressions?

[GSkachkov]

(In reply to comment #21)
> Comment on attachment 280527 [details]
> Patch
> 
> Let's try landing it. Can you also run sunspider locally to see if you see
> any perf regressions?

See results:

VMs tested:
"withoutfix" at /Users/Developer/Projects/Webkit2/WebKitBuild/trailing_comma_perf/Release/jsc
"withfix" at /Users/Developer/Projects/Webkit2/WebKitBuild/trailing_comma/Release/jsc

Collected 4 samples per benchmark/VM, with 4 VM invocations per benchmark. Ran 8 benchmark iterations, and
measured the total time of those iterations, for each sample. Emitted a call to gc() between sample
measurements. Used 32 benchmark iterations per VM invocation for warm-up. Used the jsc-specific
preciseTime() function to get microsecond-level timing. Reporting benchmark execution times with 95%
confidence intervals in milliseconds.

                                  withoutfix                 withfix                                      

3d-cube                        83.8227+-24.3184          76.9651+-3.2111          might be 1.0891x faster
3d-morph                       67.9703+-6.1130     ?     69.4963+-4.5539        ? might be 1.0225x slower
3d-raytrace                    78.9442+-4.0375     ?     81.5590+-4.3219        ? might be 1.0331x slower
access-binary-trees            38.6298+-1.6829     ?     39.4618+-3.5157        ? might be 1.0215x slower
access-fannkuch               117.1779+-12.1635    ?    117.5690+-4.4082        ?
access-nbody                   40.1360+-22.6709          32.2021+-0.6917          might be 1.2464x faster
access-nsieve                  36.1190+-3.4944           35.7054+-2.2991          might be 1.0116x faster
bitops-3bit-bits-in-byte       14.3125+-0.6476     ?     14.4402+-1.7659        ?
bitops-bits-in-byte            38.8101+-2.2190           38.4263+-5.0924        
bitops-bitwise-and             22.1407+-2.9088           21.9016+-2.3408          might be 1.0109x faster
bitops-nsieve-bits             51.7688+-3.1459           50.5560+-3.9563          might be 1.0240x faster
controlflow-recursive          56.6677+-16.0759          52.8937+-3.5335          might be 1.0714x faster
crypto-aes                     57.3966+-6.6903     ?     75.6738+-37.2029       ? might be 1.3184x slower
crypto-md5                     37.9637+-3.9742     ?     38.3545+-2.0614        ? might be 1.0103x slower
crypto-sha1                    37.6180+-1.6728           37.4700+-3.6662        
date-format-tofte             107.7530+-9.8035          103.5540+-7.2543          might be 1.0405x faster
date-format-xparb             119.6010+-4.8767          110.3923+-20.2829         might be 1.0834x faster
math-cordic                    38.0562+-2.3593     ?     45.7181+-31.6069       ? might be 1.2013x slower
math-partial-sums              61.3655+-10.2370          58.5570+-5.7249          might be 1.0480x faster
math-spectral-norm             26.5539+-0.4367     ?     27.1837+-0.9924        ? might be 1.0237x slower
regexp-dna                     72.3824+-0.6244     ?     74.6293+-4.1073        ? might be 1.0310x slower
string-base64                  61.9043+-9.7523     ?     64.7947+-12.5870       ? might be 1.0467x slower
string-fasta                   84.9805+-7.7918     ?     85.1760+-5.2455        ?
string-tagcloud               114.4025+-15.2375         108.3253+-8.7008          might be 1.0561x faster
string-unpack-code            220.5273+-18.0041    ?    223.9689+-8.9968        ? might be 1.0156x slower
string-validate-input          72.3325+-33.0462          55.1055+-1.3751          might be 1.3126x faster

<arithmetic>                   67.6668+-1.1993           66.9261+-2.3691          might be 1.0111x faster

[GSkachkov]

(In reply to comment #21)
> Comment on attachment 280527 [details]
> Patch
> 
> Let's try landing it. Can you also run sunspider locally to see if you see
> any perf regressions?

Is result ok? can I land patch?

[GSkachkov]

Comment on attachment 280527 [details]
Patch

Let's try and see result

[WebKit Commit Bot]

Comment on attachment 280527 [details]
Patch

Clearing flags on attachment: 280527

Committed r201725: <http://trac.webkit.org/changeset/201725>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.