Bug 158020 - [ESNext] Support trailing commas in function param lists
Summary: [ESNext] Support trailing commas in function param lists
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL: https://jeffmo.github.io/es-trailing-...
Keywords:
Depends on: 158268
Blocks:
  Show dependency treegraph
 
Reported: 2016-05-24 03:48 PDT by GSkachkov
Modified: 2016-06-06 13:32 PDT (History)
10 users (show)

See Also:


Attachments
Patch (3.95 KB, patch)
2016-05-24 10:40 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (5.85 KB, patch)
2016-05-24 11:14 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-yosemite (1.08 MB, application/zip)
2016-05-24 11:43 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews105 for mac-yosemite-wk2 (992.99 KB, application/zip)
2016-05-24 12:02 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews121 for ios-simulator-wk2 (709.90 KB, application/zip)
2016-05-24 12:08 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews116 for mac-yosemite (1.47 MB, application/zip)
2016-05-24 12:13 PDT, Build Bot
no flags Details
Patch (9.87 KB, patch)
2016-05-25 11:42 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (72.28 KB, patch)
2016-05-25 12:43 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (9.87 KB, patch)
2016-05-25 12:59 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (10.24 KB, patch)
2016-05-27 07:40 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (10.19 KB, patch)
2016-06-04 15:08 PDT, GSkachkov
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description GSkachkov 2016-05-24 03:48:11 PDT
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.
Comment 1 GSkachkov 2016-05-24 10:40:27 PDT
Created attachment 279670 [details]
Patch

Patch with only success cases. Later will provide more fail cases
Comment 2 GSkachkov 2016-05-24 11:14:20 PDT
Created attachment 279675 [details]
Patch

Patch with more tests
Comment 3 Build Bot 2016-05-24 11:43:35 PDT
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
Comment 4 Build Bot 2016-05-24 11:43:38 PDT
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
Comment 5 Build Bot 2016-05-24 12:02:05 PDT
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
Comment 6 Build Bot 2016-05-24 12:02:08 PDT
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
Comment 7 Build Bot 2016-05-24 12:08:24 PDT
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
Comment 8 Build Bot 2016-05-24 12:08:27 PDT
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
Comment 9 Build Bot 2016-05-24 12:13:19 PDT
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
Comment 10 Build Bot 2016-05-24 12:13:23 PDT
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
Comment 11 GSkachkov 2016-05-24 15:44:52 PDT
Comment on attachment 279675 [details]
Patch

Ohh, I forgot to check already existed tests
Comment 12 GSkachkov 2016-05-25 11:42:37 PDT
Created attachment 279787 [details]
Patch

Fixed tests
Comment 13 GSkachkov 2016-05-25 12:43:59 PDT
Created attachment 279792 [details]
Patch

Rebase
Comment 14 GSkachkov 2016-05-25 12:59:35 PDT
Created attachment 279795 [details]
Patch

Wrong patch was loaded
Comment 15 Keith Miller 2016-05-26 16:15:24 PDT
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);
Comment 16 GSkachkov 2016-05-27 07:40:07 PDT
Created attachment 279960 [details]
Patch

Check fixes
Comment 17 GSkachkov 2016-05-28 13:06:54 PDT
Landed in:
http://trac.webkit.org/changeset/201488
Comment 18 WebKit Commit Bot 2016-06-01 13:29:03 PDT
Re-opened since this is blocked by bug 158268
Comment 19 Ryosuke Niwa 2016-06-01 13:29:26 PDT
This patch caused 23% regression on JetStream's crypto-md5 test.
Comment 20 GSkachkov 2016-06-04 15:08:17 PDT
Created attachment 280527 [details]
Patch

trying to fix performance issue
Comment 21 Saam Barati 2016-06-04 16:04:20 PDT
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?
Comment 22 GSkachkov 2016-06-05 05:45:54 PDT
(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
Comment 23 GSkachkov 2016-06-05 05:50:47 PDT
(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?
Comment 24 GSkachkov 2016-06-06 13:07:03 PDT
Comment on attachment 280527 [details]
Patch

Let's try and see result
Comment 25 WebKit Commit Bot 2016-06-06 13:32:13 PDT
Comment on attachment 280527 [details]
Patch

Clearing flags on attachment: 280527

Committed r201725: <http://trac.webkit.org/changeset/201725>
Comment 26 WebKit Commit Bot 2016-06-06 13:32:19 PDT
All reviewed patches have been landed.  Closing bug.