Bug 71329 - DFG inlining breaks function.arguments
Summary: DFG inlining breaks function.arguments
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-11-01 14:56 PDT by Filip Pizlo
Modified: 2011-11-01 16:14 PDT (History)
1 user (show)

See Also:


Attachments
the patch (57.33 KB, patch)
2011-11-01 15:00 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch - fix style (57.06 KB, patch)
2011-11-01 15:07 PDT, Filip Pizlo
oliver: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Filip Pizlo 2011-11-01 14:56:44 PDT
Most of the functionality is there to allow call frames to be reified in the case of inlining and uses of function.arguments, but a few pieces of glue are missing.
Comment 1 Filip Pizlo 2011-11-01 15:00:39 PDT
Created attachment 113230 [details]
the patch

This patch looks scary big, but most of that is the expectations file and a bunch of code motion to reduce duplication.
Comment 2 Filip Pizlo 2011-11-01 15:06:10 PDT
Looks to be performance-neutral.


Benchmark report for SunSpider, V8, and Kraken.

VMs tested:
"TipOfTree" at /Volumes/Data/pizlo/quinary/OpenSource/WebKitBuild/Release/jsc
"FixArguments" at /Volumes/Data/pizlo/tertiary/OpenSource/WebKitBuild/Release/jsc

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

                                            TipOfTree              FixArguments                                  
SunSpider:
   3d-cube                                7.6499+-0.2593          7.6162+-0.1762       
   3d-morph                               7.9063+-0.1820    ?     8.0282+-0.0711       ? might be 1.0154x slower
   3d-raytrace                            7.6961+-0.1969    ?     7.7956+-0.2230       ? might be 1.0129x slower
   access-binary-trees                    1.6078+-0.0605    ?     1.7233+-0.0965       ? might be 1.0718x slower
   access-fannkuch                        6.6464+-0.1293          6.5501+-0.1310         might be 1.0147x faster
   access-nbody                           3.9135+-0.1152          3.7795+-0.1050         might be 1.0355x faster
   access-nsieve                          2.7246+-0.0600          2.6066+-0.0895         might be 1.0452x faster
   bitops-3bit-bits-in-byte               1.3250+-0.0306    ?     1.3508+-0.0338       ? might be 1.0195x slower
   bitops-bits-in-byte                    2.3868+-0.0435    ?     2.4308+-0.0609       ? might be 1.0184x slower
   bitops-bitwise-and                     3.4625+-0.0592    ?     3.4809+-0.1118       ?
   bitops-nsieve-bits                     5.5378+-0.1221          5.5309+-0.1361       
   controlflow-recursive                  2.1887+-0.0393          2.1593+-0.0368         might be 1.0136x faster
   crypto-aes                             7.9143+-0.2241          7.7961+-0.2043         might be 1.0152x faster
   crypto-md5                             2.7922+-0.0625    ?     2.7973+-0.1063       ?
   crypto-sha1                            2.5486+-0.0869          2.5365+-0.0548       
   date-format-tofte                     10.8129+-0.4533         10.3446+-0.3227         might be 1.0453x faster
   date-format-xparb                      9.0835+-0.2038    ?     9.5358+-0.4901       ? might be 1.0498x slower
   math-cordic                            6.5703+-0.1055    ?     6.7803+-0.1592       ? might be 1.0320x slower
   math-partial-sums                      7.7829+-0.1472          7.7043+-0.1182         might be 1.0102x faster
   math-spectral-norm                     2.6944+-0.0389    ^     2.5631+-0.0418       ^ definitely 1.0513x faster
   regexp-dna                            11.9887+-0.2775         11.8429+-0.2067         might be 1.0123x faster
   string-base64                          4.2709+-0.2253    ?     4.3151+-0.1974       ? might be 1.0103x slower
   string-fasta                           6.5787+-0.1493    ?     6.6469+-0.2670       ? might be 1.0104x slower
   string-tagcloud                       12.0964+-0.5036    ?    12.1212+-0.2836       ?
   string-unpack-code                    21.3786+-0.5177    ?    21.5614+-0.5279       ?
   string-validate-input                  5.4997+-0.1880          5.3993+-0.1570         might be 1.0186x faster

   <arithmetic> *                         6.3484+-0.0278          6.3460+-0.0306       
   <geometric>                            5.1016+-0.0212          5.0982+-0.0284       
   <harmonic>                             4.0599+-0.0298    ?     4.0683+-0.0353       ?

                                            TipOfTree              FixArguments                                  
V8:
   crypto                                75.1757+-0.3731    ?    75.2230+-0.3923       ?
   deltablue                            169.7191+-1.1452    ?   170.4428+-0.9564       ?
   earley-boyer                          91.8773+-0.5745    ?    92.1749+-0.4571       ?
   raytrace                              64.9346+-0.9143         64.6376+-0.7337       
   regexp                               107.6506+-0.3287    !   109.3592+-0.3937       ! definitely 1.0159x slower
   richards                             128.6929+-0.4510        128.4158+-0.4081       
   splay                                 74.1930+-1.5509    ?    74.5736+-1.1579       ?

   <arithmetic>                         101.7490+-0.2789    ?   102.1181+-0.2964       ?
   <geometric> *                         96.5442+-0.2892    ?    96.8561+-0.3205       ?
   <harmonic>                            92.0604+-0.3245    ?    92.3077+-0.3561       ?

                                            TipOfTree              FixArguments                                  
Kraken:
   ai-astar                             508.4903+-3.3297    ^   499.9517+-1.4941       ^ definitely 1.0171x faster
   audio-beat-detection                 194.7723+-2.8623        193.5630+-2.1211       
   audio-dft                            282.1170+-2.5872        279.8534+-2.8109       
   audio-fft                            126.9287+-0.9359    ?   127.5717+-0.9902       ?
   audio-oscillator                     255.2655+-1.3368    ?   256.5465+-1.9383       ?
   imaging-darkroom                     307.8138+-4.4949        307.2572+-4.4170       
   imaging-desaturate                   231.2232+-0.7844        230.7775+-1.2691       
   imaging-gaussian-blur                563.5772+-2.1326        560.4199+-2.0540       
   json-parse-financial                  57.7405+-0.3588    !    58.8814+-0.2778       ! definitely 1.0198x slower
   json-stringify-tinderbox              70.7785+-0.5569    ^    69.2321+-0.4219       ^ definitely 1.0223x faster
   stanford-crypto-aes                   99.3010+-0.7644         98.6047+-1.5776       
   stanford-crypto-ccm                  101.8039+-0.6532    ?   101.9548+-1.0385       ?
   stanford-crypto-pbkdf2               197.2721+-1.2126    ?   198.6930+-1.7125       ?
   stanford-crypto-sha256-iterative      82.6257+-0.4992    ^    81.1939+-0.4549       ^ definitely 1.0176x faster

   <arithmetic> *                       219.9793+-0.6961    ^   218.8929+-0.3694       ^ definitely 1.0050x faster
   <geometric>                          174.5806+-0.5142        173.9682+-0.3012       
   <harmonic>                           139.5624+-0.3978        139.1772+-0.3166       

                                            TipOfTree              FixArguments                                  
All benchmarks:
   <arithmetic>                          84.1917+-0.1979         83.9218+-0.1213       
   <geometric>                           22.6422+-0.0496         22.6210+-0.0747       
   <harmonic>                             7.1424+-0.0509    ?     7.1566+-0.0606       ?

                                            TipOfTree              FixArguments                                  
Geomean of preferred means:
   <scaled-result>                       51.2766+-0.1098         51.2409+-0.1156
Comment 3 WebKit Review Bot 2011-11-01 15:06:12 PDT
Attachment 113230 [details] did not pass style-queue:

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

Source/JavaScriptCore/runtime/Arguments.h:176:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 1 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Filip Pizlo 2011-11-01 15:07:44 PDT
Created attachment 113232 [details]
the patch - fix style
Comment 5 Gavin Barraclough 2011-11-01 15:21:21 PDT
Comment on attachment 113232 [details]
the patch - fix style

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

> Source/JavaScriptCore/runtime/Arguments.h:170
> +            ASSERT(argc == numParameters + 1);

What does ASSERT compile to if building without assertions? - does it compile to a do-nothing statement, or an empty one?
If it compiles to an empty one, this could be a bug! - I think this should be:

ASSERT(!callFrame->isInlineCallFrame() || (argc == numParameters + 1));
Comment 6 Oliver Hunt 2011-11-01 15:23:53 PDT
Comment on attachment 113232 [details]
the patch - fix style

r=me
Comment 7 Oliver Hunt 2011-11-01 15:26:02 PDT
Comment on attachment 113232 [details]
the patch - fix style

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

>> Source/JavaScriptCore/runtime/Arguments.h:170
>> +            ASSERT(argc == numParameters + 1);
> 
> What does ASSERT compile to if building without assertions? - does it compile to a do-nothing statement, or an empty one?
> If it compiles to an empty one, this could be a bug! - I think this should be:
> 
> ASSERT(!callFrame->isInlineCallFrame() || (argc == numParameters + 1));

The ASSERT macro does the right thing -- it's do { } while(0) IIRC
Comment 8 Filip Pizlo 2011-11-01 16:14:26 PDT
Landed in http://trac.webkit.org/changeset/99009