Bug 160355 – JSVALUE64: Pass arguments in platform argument registers when making JavaScript calls

• Status: REOPENED
• Component: JavaScriptCore

[Michael Saboff]

Currently, function arguments are passed on the stack when JavaScript code makes a call to another JavaScript or Native function.  Most platform calling conventions define registers used to pass arguments as a way of reducing memory operations.  It makes sense to use these argument registers when making calls from JavaScript code at least on 64 bit platforms.  Argument registers would be used to pass the "callee", argument count in registers along with as many function arguments that fit in the remaining argument registers.

It makes less sense for 32 bit platforms for two reasons.  First, the 32 bit platforms we support have at most 4 argument registers.  Second, JSValues require two registers, one for the tag and another for the payload.  Taken together, we would only be able to only pass the "this" value in registers on 32 bit platforms along with the callee and argument count.

It is important to note that all JavaScriptCore tiers pass arguments in registers when calling out to C++ helper functions, in accordance with platform calling conventions.  The work described here is to expand the use of argument registers when making JavaScript calls.  This work will still allow for arguments to be passed on the stack, but that passing arguments via registers would be supported and the "fast path" in the DFG and FTL tiers.  The LLInt interpreter and code generated by the baseline JIT would continue to operate expecting arguments on the stack, but provide an additional entry point that would spill arguments to the stack before continuing as they do now.

[Radar WebKit Bug Importer]

<rdar://problem/27615788>

[Michael Saboff]

Created attachment 294295 [details]
Work in Progress

Compiles and runs on X86_64 and ARM64 bit platforms.  Passed JS regression tests debug.

Performance is neutral to slightly faster on most benchmarks.  It seems to spend more time compiling.  Investigating that now.

[Michael Saboff]

It appears that prediction logic for GetArgumentRegister nodes has some bugs in it when compared to SetArgument / GetLocal node pairs.  This leads to extra OSR exits and therefore recompiles.

The compile time with this patch can be slightly longer, but that doesn't seem to be responsible for much slowdown.

[Michael Saboff]

Created attachment 294722 [details]
Fixes to argument predictions

[Michael Saboff]

Performance results with the latest patch:

VMs tested:
"Baseline" at /Volumes/Data/src/webkit.work/WebKitBuild/Release/JavaScriptCore.framework/Versions/A/Resources/jsc
"RegCalls" at /Volumes/Data/src/webkit/WebKitBuild/Release/JavaScriptCore.framework/Versions/A/Resources/jsc

Collected 72 samples per benchmark/VM, with 12 VM invocations per benchmark. Emitted a call to gc() between sample
measurements. Used 1 benchmark iteration 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.

                                                 Baseline                  RegCalls                                     
SunSpider:
   3d-cube                                    6.1100+-0.0887     !      6.3230+-0.0932        ! definitely 1.0349x slower
   3d-morph                                   6.2299+-0.0461     !      6.3448+-0.0649        ! definitely 1.0184x slower
   3d-raytrace                                7.4219+-0.0779     !      7.6257+-0.0830        ! definitely 1.0275x slower
   access-binary-trees                        2.3965+-0.0283     ?      2.4229+-0.0324        ? might be 1.0110x slower
   access-fannkuch                            6.8164+-0.0482            6.7816+-0.0566        
   access-nbody                               3.2461+-0.0321     ?      3.2987+-0.0346        ? might be 1.0162x slower
   access-nsieve                              3.8305+-0.0305     ?      3.8597+-0.0338        ?
   bitops-3bit-bits-in-byte                   1.3746+-0.0209     !      1.7606+-0.0199        ! definitely 1.2808x slower
   bitops-bits-in-byte                        3.0005+-0.0201     ^      2.8661+-0.0278        ^ definitely 1.0469x faster
   bitops-bitwise-and                         2.4026+-0.0220     ?      2.4039+-0.0275        ?
   bitops-nsieve-bits                         3.8619+-0.0318     ?      3.9092+-0.0279        ? might be 1.0123x slower
   controlflow-recursive                      2.8513+-0.0239     !      2.9298+-0.0265        ! definitely 1.0275x slower
   crypto-aes                                 5.9606+-0.0397     !      6.0883+-0.0466        ! definitely 1.0214x slower
   crypto-md5                                 3.1065+-0.0427     ?      3.1870+-0.0506        ? might be 1.0259x slower
   crypto-sha1                                3.8182+-0.0262     !      4.0426+-0.0313        ! definitely 1.0588x slower
   date-format-tofte                         10.9435+-0.0803     !     11.3206+-0.0693        ! definitely 1.0345x slower
   date-format-xparb                          6.0358+-0.0553     ?      6.1096+-0.0415        ? might be 1.0122x slower
   math-cordic                                3.4835+-0.0272     !      3.5906+-0.0308        ! definitely 1.0307x slower
   math-partial-sums                          5.3341+-0.0397     ?      5.4050+-0.0419        ? might be 1.0133x slower
   math-spectral-norm                         2.4414+-0.0211     ?      2.4719+-0.0224        ? might be 1.0125x slower
   regexp-dna                                 7.7955+-0.0589            7.7953+-0.0741        
   string-base64                              6.0927+-0.1020     ^      5.9099+-0.0643        ^ definitely 1.0309x faster
   string-fasta                               6.8490+-0.0611     ?      6.9392+-0.0582        ? might be 1.0132x slower
   string-tagcloud                           10.2968+-0.0557     !     10.7282+-0.0769        ! definitely 1.0419x slower
   string-unpack-code                        22.2811+-0.0958     !     22.7592+-0.1541        ! definitely 1.0215x slower
   string-validate-input                      5.1730+-0.0351     !      5.2457+-0.0332        ! definitely 1.0140x slower

   <arithmetic>                               5.7367+-0.0181     !      5.8507+-0.0219        ! definitely 1.0199x slower

                                                 Baseline                  RegCalls                                     
LongSpider:
   3d-cube                                 1001.1409+-3.2410     !   1020.7687+-5.9024        ! definitely 1.0196x slower
   3d-morph                                 731.0439+-1.3206     ?    731.4462+-3.1071        ?
   3d-raytrace                              604.4466+-2.2632     ?    604.8496+-1.7332        ?
   access-binary-trees                      992.6355+-4.2873     ^    977.0483+-2.5367        ^ definitely 1.0160x faster
   access-fannkuch                          314.9290+-3.1567     !    321.7434+-3.0624        ! definitely 1.0216x slower
   access-nbody                             665.5799+-1.3557          662.1875+-4.5029        
   access-nsieve                            403.0147+-2.5303          402.6175+-3.5093        
   bitops-3bit-bits-in-byte                  36.5278+-0.2076     !     37.0868+-0.1299        ! definitely 1.0153x slower
   bitops-bits-in-byte                      123.7846+-1.0265          123.6671+-0.5522        
   bitops-nsieve-bits                       444.7577+-1.1756     ^    439.1797+-1.2388        ^ definitely 1.0127x faster
   controlflow-recursive                    500.6734+-2.7890     ^    489.6150+-2.4510        ^ definitely 1.0226x faster
   crypto-aes                               703.6378+-2.0234     !    714.9169+-1.3944        ! definitely 1.0160x slower
   crypto-md5                               568.3618+-3.3815     !    607.5667+-1.9879        ! definitely 1.0690x slower
   crypto-sha1                              763.2085+-2.2981     !    773.6934+-5.7391        ! definitely 1.0137x slower
   date-format-tofte                        543.5079+-2.3439          542.9402+-2.4089        
   date-format-xparb                        805.9694+-9.9023     ?    810.9942+-6.7525        ?
   hash-map                                 167.7655+-1.3360          165.7488+-1.2368          might be 1.0122x faster
   math-cordic                              533.6460+-1.2530     !    564.7302+-3.3072        ! definitely 1.0582x slower
   math-partial-sums                        416.1075+-1.5890          413.9803+-1.5941        
   math-spectral-norm                       603.9205+-1.3105     ?    605.7023+-2.3831        ?
   string-base64                            566.3441+-1.8800          565.4010+-2.8286        
   string-fasta                             432.3645+-1.3038     ?    434.1280+-2.6040        ?
   string-tagcloud                          202.9656+-0.7363     !    214.4090+-1.0346        ! definitely 1.0564x slower

   <geometric>                              437.5869+-0.4755     !    441.4119+-0.5580        ! definitely 1.0087x slower

                                                 Baseline                  RegCalls                                     
V8Spider:
   crypto                                    45.8716+-0.1157     !     46.5044+-0.2556        ! definitely 1.0138x slower
   deltablue                                 67.3221+-0.3871     !     68.3472+-0.4750        ! definitely 1.0152x slower
   earley-boyer                              43.5740+-0.2133     ?     43.8537+-0.2933        ?
   raytrace                                  34.3012+-0.2376     !     35.5822+-0.2124        ! definitely 1.0373x slower
   regexp                                    69.2668+-0.2450     ?     69.7873+-0.3807        ?
   richards                                  55.2410+-0.3010     ?     55.3565+-0.3359        ?
   splay                                     33.7589+-0.1354           33.6771+-0.1088        

   <geometric>                               48.0975+-0.0902     !     48.6395+-0.0882        ! definitely 1.0113x slower

                                                 Baseline                  RegCalls                                     
Octane:
   encrypt                                   0.20064+-0.00074    ?     0.20207+-0.00082       ?
   decrypt                                   3.46268+-0.01175    !     3.61847+-0.00916       ! definitely 1.0450x slower
   deltablue                        x2       0.16941+-0.00329    ^     0.15714+-0.00115       ^ definitely 1.0781x faster
   earley                                    0.32156+-0.00135    !     0.32663+-0.00116       ! definitely 1.0158x slower
   boyer                                     5.14986+-0.01429          5.13871+-0.02516       
   navier-stokes                    x2       5.55939+-0.03449          5.52549+-0.01775       
   raytrace                         x2       0.90040+-0.00218    !     0.90872+-0.00337       ! definitely 1.0092x slower
   richards                         x2       0.10979+-0.00047    ^     0.10515+-0.00100       ^ definitely 1.0442x faster
   splay                            x2       0.37585+-0.00109          0.37520+-0.00085       
   regexp                           x2      20.81527+-0.45032         20.75639+-0.45699       
   pdfjs                            x2      44.23691+-0.82671    ?    44.38860+-0.95322       ?
   mandreel                         x2      45.66958+-1.00996         44.91731+-1.01362         might be 1.0167x faster
   gbemu                            x2      32.83425+-1.39569         32.72742+-1.65382       
   closure                                   0.69301+-0.00435    !     0.70877+-0.00380       ! definitely 1.0227x slower
   jquery                                    8.89339+-0.04420    !     9.00218+-0.04147       ! definitely 1.0122x slower
   box2d                            x2      10.60272+-0.25444         10.47804+-0.26500         might be 1.0119x faster
   zlib                             x2     262.20991+-17.43653   ?   265.49640+-17.17956      ? might be 1.0125x slower
   typescript                       x2     572.83150+-42.60034       555.04961+-45.58792        might be 1.0320x faster

   <geometric>                               5.64870+-0.08623          5.60186+-0.09137         might be 1.0084x faster

                                                 Baseline                  RegCalls                                     
Kraken:
   ai-astar                                  120.801+-0.564      ?     121.121+-0.479         ?
   audio-beat-detection                       45.074+-0.610      ?      45.472+-0.466         ?
   audio-dft                                 105.395+-1.249      ?     105.673+-1.285         ?
   audio-fft                                  37.497+-0.293      !      38.845+-0.414         ! definitely 1.0360x slower
   audio-oscillator                           55.376+-0.157      ?      55.478+-0.135         ?
   imaging-darkroom                           76.379+-0.416      ?      77.130+-0.435         ?
   imaging-desaturate                         64.565+-0.274             64.144+-0.189         
   imaging-gaussian-blur                      88.501+-1.637      ?      89.002+-1.607         ?
   json-parse-financial                       44.091+-0.094      !      45.365+-0.165         ! definitely 1.0289x slower
   json-stringify-tinderbox                   29.300+-0.263             29.113+-0.063         
   stanford-crypto-aes                        43.814+-0.108      !      44.845+-0.273         ! definitely 1.0235x slower
   stanford-crypto-ccm                        42.121+-0.618      ?      43.064+-0.816         ? might be 1.0224x slower
   stanford-crypto-pbkdf2                    116.472+-0.900      ?     117.220+-0.757         ?
   stanford-crypto-sha256-iterative           38.255+-0.314      ?      38.392+-0.139         ?

   <arithmetic>                               64.832+-0.206      !      65.347+-0.225         ! definitely 1.0080x slower

                                                 Baseline                  RegCalls                                     
AsmBench:
   bigfib.cpp                               519.5968+-6.5631          517.3772+-6.5137        
   cray.c                                   438.3193+-1.6736     ?    438.5137+-1.7759        ?
   dry.c                                    569.8059+-23.0601    ?    592.7718+-24.0750       ? might be 1.0403x slower
   FloatMM.c                                854.8821+-2.7292     ?    861.2240+-3.9856        ?
   gcc-loops.cpp                           4257.5992+-12.7833    ?   4279.2718+-24.5958       ?
   n-body.c                                1021.5330+-2.7746     !   1029.3996+-4.8197        ! definitely 1.0077x slower
   Quicksort.c                              474.3958+-2.6139     ^    444.5173+-2.5708        ^ definitely 1.0672x faster
   stepanov_container.cpp                  3727.9221+-15.9067        3706.4846+-9.4443        
   Towers.c                                 267.8728+-1.4349     ^    255.7289+-2.0266        ^ definitely 1.0475x faster

   <geometric>                              847.4265+-4.1405          841.5890+-4.7080          might be 1.0069x faster

                                                 Baseline                  RegCalls                                     
Geomean of preferred means:
   <scaled-result>                           57.8312+-0.2180     ?     58.1409+-0.2402        ? might be 1.0054x slower

[Michael Saboff]

Created attachment 296447 [details]
Current Work in Progress - Should compile on Mac 32 & 64

[WebKit Commit Bot]

Attachment 296447 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/jit/JITEntrypoints.h:26:  Use #pragma once instead of #ifndef for header guard.  [build/header_guard] [5]
ERROR: Source/JavaScriptCore/jit/JITEntrypoints.h:38:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/jit/JITEntrypoints.h:209:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITEntrypoints.h:210:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITEntrypoints.h:220:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/Repatch.cpp:917:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/dfg/DFGJITFinalizer.cpp:45:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/bytecode/CodeBlock.cpp:1906:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1014:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/dfg/DFGSSAConversionPhase.cpp:108:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/dfg/DFGJITCompiler.cpp:444:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/dfg/DFGJITCompiler.cpp:445:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/jit/ThunkGenerators.cpp:240:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/ftl/FTLOutput.h:101:  The parameter name "reg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/ftl/FTLOutput.h:102:  The parameter name "reg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/ftl/FTLLink.cpp:175:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/ftl/FTLLink.cpp:293:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/dfg/DFGGraph.cpp:1626:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/dfg/DFGGraph.cpp:1626:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/b3/B3Validate.cpp:186:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/b3/B3Validate.cpp:187:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/jit/CachedRecovery.cpp:40:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/jit/JSInterfaceJIT.h:66:  The parameter name "reg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/jit/CallFrameShuffler.cpp:63:  One line control clauses should not use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/jit/CallFrameShuffler.cpp:206:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:1213:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:1841:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 27 in 118 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Michael Saboff]

Created attachment 296454 [details]
Fixed definite style issues, disabled dataLog

Don't know why there is an issue with the WebKit generated crypto .cpp files.

[WebKit Commit Bot]

Attachment 296454 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/jit/JITEntrypoints.h:208:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITEntrypoints.h:209:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITEntrypoints.h:219:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGJITFinalizer.cpp:45:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/bytecode/CodeBlock.cpp:1906:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGJITCompiler.cpp:444:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/ftl/FTLOutput.h:101:  The parameter name "reg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/ftl/FTLOutput.h:102:  The parameter name "reg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/ftl/FTLLink.cpp:293:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/jit/JSInterfaceJIT.h:66:  The parameter name "reg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:1213:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 11 in 117 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Michael Saboff]

Created attachment 296611 [details]
Likely fix for the build issue - fixed inconsistent case in new include file name.

[WebKit Commit Bot]

Attachment 296611 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/dfg/DFGJITFinalizer.cpp:45:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/bytecode/CodeBlock.cpp:1906:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGJITCompiler.cpp:444:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:863:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/ftl/FTLOutput.h:101:  The parameter name "reg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/ftl/FTLOutput.h:102:  The parameter name "reg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/ftl/FTLLink.cpp:293:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/jit/JITEntryPoints.h:208:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITEntryPoints.h:209:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITEntryPoints.h:219:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JSInterfaceJIT.h:66:  The parameter name "reg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:1213:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 12 in 117 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Michael Saboff]

Created attachment 296614 [details]
Fix remaining include typos

[WebKit Commit Bot]

Attachment 296614 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/dfg/DFGJITFinalizer.cpp:45:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/bytecode/CodeBlock.cpp:1906:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGJITCompiler.cpp:444:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:863:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/ftl/FTLOutput.h:101:  The parameter name "reg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/ftl/FTLOutput.h:102:  The parameter name "reg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/ftl/FTLLink.cpp:293:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/jit/JITEntryPoints.h:208:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITEntryPoints.h:209:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITEntryPoints.h:219:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JSInterfaceJIT.h:66:  The parameter name "reg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:1213:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 12 in 117 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Michael Saboff]

Created attachment 296624 [details]
Rebased patch

[WebKit Commit Bot]

Attachment 296624 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/dfg/DFGJITFinalizer.cpp:45:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/bytecode/CodeBlock.cpp:1906:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGJITCompiler.cpp:444:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:863:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/ftl/FTLOutput.h:101:  The parameter name "reg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/ftl/FTLOutput.h:102:  The parameter name "reg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/ftl/FTLLink.cpp:293:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/jit/JITEntryPoints.h:208:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITEntryPoints.h:209:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITEntryPoints.h:219:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JSInterfaceJIT.h:66:  The parameter name "reg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:1213:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 12 in 118 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 296624 [details]
Rebased patch

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

New failing tests:
inspector/codemirror/prettyprinting-css-rules.html

[Build Bot]

Created attachment 296639 [details]
Archive of layout-test-results from ews101 for mac-yosemite

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

[Build Bot]

Comment on attachment 296624 [details]
Rebased patch

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

New failing tests:
inspector/debugger/breakpoints/resolved-dump-all-pause-locations.html

[Build Bot]

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

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

[Michael Saboff]

Created attachment 296708 [details]
Almost done.  Patch for EWS testing.  Need to write change logs.

[WebKit Commit Bot]

Attachment 296708 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/dfg/DFGJITFinalizer.cpp:45:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/bytecode/CodeBlock.cpp:1906:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGJITCompiler.cpp:444:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:863:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/ftl/FTLOutput.h:101:  The parameter name "reg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/ftl/FTLOutput.h:102:  The parameter name "reg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/ftl/FTLLink.cpp:293:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/jit/JITEntryPoints.h:207:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITEntryPoints.h:208:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITEntryPoints.h:218:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JSInterfaceJIT.h:66:  The parameter name "reg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:1213:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 12 in 118 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Michael Saboff]

Created attachment 296726 [details]
Patch for Review

[Saam Barati]

Comment on attachment 296726 [details]
Patch for Review

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

> Source/WTF/wtf/DataLog.cpp:37
> +#define DATA_LOG_TO_FILE 1

oops

[WebKit Commit Bot]

Attachment 296726 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/dfg/DFGJITFinalizer.cpp:45:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/bytecode/CodeBlock.cpp:1906:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGJITCompiler.cpp:444:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:863:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/ftl/FTLOutput.h:101:  The parameter name "reg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/ftl/FTLOutput.h:102:  The parameter name "reg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/ftl/FTLLink.cpp:293:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/jit/JITEntryPoints.h:207:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITEntryPoints.h:208:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITEntryPoints.h:218:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
ERROR: Source/JavaScriptCore/jit/JSInterfaceJIT.h:66:  The parameter name "reg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:1213:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 13 in 122 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Michael Saboff]

(In reply to comment #23)
> Comment on attachment 296726 [details]
> Patch for Review
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=296726&action=review
> 
> > Source/WTF/wtf/DataLog.cpp:37
> > +#define DATA_LOG_TO_FILE 1
> 
> oops

Forgot to revert this.  I've taken care of this locally.

[Michael Saboff]

Created attachment 296745 [details]
Updated patch.  Rebased, addressed Saam's comment and removed superfluous white space changes

[WebKit Commit Bot]

Attachment 296745 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/dfg/DFGJITFinalizer.cpp:45:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGJITCompiler.cpp:444:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:863:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/ftl/FTLOutput.h:101:  The parameter name "reg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/ftl/FTLOutput.h:102:  The parameter name "reg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/ftl/FTLLink.cpp:293:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/jit/JITEntryPoints.h:207:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITEntryPoints.h:208:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITEntryPoints.h:218:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JSInterfaceJIT.h:66:  The parameter name "reg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:1213:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 11 in 117 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Filip Pizlo]

Starting to look at this now.

[Filip Pizlo]

Comment on attachment 296745 [details]
Updated patch.  Rebased, addressed Saam's comment and removed superfluous white space changes

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

I found a bug, but it's an easy bug to fix.  I'll keep reading.

> Source/JavaScriptCore/b3/B3Validate.cpp:187
> +                VALIDATE((value->as<ArgumentRegValue>()->argumentReg().isGPR()
> +                    ? (value->type() == pointerType() || value->type() == Int32)
> +                    : value->type() == Double), ("At ", *value));

I think that this is subtly wrong - for example we'll CSE a int32 ArgumentReg with a int64 ArgumentReg if they are the same reg.  It's not correct to "just" allow this.

If you want 32-bit arguments, the right way to go is Trunc(ArgumentReg(...)).  Trunc is a very cheap op and it's coalesced away by instruction selection - i.e. it doesn't even add load to the register allocator's coalescer.

I'd be OK with you filing a bug about this and putting a FIXME here and wherever your used the Int32 ArgumentReg feature.

> Source/JavaScriptCore/dfg/DFGJITCompiler.cpp:476
> +#endif

Super useful if you added a // NUMBER_OF_JS_FUNCTION_ARGUMENT_REGISTERS here.

I'm not an advocate of mandating such comments, as they are often pointless, but I like them for long chunks of code in long functions that have multiple #if blocks.

> Source/JavaScriptCore/dfg/DFGJITCompiler.cpp:579
> +#endif

Ditto.

> Source/JavaScriptCore/dfg/DFGMinifiedNode.cpp:45
> +        result.m_info = 0;

Put the argument index here so that you fix the bug in DFGVariableEventStream!

> Source/JavaScriptCore/dfg/DFGVariableEventStream.cpp:172
> +                unsigned argument = jsFunctionArgumentForArgumentRegister(gpr);

Bug!  We could have spilled this node and refilled it into a different register.

[Filip Pizlo]

Comment on attachment 296745 [details]
Updated patch.  Rebased, addressed Saam's comment and removed superfluous white space changes

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

LGTM other than the bug I found!  I want to look at the patch with the fix.  Hopefully it's easy to do.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:1910
> +    //  &&&&  Revisit this to use m_argumentsForChecking

Remove the comment or revisit this!

[Michael Saboff]

Created attachment 296782 [details]
Addressed Phil's issues and rebased

[WebKit Commit Bot]

Attachment 296782 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/dfg/DFGJITFinalizer.cpp:45:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGJITCompiler.cpp:444:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:863:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/ftl/FTLOutput.h:101:  The parameter name "reg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/ftl/FTLOutput.h:102:  The parameter name "reg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/ftl/FTLLink.cpp:293:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/jit/JITEntryPoints.h:207:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITEntryPoints.h:208:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITEntryPoints.h:218:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JSInterfaceJIT.h:66:  The parameter name "reg" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:1213:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 11 in 117 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Michael Saboff]

Committed r209653: <http://trac.webkit.org/changeset/209653>

[Csaba Osztrogonác]

(In reply to comment #33)
> Committed r209653: <http://trac.webkit.org/changeset/209653>

It broke the cloop build:
https://build.webkit.org/builders/Apple%20El%20Capitan%20CMake%20Debug%20%28Build%29/builds/10649

[Csaba Osztrogonác]

https://build.webkit.org/builders/Apple%20El%20Capitan%20LLINT%20CLoop%20%28BuildAndTest%29/builds/12400

[Michael Saboff]

(In reply to comment #34)
> (In reply to comment #33)
> > Committed r209653: <http://trac.webkit.org/changeset/209653>
> 
> It broke the cloop build:
> https://build.webkit.org/builders/
> Apple%20El%20Capitan%20CMake%20Debug%20%28Build%29/builds/10649

Working on fixing the loop build.

[Michael Saboff]

(In reply to comment #36)
> (In reply to comment #34)
> > (In reply to comment #33)
> > > Committed r209653: <http://trac.webkit.org/changeset/209653>
> > 
> > It broke the cloop build:
> > https://build.webkit.org/builders/
> > Apple%20El%20Capitan%20CMake%20Debug%20%28Build%29/builds/10649
> 
> Working on fixing the loop build.

Cloop fix landed in r209663: <http://trac.webkit.org/changeset/209663>

[Joseph Pecoraro]

I'm seeing lots of crashes when opening Web Inspector.

They look like:

> Exception Type:        EXC_BAD_ACCESS (SIGSEGV)
> Exception Codes:       KERN_INVALID_ADDRESS at 0x0000000000000000
> Exception Note:        EXC_CORPSE_NOTIFY
> 
> Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
> 0   com.apple.JavaScriptCore      	0x000000010a822840 JSC::CallFrameShuffler::snapshot(JSC::ArgumentsLocation) const + 688 (CachedRecovery.h:115)
> 1   com.apple.JavaScriptCore      	0x000000010a8211e6 JSC::linkPolymorphicCall(JSC::ExecState*, JSC::CallLinkInfo&, JSC::CallVariant) + 4454 (Repatch.cpp:929)
> 2   com.apple.JavaScriptCore      	0x000000010a5492c3 operationLinkPolymorphicCall + 51 (JITOperations.cpp:1094)
> 3   ???                           	0x0000465d14e057a2 0 + 77365596149666
> 4   ???                           	0x0000465d15269754 0 + 77365600753492
> ...

Sounds like it could be this change.

[Michael Saboff]

(In reply to comment #38)
> I'm seeing lots of crashes when opening Web Inspector.
> 
> They look like:
> 
> > Exception Type:        EXC_BAD_ACCESS (SIGSEGV)
> > Exception Codes:       KERN_INVALID_ADDRESS at 0x0000000000000000
> > Exception Note:        EXC_CORPSE_NOTIFY
> > 
> > Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
> > 0   com.apple.JavaScriptCore      	0x000000010a822840 JSC::CallFrameShuffler::snapshot(JSC::ArgumentsLocation) const + 688 (CachedRecovery.h:115)
> > 1   com.apple.JavaScriptCore      	0x000000010a8211e6 JSC::linkPolymorphicCall(JSC::ExecState*, JSC::CallLinkInfo&, JSC::CallVariant) + 4454 (Repatch.cpp:929)
> > 2   com.apple.JavaScriptCore      	0x000000010a5492c3 operationLinkPolymorphicCall + 51 (JITOperations.cpp:1094)
> > 3   ???                           	0x0000465d14e057a2 0 + 77365596149666
> > 4   ???                           	0x0000465d15269754 0 + 77365600753492
> > ...
> 
> Sounds like it could be this change.

Tracked in <https://bugs.webkit.org/show_bug.cgi?id=165728> - "REGRESSION(r209653) Crash in CallFrameShuffler::snapshot()"

[Filip Pizlo]

I think this made Speedometer crash.  I see the crash like so:

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.JavaScriptCore      	0x0000000106e5b12a operationGetByIdOptimize + 1194 (StructureIDTable.h:125)
1   ???                           	0x0000246893c245f2 0 + 40031574181362
2   ???                           	0x0000246893c0b5c4 0 + 40031574078916
3   ???                           	0x0000246893bdad79 0 + 40031573880185
4   ???                           	0x0000246893c50185 0 + 40031574360453
5   com.apple.JavaScriptCore      	0x0000000106fdad0b llint_entry + 26589 (LowLevelInterpreter.asm:778)
6   ???                           	0x0000246893c5eea6 0 + 40031574421158
7   ???                           	0x0000246893c77d10 0 + 40031574523152
8   ???                           	0x0000246893c4eb53 0 + 40031574354771
9   com.apple.JavaScriptCore      	0x0000000106fd434b vmEntryToJavaScript + 299 (LowLevelInterpreter64.asm:256)
10  com.apple.JavaScriptCore      	0x0000000106e3d35b JSC::JITCode::execute(JSC::VM*, JSC::ProtoCallFrame*) + 187 (JITCode.cpp:82)

So, not very helpful.  The last perf result on the perf bots for Speedometer was just before your patch.  The latest fix does not resolve it.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 165739

[Michael Saboff]

Relanded with fix in r209725: <http://trac.webkit.org/changeset/209725>

[Filip Pizlo]

This looks like a 1% JetStream regression.

[Filip Pizlo]

(In reply to comment #43)
> This looks like a 1% JetStream regression.

Actually, the bot bounced back and I don't think there is a real regression.  It looks like your patch got terribly unlucky on one of the bots and landed just as that bot was having a bad day.

[Michael Saboff]

Reopened as this change and subsequent fixes was rolled out in change set r209764: <http://trac.webkit.org/changeset/209764>