Bug 68582 – DFG JIT does not support to_primitive or strcat

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Filip Pizlo]

The lack of support for to_primitive or strcat means that if a code block has expressions like "foo " + bar + " baz" then the DFG cannot optimize the code block at all.

[Filip Pizlo]

Created attachment 108274 [details]
the patch

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1

Source/JavaScriptCore/assembler/AbstractMacroAssembler.h:170:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Source/JavaScriptCore/dfg/DFGOperations.h:79:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 13 files


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

[Filip Pizlo]

Comment on attachment 108274 [details]
the patch

Cleared review flag, because I didn't really mean to set it.  This is still a work in progress.

[Filip Pizlo]

Created attachment 108296 [details]
the patch

When combined with https://bugs.webkit.org/show_bug.cgi?id=68597, it's a 14% speed-up on Kraken.


Benchmark report for SunSpider, V8, and Kraken.

VMs tested:
"TipOfTree" at /Volumes/Data/pizlo/quinary/OpenSource/WebKitBuild/Release/jsc
"IntDiv" at /Volumes/Data/pizlo/OpenSource/WebKitBuild/Release/jsc
"IntDivStrCat" at /Volumes/Data/pizlo/septenary/OpenSource/WebKitBuild/Release/jsc

Collected 12 samples per benchmark/VM, with 4 VM invocations per benchmark. 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.

                                            TipOfTree                 IntDiv               IntDivStrCat        IntDivStrCat v. TipOfTree 
SunSpider:
   3d-cube                                7.7031+-0.2217    ?     7.8395+-0.2910          7.6701+-0.1962       
   3d-morph                               7.5458+-0.1330          7.5094+-0.2132          7.4666+-0.2495         might be 1.0106x faster
   3d-raytrace                            7.5513+-0.2014    ?     7.8452+-0.3016          7.7827+-0.1491       ? might be 1.0306x slower
   access-binary-trees                    2.0995+-0.0579          2.0684+-0.0596    ?     2.1379+-0.1433       ? might be 1.0183x slower
   access-fannkuch                       11.8476+-0.3710         11.7814+-0.2936    ?    11.8475+-0.3008       
   access-nbody                           3.5674+-0.0885          3.4518+-0.0559    ?     3.5763+-0.0967       ?
   access-nsieve                          2.5801+-0.0738    ?     2.6087+-0.0895          2.5845+-0.0629       ?
   bitops-3bit-bits-in-byte               1.8394+-0.0448    ?     1.8507+-0.0570          1.8055+-0.0425         might be 1.0188x faster
   bitops-bits-in-byte                    2.8397+-0.0606    ?     2.8577+-0.0704          2.8486+-0.0719       ?
   bitops-bitwise-and                     3.7522+-0.1308          3.6579+-0.1029          3.6303+-0.1049         might be 1.0336x faster
   bitops-nsieve-bits                     5.3488+-0.1198          5.2944+-0.1290    ?     5.4255+-0.1257       ? might be 1.0143x slower
   controlflow-recursive                  2.2455+-0.0651          2.2167+-0.0437          2.2005+-0.0542         might be 1.0204x faster
   crypto-aes                             6.8923+-0.3697          6.3176+-0.2402          6.1573+-0.1484       ^ definitely 1.1194x faster
   crypto-md5                             2.8049+-0.0709    ^     2.5663+-0.0668          2.4669+-0.0729       ^ definitely 1.1370x faster
   crypto-sha1                            2.3478+-0.0652    ^     2.1351+-0.0607    ?     2.1476+-0.0806       ^ definitely 1.0932x faster
   date-format-tofte                     10.3161+-0.2810         10.2830+-0.2989    ?    10.4249+-0.2628       ? might be 1.0105x slower
   date-format-xparb                      8.7798+-0.2838          8.7494+-0.2496    ?     8.9220+-0.2599       ? might be 1.0162x slower
   math-cordic                            6.3076+-0.1537          6.1111+-0.1149          6.0745+-0.0764       ^ definitely 1.0384x faster
   math-partial-sums                      7.3659+-0.1267    ?     7.4812+-0.1393    ?     7.6897+-0.2075       ? might be 1.0440x slower
   math-spectral-norm                     2.7030+-0.1058    !     2.9985+-0.0863    ?     3.0546+-0.0794       ! definitely 1.1300x slower
   regexp-dna                            10.8762+-0.1921         10.8190+-0.1663    ?    10.9030+-0.2224       ?
   string-base64                          5.8533+-0.1371    ?     5.8618+-0.1759    ?     5.9906+-0.2396       ? might be 1.0235x slower
   string-fasta                           7.2075+-0.1744          7.1327+-0.1970          7.1087+-0.1609         might be 1.0139x faster
   string-tagcloud                       11.9179+-0.3311    ?    11.9804+-0.3916    ?    12.0016+-0.3266       ?
   string-unpack-code                    18.7440+-0.3804    ?    18.8508+-0.5263    ?    19.2606+-0.6360       ? might be 1.0276x slower
   string-validate-input                  6.4930+-0.0958    ?     6.6237+-0.2460          6.4775+-0.1372       

   <arithmetic>                           6.4435+-0.0377          6.4189+-0.0338    ?     6.4483+-0.0393       ?
   <geometric>                            5.3328+-0.0319          5.2913+-0.0219    ?     5.3018+-0.0402       
   <harmonic>                             4.3898+-0.0353          4.3388+-0.0330          4.3374+-0.0541         might be 1.0121x faster

                                            TipOfTree                 IntDiv               IntDivStrCat        IntDivStrCat v. TipOfTree 
V8:
   crypto                                71.2496+-0.2918    ?    71.3862+-0.3429         71.1184+-0.5196       
   deltablue                            237.3015+-1.4425    ?   237.3798+-1.0827        236.5956+-1.3604       
   earley-boyer                          88.2273+-0.3942         88.1260+-0.3059         87.9492+-0.3159       
   raytrace                              64.1057+-0.6186    ?    65.0615+-0.5036    ^    62.6563+-0.4224       ^ definitely 1.0231x faster
   regexp                               105.5161+-0.4444    !   106.6996+-0.3355    !   108.3552+-0.6639       ! definitely 1.0269x slower
   richards                             198.5336+-0.7411    ?   199.2889+-0.7026        199.0919+-0.6748       ?
   splay                                 98.3329+-0.2213    ?    98.5661+-0.5356         98.0713+-0.4491       

   <arithmetic>                         123.3238+-0.2493    ?   123.7869+-0.2621        123.4054+-0.1661       ?
   <geometric>                          110.1665+-0.2139    !   110.6913+-0.2509    ^   110.1008+-0.1791       
   <harmonic>                           100.1846+-0.2352    !   100.7525+-0.2618    ^    99.8989+-0.2373       

                                            TipOfTree                 IntDiv               IntDivStrCat        IntDivStrCat v. TipOfTree 
Kraken:
   ai-astar                             616.6539+-2.9734        611.4591+-4.2299    ?   615.3805+-4.1636       
   audio-beat-detection                 467.0274+-4.0021    ^   377.9344+-1.6609    ^   204.5658+-1.0722       ^ definitely 2.2830x faster
   audio-dft                            422.0664+-1.7690        421.1265+-2.1315        419.6968+-2.5885       
   audio-fft                            363.5815+-2.0561    ^   312.9141+-0.8157    ^   141.5586+-0.9472       ^ definitely 2.5684x faster
   audio-oscillator                     254.2253+-1.3820    !   258.3617+-1.3677    ?   258.4752+-1.0439       ! definitely 1.0167x slower
   imaging-darkroom                     422.4535+-1.9687        420.4771+-1.1829    ?   421.7214+-0.7025       
   imaging-desaturate                   208.7662+-0.5285    ?   208.9123+-0.7579    ?   210.1087+-1.2589       ?
   imaging-gaussian-blur                594.3785+-1.2263        594.0332+-0.8333    !   598.2852+-3.3919       ?
   json-parse-financial                  48.0277+-0.2236    !    48.6539+-0.3093         48.6490+-0.2884       ! definitely 1.0129x slower
   json-stringify-tinderbox              68.8110+-0.2238    ^    68.0999+-0.3462    !    70.1338+-0.5934       ! definitely 1.0192x slower
   stanford-crypto-aes                  137.5441+-0.4898    ^   134.0939+-1.0359    ?   135.0720+-0.7700       ^ definitely 1.0183x faster
   stanford-crypto-ccm                  109.4925+-0.5785    ^   104.6828+-0.9352    !   105.8698+-0.2324       ^ definitely 1.0342x faster
   stanford-crypto-pbkdf2               208.6068+-3.2304        204.5755+-0.9357    ?   205.3682+-1.0130         might be 1.0158x faster
   stanford-crypto-sha256-iterative      84.5903+-0.2556    ?    84.9048+-0.5000    ?    85.2254+-0.4590       ?

   <arithmetic>                         286.1589+-0.5195    ^   275.0164+-0.4482    ^   251.4365+-0.4675       ^ definitely 1.1381x faster
   <geometric>                          217.1089+-0.4842    ^   210.3265+-0.3279    ^   191.2333+-0.3325       ^ definitely 1.1353x faster
   <harmonic>                           155.4981+-0.3995    ^   153.0205+-0.3357    ^   144.4740+-0.3776       ^ definitely 1.0763x faster

                                            TipOfTree                 IntDiv               IntDivStrCat        IntDivStrCat v. TipOfTree 
All benchmarks:
   <arithmetic>                         107.1707+-0.1773    ^   103.9070+-0.1572    ^    96.8426+-0.1626       ^ definitely 1.1066x faster
   <geometric>                           25.2527+-0.0971    ^    24.9250+-0.0673    ^    24.2354+-0.1112       ^ definitely 1.0420x faster
   <harmonic>                             7.7267+-0.0608          7.6381+-0.0568          7.6281+-0.0927         might be 1.0129x faster

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1

Source/JavaScriptCore/dfg/DFGOperations.h:79:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 13 files


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

[Filip Pizlo]

Created attachment 108297 [details]
the patch - fix style

[Darin Adler]

Comment on attachment 108297 [details]
the patch - fix style

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

> Source/JavaScriptCore/assembler/AbstractMacroAssembler.h:166
> +        // This is only here so that TrustedImmPtr(0) does not confuse the C++
> +        // overload handling rules.

I am a little confused by this. Why is it important to be able to compile conversion from the constant 0 and not the constant 1?

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:1552
> +        // FIXME: add string speculation here.

Should capitalize the "a" in "Add".

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:1611
> +        for (int operandIdx = 0; operandIdx < (int)node.numChildren(); ++operandIdx) {

Could we use size_t for this type instead of int, or is there a good reason to use int and cast numChildren? Is it just that we don’t want to use a 64-bit index?

[Filip Pizlo]

(In reply to comment #7)
> (From update of attachment 108297 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=108297&action=review
> 
> > Source/JavaScriptCore/assembler/AbstractMacroAssembler.h:166
> > +        // This is only here so that TrustedImmPtr(0) does not confuse the C++
> > +        // overload handling rules.
> 
> I am a little confused by this. Why is it important to be able to compile conversion from the constant 0 and not the constant 1?

You should never use TrustedImmPtr(1) to assemble an int constant; that's what Imm32() and friends are for.

What happened is that if you only have one overload for TrustedImmPtr(), and it takes void*, then you can safely call TrustedImmPtr(0).  But when I added a size_t overload for TrustedImmPtr(), it meant that the compiler would reject the conversion from 0 to void*, and loads of code failed to compile.  So I added an int overload, but wanted to have this overload assert that it's not being used to store real int32s. 

> 
> > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:1552
> > +        // FIXME: add string speculation here.
> 
> Should capitalize the "a" in "Add".

Got it!

> 
> > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:1611
> > +        for (int operandIdx = 0; operandIdx < (int)node.numChildren(); ++operandIdx) {
> 
> Could we use size_t for this type instead of int, or is there a good reason to use int and cast numChildren? Is it just that we don’t want to use a 64-bit index?

Yeah, of course, that would be better.  I think actually numChildren() is unsigned, so I should use that.  (It's unsigned and not size_t because we try to pack things tightly in DFG::Node, and because if we have a DFG with more than 2 billion nodes in it, we're probably dead already.)

[Filip Pizlo]

Landed in r95758.