Bug 71977 - DFG should not reparse code that was just parsed
Summary: DFG should not reparse code that was just parsed
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: InRadar
Depends on: 71978
Blocks:
  Show dependency treegraph
 
Reported: 2011-11-09 17:41 PST by Filip Pizlo
Modified: 2011-11-10 14:04 PST (History)
2 users (show)

See Also:


Attachments
work in progress (49.28 KB, patch)
2011-11-10 00:23 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
work in progress (55.19 KB, patch)
2011-11-10 03:39 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (57.09 KB, patch)
2011-11-10 04:07 PST, Filip Pizlo
ggaren: review+
ggaren: commit-queue-
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-09 17:41:23 PST
The DFG will currently redo parsing on an executable when that executable had just been parsed by the old JIT.  It will also repeatedly reparse code that is inlined multiple times.  This is likely not particularly great for performance.

This is an umbrella bug for now, since there are a number of independent things that need to be fixed.
Comment 1 Filip Pizlo 2011-11-10 00:23:33 PST
Created attachment 114448 [details]
work in progress

Putting it up here as backup.  I'm pretty sure this won't compile yet.
Comment 2 Filip Pizlo 2011-11-10 03:39:13 PST
Created attachment 114465 [details]
work in progress

It's starting to work.
Comment 3 Filip Pizlo 2011-11-10 03:54:17 PST
Not bad.



Benchmark report for SunSpider, V8, and Kraken on oldmac.local (MacPro4,1).

VMs tested:
"TipOfTree" at /Volumes/Data/pizlo/quinary/OpenSource/WebKitBuild/Release/jsc (r99833)
"DontReparse" at /Volumes/Data/pizlo/octonary/OpenSource/WebKitBuild/Release/jsc (r99833)

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              DontReparse                                   
SunSpider:
   3d-cube                                9.1379+-0.0258    ^     8.9776+-0.0431       ^ definitely 1.0179x faster
   3d-morph                              10.2370+-0.1657         10.0917+-0.0355         might be 1.0144x faster
   3d-raytrace                            9.6404+-0.0490    ^     9.2559+-0.1173       ^ definitely 1.0415x faster
   access-binary-trees                    1.9784+-0.0069    ^     1.9054+-0.0058       ^ definitely 1.0383x faster
   access-fannkuch                        9.3247+-0.0053    ^     9.1539+-0.0087       ^ definitely 1.0187x faster
   access-nbody                           5.3036+-0.0459    ^     5.0134+-0.0038       ^ definitely 1.0579x faster
   access-nsieve                          3.7628+-0.0067    ?     3.7635+-0.0509       ?
   bitops-3bit-bits-in-byte               1.5622+-0.0153    ^     1.4952+-0.0147       ^ definitely 1.0448x faster
   bitops-bits-in-byte                    6.1942+-0.0324          6.1408+-0.0401       
   bitops-bitwise-and                     3.9978+-0.0078    ^     3.9756+-0.0045       ^ definitely 1.0056x faster
   bitops-nsieve-bits                     6.8386+-0.0425          6.8337+-0.0432       
   controlflow-recursive                  2.8144+-0.0257    ^     2.7221+-0.0205       ^ definitely 1.0339x faster
   crypto-aes                             9.3584+-0.0448    ^     8.6663+-0.0735       ^ definitely 1.0799x faster
   crypto-md5                             3.3037+-0.0118    ^     3.0594+-0.0265       ^ definitely 1.0799x faster
   crypto-sha1                            3.0046+-0.0219    ^     2.6333+-0.0255       ^ definitely 1.1410x faster
   date-format-tofte                     13.1019+-0.1050         12.9822+-0.1197       
   date-format-xparb                     12.6418+-0.2223         12.4466+-0.1807         might be 1.0157x faster
   math-cordic                            9.1270+-0.3557          9.1158+-0.3626       
   math-partial-sums                     12.6792+-0.0344         12.6775+-0.0641       
   math-spectral-norm                     3.3260+-0.0107    ^     3.1266+-0.0059       ^ definitely 1.0638x faster
   regexp-dna                            16.6620+-0.1701         16.4830+-0.1113         might be 1.0109x faster
   string-base64                          5.0312+-0.0447    ^     4.7826+-0.0561       ^ definitely 1.0520x faster
   string-fasta                           8.6124+-0.0827    ^     8.4399+-0.0109       ^ definitely 1.0204x faster
   string-tagcloud                       16.2052+-0.1513         16.1084+-0.1053       
   string-unpack-code                    28.6954+-0.0939    ^    27.5795+-0.0749       ^ definitely 1.0405x faster
   string-validate-input                  6.8491+-0.0488    ?     6.9466+-0.0508       ? might be 1.0142x slower

   <arithmetic> *                         8.4381+-0.0379    ^     8.2453+-0.0344       ^ definitely 1.0234x faster
   <geometric>                            6.7285+-0.0257    ^     6.5338+-0.0271       ^ definitely 1.0298x faster
   <harmonic>                             5.2671+-0.0166    ^     5.0645+-0.0213       ^ definitely 1.0400x faster

                                            TipOfTree              DontReparse                                   
V8:
   crypto                                96.4900+-0.2991    ^    94.1861+-0.2984       ^ definitely 1.0245x faster
   deltablue                            217.5844+-0.9899    ^   212.8585+-0.9464       ^ definitely 1.0222x faster
   earley-boyer                         130.9492+-1.0780    ^   127.8899+-1.0225       ^ definitely 1.0239x faster
   raytrace                              81.0926+-0.6513    ^    77.3365+-0.4481       ^ definitely 1.0486x faster
   regexp                               151.3493+-0.3147    ^   149.5843+-0.3025       ^ definitely 1.0118x faster
   richards                             173.5795+-1.5279    ^   168.0494+-0.3756       ^ definitely 1.0329x faster
   splay                                108.7153+-1.4730        106.6012+-1.2274         might be 1.0198x faster

   <arithmetic>                         137.1086+-0.5109    ^   133.7865+-0.2295       ^ definitely 1.0248x faster
   <geometric> *                        130.2653+-0.4899    ^   126.9433+-0.2811       ^ definitely 1.0262x faster
   <harmonic>                           123.8923+-0.4964    ^   120.5107+-0.3238       ^ definitely 1.0281x faster

                                            TipOfTree              DontReparse                                   
Kraken:
   ai-astar                             896.4849+-0.6290    ?   896.7507+-0.6768       ?
   audio-beat-detection                 256.6795+-1.3877    ?   257.9164+-3.2833       ?
   audio-dft                            313.6312+-2.6722    ?   314.6306+-3.0242       ?
   audio-fft                            166.9654+-0.4950        166.8619+-0.2442       
   audio-oscillator                     350.5585+-0.7851    !   353.6672+-2.3029       ! definitely 1.0089x slower
   imaging-darkroom                     403.5161+-5.9836        403.3743+-5.9462       
   imaging-desaturate                   291.2893+-0.1042        291.1998+-0.1519       
   imaging-gaussian-blur                750.8418+-0.1805    ?   753.1201+-3.0041       ?
   json-parse-financial                  86.7433+-0.7667    !    88.5801+-0.3610       ! definitely 1.0212x slower
   json-stringify-tinderbox              94.8303+-0.5800    !    96.2029+-0.6412       ! definitely 1.0145x slower
   stanford-crypto-aes                  139.4328+-0.8114        138.7555+-0.7377       
   stanford-crypto-ccm                  136.8853+-0.4446    !   138.3731+-0.9205       ! definitely 1.0109x slower
   stanford-crypto-pbkdf2               281.8990+-2.1521        280.7161+-2.1571       
   stanford-crypto-sha256-iterative     118.3287+-0.2864        118.2437+-0.4310       

   <arithmetic> *                       306.2919+-0.5709    ?   307.0280+-0.7822       ?
   <geometric>                          238.7151+-0.5075    ?   239.6552+-0.5550       ?
   <harmonic>                           192.2048+-0.5310    !   193.4355+-0.3946       ! definitely 1.0064x slower

                                            TipOfTree              DontReparse                                   
All benchmarks:
   <arithmetic>                         116.3242+-0.1934        115.9420+-0.2147       
   <geometric>                           30.2882+-0.0696    ^    29.7204+-0.0740       ^ definitely 1.0191x faster
   <harmonic>                             9.2782+-0.0285    ^     8.9281+-0.0367       ^ definitely 1.0392x faster

                                            TipOfTree              DontReparse                                   
Geomean of preferred means:
   <scaled-result>                       69.5663+-0.1551    ^    68.4953+-0.1164       ^ definitely 1.0156x faster
Comment 4 Filip Pizlo 2011-11-10 03:59:00 PST
And another computer…


Benchmark report for SunSpider, V8, and Kraken on nitroflex.local (MacBookPro8,2).

VMs tested:
"TipOfTree" at /Volumes/Data/pizlo/quinary/OpenSource/WebKitBuild/Release/jsc (r99833)
"DontReparse" at /Volumes/Data/pizlo/octonary/OpenSource/WebKitBuild/Release/jsc (r99833)

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              DontReparse                                   
SunSpider:
   3d-cube                                7.1160+-0.2107          7.0391+-0.1941         might be 1.0109x faster
   3d-morph                               7.4623+-0.1149          7.4498+-0.0730       
   3d-raytrace                            7.3824+-0.1565          7.1738+-0.1668         might be 1.0291x faster
   access-binary-trees                    1.5903+-0.0913          1.5834+-0.0779       
   access-fannkuch                        6.3282+-0.1507          6.1782+-0.1300         might be 1.0243x faster
   access-nbody                           3.6238+-0.0754    ^     3.4095+-0.0862       ^ definitely 1.0629x faster
   access-nsieve                          2.5397+-0.0578    ?     2.5805+-0.1049       ? might be 1.0161x slower
   bitops-3bit-bits-in-byte               1.2867+-0.0294          1.2578+-0.0243         might be 1.0230x faster
   bitops-bits-in-byte                    2.3013+-0.0503          2.2661+-0.0535         might be 1.0155x faster
   bitops-bitwise-and                     3.2182+-0.0962    ?     3.2186+-0.0473       ?
   bitops-nsieve-bits                     5.1653+-0.0542    ?     5.2901+-0.0994       ? might be 1.0242x slower
   controlflow-recursive                  2.1026+-0.0452          2.0529+-0.0553         might be 1.0243x faster
   crypto-aes                             7.4216+-0.1925    ^     6.9160+-0.2297       ^ definitely 1.0731x faster
   crypto-md5                             2.5961+-0.0629    ^     2.3522+-0.0650       ^ definitely 1.1037x faster
   crypto-sha1                            2.3015+-0.0560    ^     2.0612+-0.0614       ^ definitely 1.1166x faster
   date-format-tofte                      9.9099+-0.3246          9.8278+-0.1697       
   date-format-xparb                      9.2049+-0.2389    ?     9.6297+-0.2415       ? might be 1.0461x slower
   math-cordic                            6.4217+-0.0867    ?     6.4958+-0.0747       ? might be 1.0115x slower
   math-partial-sums                      7.5150+-0.1556    ?     7.5324+-0.0968       ?
   math-spectral-norm                     2.5063+-0.0604    ^     2.3266+-0.0575       ^ definitely 1.0773x faster
   regexp-dna                            11.5327+-0.2318         11.5130+-0.2307       
   string-base64                          3.9163+-0.1086    ^     3.7127+-0.0613       ^ definitely 1.0548x faster
   string-fasta                           6.2300+-0.1228    ?     6.3211+-0.1558       ? might be 1.0146x slower
   string-tagcloud                       11.7260+-0.2376         11.3764+-0.2851         might be 1.0307x faster
   string-unpack-code                    20.3561+-0.3019         19.9831+-0.4012         might be 1.0187x faster
   string-validate-input                  5.1792+-0.1294          5.1156+-0.0853         might be 1.0124x faster

   <arithmetic> *                         6.0359+-0.0345    ^     5.9486+-0.0316       ^ definitely 1.0147x faster
   <geometric>                            4.8367+-0.0247    ^     4.7343+-0.0335       ^ definitely 1.0216x faster
   <harmonic>                             3.8515+-0.0262    ^     3.7419+-0.0376       ^ definitely 1.0293x faster

                                            TipOfTree              DontReparse                                   
V8:
   crypto                                71.4460+-0.3823    ^    69.7978+-0.5671       ^ definitely 1.0236x faster
   deltablue                            157.8425+-0.6965    ^   154.9324+-0.8671       ^ definitely 1.0188x faster
   earley-boyer                          87.6959+-0.7451    ^    85.4942+-0.8234       ^ definitely 1.0258x faster
   raytrace                              59.1921+-0.4369    ^    56.6614+-0.5331       ^ definitely 1.0447x faster
   regexp                               105.1183+-0.6274    ^   103.9579+-0.3793       ^ definitely 1.0112x faster
   richards                             122.5364+-0.3778    ^   120.4521+-0.3004       ^ definitely 1.0173x faster
   splay                                 70.8957+-0.6718    ?    71.7627+-1.3166       ? might be 1.0122x slower

   <arithmetic>                          96.3896+-0.1855    ^    94.7226+-0.2461       ^ definitely 1.0176x faster
   <geometric> *                         91.4376+-0.2092    ^    89.7871+-0.2735       ^ definitely 1.0184x faster
   <harmonic>                            87.0474+-0.2357    ^    85.3657+-0.3124       ^ definitely 1.0197x faster

                                            TipOfTree              DontReparse                                   
Kraken:
   ai-astar                             488.4696+-3.7278        486.2353+-2.2241       
   audio-beat-detection                 187.0626+-0.4844        186.4867+-0.7610       
   audio-dft                            262.7114+-2.6286        259.0819+-2.1574         might be 1.0140x faster
   audio-fft                            121.3754+-0.3667    !   122.9276+-0.9519       ! definitely 1.0128x slower
   audio-oscillator                     247.5074+-0.9471    ?   247.8469+-1.0463       ?
   imaging-darkroom                     296.4649+-4.2409    ?   296.6776+-4.4837       ?
   imaging-desaturate                   221.6823+-0.6434        221.5570+-0.7926       
   imaging-gaussian-blur                546.3750+-1.4749        544.4993+-1.1686       
   json-parse-financial                  56.9925+-0.2459    ?    57.1872+-0.1117       ?
   json-stringify-tinderbox              67.6576+-0.1270    ?    67.7753+-0.5094       ?
   stanford-crypto-aes                   94.2014+-0.4049    ?    95.0778+-0.5380       ?
   stanford-crypto-ccm                   97.8710+-0.7897    ?    98.1923+-0.5955       ?
   stanford-crypto-pbkdf2               186.9708+-0.6859    !   189.3529+-1.1652       ! definitely 1.0127x slower
   stanford-crypto-sha256-iterative      79.0255+-0.3167         78.6344+-0.1717       

   <arithmetic> *                       211.0262+-0.4791        210.8237+-0.3195       
   <geometric>                          167.4214+-0.2784    ?   167.5952+-0.2236       ?
   <harmonic>                           134.1494+-0.1889    ?   134.4629+-0.1789       ?

                                            TipOfTree              DontReparse                                   
All benchmarks:
   <arithmetic>                          80.5538+-0.1423    ^    80.1969+-0.1096       ^ definitely 1.0045x faster
   <geometric>                           21.5367+-0.0629    ^    21.2318+-0.0878       ^ definitely 1.0144x faster
   <harmonic>                             6.7768+-0.0449    ^     6.5878+-0.0646       ^ definitely 1.0287x faster

                                            TipOfTree              DontReparse                                   
Geomean of preferred means:
   <scaled-result>                       48.8348+-0.0937    ^    48.2887+-0.1133       ^ definitely 1.0113x faster
Comment 5 Filip Pizlo 2011-11-10 04:03:28 PST
And another….


Benchmark report for SunSpider, V8, and Kraken on bigmac.local (MacPro5,1).

VMs tested:
"TipOfTree" at /Volumes/Data/pizlo/quinary/OpenSource/WebKitBuild/Release/jsc (r99833)
"DontReparse" at /Volumes/Data/pizlo/octonary/OpenSource/WebKitBuild/Release/jsc (r99833)

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              DontReparse                                   
SunSpider:
   3d-cube                                7.6214+-0.0260    ^     7.4704+-0.0339       ^ definitely 1.0202x faster
   3d-morph                               8.5726+-0.2168          8.5425+-0.1181       
   3d-raytrace                            8.0998+-0.0556    ^     7.8017+-0.0736       ^ definitely 1.0382x faster
   access-binary-trees                    1.6547+-0.0060    ^     1.5997+-0.0131       ^ definitely 1.0344x faster
   access-fannkuch                        7.7523+-0.0579          7.6534+-0.0855         might be 1.0129x faster
   access-nbody                           4.3934+-0.0093    ^     4.1874+-0.0198       ^ definitely 1.0492x faster
   access-nsieve                          3.1666+-0.0123          3.1577+-0.0385       
   bitops-3bit-bits-in-byte               1.3018+-0.0160    ^     1.2567+-0.0156       ^ definitely 1.0359x faster
   bitops-bits-in-byte                    5.1806+-0.0588    ^     5.0844+-0.0243       ^ definitely 1.0189x faster
   bitops-bitwise-and                     3.3273+-0.0264          3.3032+-0.0160       
   bitops-nsieve-bits                     5.6694+-0.0387    ?     5.7061+-0.0751       ?
   controlflow-recursive                  2.3453+-0.0259          2.3022+-0.0323         might be 1.0187x faster
   crypto-aes                             7.8643+-0.1285    ^     7.1920+-0.0663       ^ definitely 1.0935x faster
   crypto-md5                             2.8297+-0.0241    ^     2.5823+-0.0402       ^ definitely 1.0958x faster
   crypto-sha1                            2.5183+-0.0171    ^     2.2355+-0.0192       ^ definitely 1.1265x faster
   date-format-tofte                     10.7092+-0.0645    ?    10.9788+-0.3545       ? might be 1.0252x slower
   date-format-xparb                     10.1238+-0.1178    ?    10.1934+-0.2675       ?
   math-cordic                            8.0333+-0.2888    ^     7.4030+-0.2621       ^ definitely 1.0852x faster
   math-partial-sums                     10.5474+-0.0294    ?    10.5618+-0.0540       ?
   math-spectral-norm                     2.7740+-0.0234    ^     2.6236+-0.0308       ^ definitely 1.0573x faster
   regexp-dna                            13.4838+-0.2007         13.3818+-0.1448       
   string-base64                          4.2050+-0.1004    ^     3.9436+-0.0208       ^ definitely 1.0663x faster
   string-fasta                           7.1460+-0.0195          7.1042+-0.0590       
   string-tagcloud                       13.3047+-0.0846    ?    13.3969+-0.1912       ?
   string-unpack-code                    23.2199+-0.1831    ^    22.3907+-0.1962       ^ definitely 1.0370x faster
   string-validate-input                  5.7297+-0.0603    ?     5.7545+-0.0318       ?

   <arithmetic> *                         6.9836+-0.0242    ^     6.8387+-0.0298       ^ definitely 1.0212x faster
   <geometric>                            5.6027+-0.0181    ^     5.4455+-0.0190       ^ definitely 1.0289x faster
   <harmonic>                             4.4005+-0.0171    ^     4.2394+-0.0178       ^ definitely 1.0380x faster

                                            TipOfTree              DontReparse                                   
V8:
   crypto                                80.2850+-0.3858    ^    78.1313+-0.2512       ^ definitely 1.0276x faster
   deltablue                            180.4189+-0.7193    ?   180.5901+-1.6778       ?
   earley-boyer                         109.3444+-0.6908    ^   106.7310+-0.9064       ^ definitely 1.0245x faster
   raytrace                              67.1571+-0.5759    ^    63.9823+-0.3642       ^ definitely 1.0496x faster
   regexp                               127.2048+-0.4775    ^   125.1099+-0.4190       ^ definitely 1.0167x faster
   richards                             143.2527+-0.4555    ^   139.8843+-0.6572       ^ definitely 1.0241x faster
   splay                                 92.1319+-1.3306         91.2307+-1.3058       

   <arithmetic>                         114.2564+-0.1739    ^   112.2371+-0.3763       ^ definitely 1.0180x faster
   <geometric> *                        108.6299+-0.2137    ^   106.3404+-0.3306       ^ definitely 1.0215x faster
   <harmonic>                           103.3378+-0.2485    ^   100.7975+-0.3126       ^ definitely 1.0252x faster

                                            TipOfTree              DontReparse                                   
Kraken:
   ai-astar                             811.1446+-12.1836   !   829.6888+-1.2981       ! definitely 1.0229x slower
   audio-beat-detection                 210.4634+-0.3345        210.3719+-0.3645       
   audio-dft                            267.3029+-8.5981        263.2959+-2.3874         might be 1.0152x faster
   audio-fft                            137.5691+-0.3430    ?   137.5746+-0.3763       ?
   audio-oscillator                     292.3153+-2.9677        290.9230+-0.8406       
   imaging-darkroom                     334.1868+-4.7150    ?   335.8100+-4.9925       ?
   imaging-desaturate                   241.3295+-0.2320    ?   241.5680+-0.2330       ?
   imaging-gaussian-blur                622.9331+-0.6125    ?   622.9645+-0.7185       ?
   json-parse-financial                  71.5099+-0.2457    !    72.7217+-0.2078       ! definitely 1.0169x slower
   json-stringify-tinderbox              78.9935+-0.5460    !    79.9847+-0.3438       ! definitely 1.0125x slower
   stanford-crypto-aes                  116.4337+-0.5026        115.5108+-1.2490       
   stanford-crypto-ccm                  117.1500+-3.0983        115.0906+-0.5055         might be 1.0179x faster
   stanford-crypto-pbkdf2               232.4978+-0.4830    ^   231.4326+-0.5179       ^ definitely 1.0046x faster
   stanford-crypto-sha256-iterative      97.9107+-0.2340         97.7429+-0.3122       

   <arithmetic> *                       259.4100+-0.7933    ?   260.3343+-0.3323       ?
   <geometric>                          199.7779+-0.5859    ?   199.8830+-0.2001       ?
   <harmonic>                           160.0671+-0.5030    ?   160.2880+-0.1659       ?

                                            TipOfTree              DontReparse                                   
All benchmarks:
   <arithmetic>                          98.1513+-0.2377         98.0457+-0.1025       
   <geometric>                           25.2640+-0.0594    ^    24.7944+-0.0549       ^ definitely 1.0189x faster
   <harmonic>                             7.7511+-0.0296    ^     7.4725+-0.0307       ^ definitely 1.0373x faster

                                            TipOfTree              DontReparse                                   
Geomean of preferred means:
   <scaled-result>                       58.1661+-0.1051    ^    57.4204+-0.1284       ^ definitely 1.0130x faster
Comment 6 Filip Pizlo 2011-11-10 04:07:25 PST
Created attachment 114468 [details]
the patch
Comment 7 Filip Pizlo 2011-11-10 04:11:28 PST
<rdar://problem/10425588>
Comment 8 Geoffrey Garen 2011-11-10 12:06:47 PST
Comment on attachment 114468 [details]
the patch

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

r=me, with some comments below.

> Source/JavaScriptCore/bytecode/CodeBlock.cpp:1655
> +    if (hasInstructions() && m_shouldDiscardBytecodeLater)

The "Later" in "m_shouldDiscardBytecodeLater" confused me because it sounded like setting the flag was a way to delay or prevent discarding bytecode ("Not now, later"), but actually it's the way to cause bytecode to be discarded. I think "m_shouldDiscardBytecode" would be better.

> Source/JavaScriptCore/dfg/DFGByteCodeCache.h:68
> +        // This is likely to be good enough.

Sneaky, but true. I would remove this comment, or add a "why" explanation.

Thinking this through, I wonder if | might be clearer than ^ here. This hash function really just relies on the low bits of m_executable being zero, and the low bits of m_kind being meaningful.

> Source/JavaScriptCore/heap/ListableHandler.h:60
> +        void addNotThreadSafe(T* handler)

Please make this private.

> Source/JavaScriptCore/heap/UltraWeakFinalizer.h:37
> +class UltraWeakFinalizer : public ListableHandler<UltraWeakFinalizer> {

The "Ultra" in "UltraWeak" didn't do enough to tell me what this class is. To my mind, two things are unique about this kind of "finalizer": (1) It fires no matter what, even if the owning object is live; (2) If GC happens soon enough, it might prefer not to fire, thus avoiding cache churn. Based on those facts, I'd suggest:

- CacheOwner (with a "clearCache" callback)
OR
- UnconditionalFinalizer (with a "finalize" callback)
Comment 9 Filip Pizlo 2011-11-10 14:03:20 PST
> The "Later" in "m_shouldDiscardBytecodeLater" confused me because it sounded like setting the flag was a way to delay or prevent discarding bytecode ("Not now, later"), but actually it's the way to cause bytecode to be discarded. I think "m_shouldDiscardBytecode" would be better.

I dropped the Later.

> 
> > Source/JavaScriptCore/dfg/DFGByteCodeCache.h:68
> > +        // This is likely to be good enough.
> 
> Sneaky, but true. I would remove this comment, or add a "why" explanation.
> 
> Thinking this through, I wonder if | might be clearer than ^ here. This hash function really just relies on the low bits of m_executable being zero, and the low bits of m_kind being meaningful.

I dropped the comment.  It's actually less sneaky and even more correct: I'm using the PtrHash, which picks an appropriate int hash, so even just the pointer hash has all bits populated with some manner of rubbish.  Then I xor in the mode enum, which flips the low bit.

It's only "likely" to be good enough because if someone changed the hash table implementation to use high bits instead of low bits in the mask, then this would become slightly sub-optimal.  But even then it would only be *slightly* sub-optimal because it would mean collisions when you want to inline an executable for both a call and a construction.  And even then that collision probably won't cost you anything since the dominant cost of inlining is, well, inlining.  So we're starting to enter into the unlikely * unlikely * unlikely domain of probabilities.

> 
> > Source/JavaScriptCore/heap/ListableHandler.h:60
> > +        void addNotThreadSafe(T* handler)
> 
> Please make this private.

I did, though probably one of my next patches will make it public again.  The WeakReferenceHarvester list should be processed multiple times, and the easiest way to do that is for the GC to consume the global one into a private one, which can be added to without locking.

> 
> > Source/JavaScriptCore/heap/UltraWeakFinalizer.h:37
> > +class UltraWeakFinalizer : public ListableHandler<UltraWeakFinalizer> {
> 
> The "Ultra" in "UltraWeak" didn't do enough to tell me what this class is. To my mind, two things are unique about this kind of "finalizer": (1) It fires no matter what, even if the owning object is live; (2) If GC happens soon enough, it might prefer not to fire, thus avoiding cache churn. Based on those facts, I'd suggest:
> 
> - CacheOwner (with a "clearCache" callback)
> OR
> - UnconditionalFinalizer (with a "finalize" callback)

I picked UnconditionalFinalizer, and made the callback "finalizeUnconditionally", since I did not want CodeBlock to have a method called "finalize".  It would be hard to tell where that method came from.
Comment 10 Filip Pizlo 2011-11-10 14:04:25 PST
Landed in http://trac.webkit.org/changeset/99898