Bug 156669 – [JSC] DFG should support relational comparisons of Number and Other

• Component: New Bugs
• Status: RESOLVED FIXED

[Benjamin Poulain]

[JSC] DFG should support relational comparisons of Number and Other

[Benjamin Poulain]

Created attachment 276564 [details]
Patch

[Benjamin Poulain]

>5% on Sunspider/3d-raytrace. Easy wins here and there:

                                                  Conf#1                    Conf#2                                      
SunSpider:
   3d-cube                                    4.9859+-0.1048     ?      5.0979+-0.0845        ? might be 1.0225x slower
   3d-morph                                   5.1717+-0.0509     ?      5.1923+-0.0660        ?
   3d-raytrace                                5.5222+-0.0303     ^      5.2502+-0.0965        ^ definitely 1.0518x faster
   access-binary-trees                        2.1145+-0.0198     ?      2.1724+-0.0559        ? might be 1.0274x slower
   access-fannkuch                            5.9337+-0.0715            5.8528+-0.1176          might be 1.0138x faster
   access-nbody                               2.6371+-0.1096            2.5488+-0.0300          might be 1.0347x faster
   access-nsieve                              3.0457+-0.0229     ?      3.0681+-0.0387        ?
   bitops-3bit-bits-in-byte                   1.1351+-0.0109     ?      1.1405+-0.0125        ?
   bitops-bits-in-byte                        2.7861+-0.0213     ?      2.8001+-0.0502        ?
   bitops-bitwise-and                         2.0675+-0.0332            2.0480+-0.0144        
   bitops-nsieve-bits                         3.1379+-0.0645            3.1296+-0.0467        
   controlflow-recursive                      2.3544+-0.0174            2.3503+-0.0337        
   crypto-aes                                 4.0292+-0.0413     ?      4.0324+-0.0696        ?
   crypto-md5                                 2.5080+-0.0444            2.5002+-0.0531        
   crypto-sha1                                2.3578+-0.0277            2.3432+-0.0190        
   date-format-tofte                          6.4053+-0.0903     ?      6.4075+-0.0770        ?
   date-format-xparb                          4.8294+-0.1402     ?      4.8493+-0.1692        ?
   math-cordic                                2.9286+-0.0389            2.8776+-0.0131          might be 1.0177x faster
   math-partial-sums                          4.9225+-0.1037            4.8803+-0.0690        
   math-spectral-norm                         1.9831+-0.0149     ?      1.9973+-0.0194        ?
   regexp-dna                                 6.3421+-0.0768            6.2721+-0.0501          might be 1.0112x faster
   string-base64                              4.7411+-0.0730     ?      4.8863+-0.1663        ? might be 1.0306x slower
   string-fasta                               5.9668+-0.1654            5.8749+-0.0860          might be 1.0157x faster
   string-tagcloud                            8.1567+-0.0770     ?      8.2552+-0.2033        ? might be 1.0121x slower
   string-unpack-code                        19.4009+-0.4762     ?     19.4415+-0.4419        ?
   string-validate-input                      4.4489+-0.1219            4.3643+-0.0198          might be 1.0194x faster

   <arithmetic>                               4.6120+-0.0239            4.6013+-0.0260          might be 1.0023x faster

                                                  Conf#1                    Conf#2                                      
Octane:
   encrypt                                   0.16215+-0.00089          0.16188+-0.00077       
   decrypt                                   2.84181+-0.00700          2.83793+-0.00366       
   deltablue                        x2       0.13987+-0.00160          0.13956+-0.00119       
   earley                                    0.28584+-0.00080    ?     0.28594+-0.00089       ?
   boyer                                     5.01632+-0.06001    ?     5.02915+-0.05367       ?
   navier-stokes                    x2       4.99973+-0.00721    ?     5.00253+-0.00861       ?
   raytrace                         x2       0.79912+-0.00250    ?     0.80120+-0.00283       ?
   richards                         x2       0.08335+-0.00063    ?     0.08385+-0.00046       ?
   splay                            x2       0.34159+-0.00129    ?     0.34190+-0.00111       ?
   regexp                           x2      15.76614+-0.11218    ?    15.77291+-0.10053       ?
   pdfjs                            x2      39.76868+-0.27101    ?    39.80804+-0.28669       ?
   mandreel                         x2      42.61976+-0.08029         42.54716+-0.12615       
   gbemu                            x2      24.19040+-0.07575         24.14995+-0.08258       
   closure                                   0.53466+-0.00131    ^     0.53167+-0.00110       ^ definitely 1.0056x faster
   jquery                                    6.83269+-0.02810          6.82235+-0.02935       
   box2d                            x2       9.17647+-0.03200    ?     9.18539+-0.02331       ?
   zlib                             x2     360.04454+-3.37642        358.57454+-4.06083       
   typescript                       x2     617.64299+-2.06962    ?   617.86455+-1.89938       ?

   <geometric>                               5.02176+-0.00666          5.02161+-0.00704         might be 1.0000x faster

                                                  Conf#1                    Conf#2                                      
Kraken:
   ai-astar                                   88.104+-0.407      ?      88.946+-1.186         ?
   audio-beat-detection                       42.056+-0.063      ^      41.061+-0.096         ^ definitely 1.0242x faster
   audio-dft                                  99.650+-0.822             99.536+-0.929         
   audio-fft                                  32.752+-0.019      ?      32.971+-0.364         ?
   audio-oscillator                           47.862+-0.081             47.851+-0.075         
   imaging-darkroom                           60.532+-0.482      ^      59.988+-0.054         ^ definitely 1.0091x faster
   imaging-desaturate                         45.517+-0.690             45.255+-0.166         
   imaging-gaussian-blur                      64.913+-1.672             63.702+-2.006           might be 1.0190x faster
   json-parse-financial                       37.855+-0.139      ?      37.921+-0.302         ?
   json-stringify-tinderbox                   24.565+-0.790             24.554+-0.743         
   stanford-crypto-aes                        39.708+-0.447             39.595+-0.794         
   stanford-crypto-ccm                        32.765+-0.619      ?      33.094+-0.625         ? might be 1.0100x slower
   stanford-crypto-pbkdf2                     96.472+-0.274      ?      96.727+-0.343         ?
   stanford-crypto-sha256-iterative           36.772+-0.126             36.634+-0.085         

   <arithmetic>                               53.537+-0.190             53.417+-0.186           might be 1.0023x faster

                                                  Conf#1                    Conf#2                                      
AsmBench:
   bigfib.cpp                               441.5196+-0.7669     ?    443.0796+-1.7072        ?
   cray.c                                   363.6939+-0.5656     ?    364.3559+-2.4752        ?
   dry.c                                    461.6533+-27.8064    ?    462.1714+-28.9926       ?
   FloatMM.c                                723.4377+-3.6195     ?    726.6473+-3.5986        ?
   gcc-loops.cpp                           3709.0511+-4.2150     ?   3718.9008+-11.3078       ?
   n-body.c                                 808.7373+-1.9564          807.3993+-1.6012        
   Quicksort.c                              398.2005+-0.7623          397.2776+-0.6105        
   stepanov_container.cpp                  3310.4908+-12.0120    ?   3316.4831+-13.6885       ?
   Towers.c                                 273.4291+-0.9558     ?    273.5383+-0.8914        ?

   <geometric>                              728.2313+-4.3860     ?    729.1486+-5.3243        ? might be 1.0013x slower

                                                  Conf#1                    Conf#2                                      
Geomean of preferred means:
   <scaled-result>                           30.8251+-0.0657           30.7990+-0.0761          might be 1.0008x faster

[Darin Adler]

Comment on attachment 276564 [details]
Patch

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

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:411
> +            if (child.isNumber()) {
> +                setConstant(node, jsDoubleNumber(child.asNumber()));
> +                break;
> +            }
> +            if (child.isUndefined()) {
> +                setConstant(node, jsDoubleNumber(PNaN));
> +                break;
> +            }
> +            if (child.isNull() || child.isFalse()) {
> +                setConstant(node, jsDoubleNumber(0));
> +                break;
> +            }
> +            if (child.isTrue()) {
> +                setConstant(node, jsDoubleNumber(1));
> +                break;
> +            }

Seems like we would want this to share code with JSValue::toNumber function. This code should be something like this:

    if (child && !child.isCell()) {
        setConstant(node, jsDoubleNumber(child.toNumberNonCell()))
        break;
    }

I suppose it’s OK to write it out like this instead, but it seems nice to concentrate this knowledge of how to turn values into numbers in one place.

[Benjamin Poulain]

(In reply to comment #3)
> Seems like we would want this to share code with JSValue::toNumber function.
> This code should be something like this:
> 
>     if (child && !child.isCell()) {
>         setConstant(node, jsDoubleNumber(child.toNumberNonCell()))
>         break;
>     }
> 
> I suppose it’s OK to write it out like this instead, but it seems nice to
> concentrate this knowledge of how to turn values into numbers in one place.

I think this may be a bit dangerous for maintainability.
In the abstract interpreter, we must ensure this transformation is thread-safe and without side effect. Someone could modify JSValue without knowing how it is used.

[WebKit Commit Bot]

Comment on attachment 276564 [details]
Patch

Clearing flags on attachment: 276564

Committed r199639: <http://trac.webkit.org/changeset/199639>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

(In reply to comment #4)
> (In reply to comment #3)
> > Seems like we would want this to share code with JSValue::toNumber function.
> > This code should be something like this:
> > 
> >     if (child && !child.isCell()) {
> >         setConstant(node, jsDoubleNumber(child.toNumberNonCell()))
> >         break;
> >     }
> > 
> > I suppose it’s OK to write it out like this instead, but it seems nice to
> > concentrate this knowledge of how to turn values into numbers in one place.
> 
> I think this may be a bit dangerous for maintainability.
> In the abstract interpreter, we must ensure this transformation is
> thread-safe and without side effect. Someone could modify JSValue without
> knowing how it is used.

I understand what you are saying, but do not agree with you. All the non-cell transformations on JSValue have this property today. That’s what the entire class is about. The lack of a VM& or ExecState& argument is part of what makes this clear.