Bug 67354 - ValueProfile does not make it safe to introspect cell values after garbage collection
Summary: ValueProfile does not make it safe to introspect cell values after garbage co...
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-08-31 20:28 PDT by Filip Pizlo
Modified: 2011-09-02 23:59 PDT (History)
6 users (show)

See Also:


Attachments
the patch (24.04 KB, patch)
2011-08-31 20:32 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch - fix style, build, conflicts (22.71 KB, patch)
2011-08-31 20:37 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch - simplifications (21.68 KB, patch)
2011-08-31 22:30 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch - fix a silly bug (21.92 KB, patch)
2011-08-31 23:41 PDT, Filip Pizlo
barraclough: review+
barraclough: commit-queue-
Details | Formatted Diff | Diff
the patch - fix review (22.00 KB, patch)
2011-09-02 15:59 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch - fix conflict (22.30 KB, patch)
2011-09-02 19:42 PDT, Filip Pizlo
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
the patch - fix more conflicts (22.05 KB, patch)
2011-09-02 21:06 PDT, Filip Pizlo
no flags 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-08-31 20:28:05 PDT
The ValueProfile class holds 8 recent values seen at a particular JavaScript bytecode site.  But if a value is a JSCell pointer, then after a garbage collection the pointer may no longer be valid.  This prevents clients of the profiler from doing introspection of values beyond checking if they are cells.  For example, it's not currently safe to check if a value is an array.

The ValueProfile should weakly reference JSCells, but should do it intelligently, so that if the JSCell is deleted by the collector, we still have some information about it: like, what was its structure, or if the structure is also deleted, then what is its class.
Comment 1 Filip Pizlo 2011-08-31 20:32:20 PDT
Created attachment 105892 [details]
the patch

Still running tests, so this is somewhat of a work in progress.
Comment 2 WebKit Review Bot 2011-08-31 20:34:02 PDT
Attachment 105892 [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/heap/Heap.cpp:591:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/JavaScriptCore/heap/WeakReferenceHarvester.h:75:  "stdint.h" already included at Source/JavaScriptCore/heap/WeakReferenceHarvester.h:23  [build/include] [4]
Total errors found: 2 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Filip Pizlo 2011-08-31 20:37:22 PDT
Created attachment 105893 [details]
the patch - fix style, build, conflicts
Comment 4 Filip Pizlo 2011-08-31 21:02:47 PDT
Comment on attachment 105893 [details]
the patch - fix style, build, conflicts

Passes tests, and does not break the web.  Still plan to do some benchmarking and some more tests of various permutations of profiling enabled.  But it's ready for review.
Comment 5 Filip Pizlo 2011-08-31 21:42:10 PDT
Comment on attachment 105893 [details]
the patch - fix style, build, conflicts

Tests pass and the web is usable no matter what permutation of value profiling options I use.  One task left: check performance.
Comment 6 Filip Pizlo 2011-08-31 22:30:49 PDT
Created attachment 105895 [details]
the patch - simplifications

Simplified the WeakReferenceHarvester API
Comment 7 Filip Pizlo 2011-08-31 22:31:32 PDT
This patch is performance-neutral.  Only differences are ~0.1% and not statistically significant.


Benchmark report for SunSpider, V8, and Kraken.

VMs tested:
"TipOfTree" at /Volumes/Data/fromMiniMe/quinary/OpenSource/WebKitBuild/Release/jsc
"ValueProfileOff" at /Volumes/Data/fromMiniMe/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            ValueProfileOff                                 
SunSpider:
   3d-cube                                8.2470+-0.0678    ?     8.2512+-0.0164       ?
   3d-morph                               8.1477+-0.0334          8.1383+-0.0191       
   3d-raytrace                            8.2481+-0.0654    ?     8.3196+-0.0619       ?
   access-binary-trees                    2.5407+-0.0381    ?     2.5602+-0.0468       ?
   access-fannkuch                       12.9529+-0.0845         12.9174+-0.0933       
   access-nbody                           4.9614+-0.0487          4.9296+-0.0202       
   access-nsieve                          3.0567+-0.0331    ?     3.0604+-0.0294       ?
   bitops-3bit-bits-in-byte               1.8573+-0.0223          1.8571+-0.0231       
   bitops-bits-in-byte                    5.6315+-0.1239          5.6254+-0.1335       
   bitops-bitwise-and                     4.0975+-0.0087          4.0974+-0.0040       
   bitops-nsieve-bits                     5.6903+-0.0320    ?     5.7280+-0.0336       ?
   controlflow-recursive                  2.2887+-0.0390    ?     2.2975+-0.0341       ?
   crypto-aes                             6.8440+-0.0369    ?     6.8944+-0.0393       ?
   crypto-md5                             3.0490+-0.0421          3.0027+-0.0277         might be 1.0154x faster
   crypto-sha1                            2.4497+-0.0374    ?     2.4579+-0.0359       ?
   date-format-tofte                     10.8528+-0.0671         10.8410+-0.0551       
   date-format-xparb                      9.5181+-0.1963    ?     9.5994+-0.1459       ?
   math-cordic                            7.1568+-0.0921    ?     7.1719+-0.1217       ?
   math-partial-sums                     10.6718+-0.0497    ?    10.6829+-0.0345       ?
   math-spectral-norm                     2.7385+-0.0336          2.7294+-0.0277       
   regexp-dna                            12.0845+-0.1535         12.0535+-0.1141       
   string-base64                          6.5590+-0.1419          6.4790+-0.0848         might be 1.0123x faster
   string-fasta                           8.2849+-0.0352    ?     8.3050+-0.0290       ?
   string-tagcloud                       15.0656+-0.0444         14.9926+-0.0728       
   string-unpack-code                    20.8166+-0.1082         20.7653+-0.0622       
   string-validate-input                  7.3637+-0.3009    ?     7.6644+-0.2635       ? might be 1.0408x slower

   <arithmetic>                           7.3529+-0.0327    ?     7.3624+-0.0254       ?
   <geometric>                            6.0956+-0.0316    ?     6.1052+-0.0280       ?
   <harmonic>                             4.9775+-0.0342    ?     4.9834+-0.0324       ?

                                            TipOfTree            ValueProfileOff                                 
V8:
   crypto                               102.7447+-0.2485    ?   103.1249+-0.2542       ?
   deltablue                            298.6366+-3.7481    ?   299.4765+-2.1650       ?
   earley-boyer                         123.2329+-0.4334        122.7717+-0.3203       
   raytrace                              87.9043+-0.9485         87.6650+-0.1515       
   regexp                               130.8935+-0.4964    !   131.9634+-0.1790       ! definitely 1.0082x slower
   richards                             302.3704+-1.4350    ?   302.8294+-1.5887       ?
   splay                                155.8481+-1.6318    ?   156.0731+-0.9100       ?

   <arithmetic>                         171.6615+-0.4166    ?   171.9863+-0.3329       ?
   <geometric>                          153.9464+-0.1610    ?   154.1990+-0.1916       ?
   <harmonic>                           140.1632+-0.2503    ?   140.3450+-0.1274       ?

                                            TipOfTree            ValueProfileOff                                 
Kraken:
   ai-astar                            1668.8957+-15.1645      1658.5114+-18.9078      
   audio-beat-detection                 542.8552+-3.1601        540.6212+-3.8099       
   audio-dft                            454.6839+-2.3303    ?   454.8397+-2.0129       ?
   audio-fft                            420.1154+-0.7348    !   423.3887+-1.6151       ! definitely 1.0078x slower
   audio-oscillator                     403.3020+-0.8085    ?   404.7480+-0.9289       ?
   imaging-darkroom                     600.3491+-16.2923   ?   613.3811+-10.6584      ? might be 1.0217x slower
   imaging-desaturate                   636.5449+-17.4696       636.1784+-17.4582      
   imaging-gaussian-blur               1859.3143+-3.9366       1855.5966+-4.3500       
   json-parse-financial                  61.4857+-0.2132    !    63.2649+-0.1963       ! definitely 1.0289x slower
   json-stringify-tinderbox              76.2311+-0.4255         75.8158+-0.2337       
   stanford-crypto-aes                  165.7540+-0.4441    ?   166.3629+-0.5324       ?
   stanford-crypto-ccm                  131.5271+-0.6480    ?   132.6696+-0.9418       ?
   stanford-crypto-pbkdf2               374.2624+-1.6078    ?   374.7365+-1.2595       ?
   stanford-crypto-sha256-iterative     144.7434+-0.6826        144.1186+-0.2453       

   <arithmetic>                         538.5760+-1.2589    ?   538.8738+-2.0228       ?
   <geometric>                          339.8348+-0.8563    ?   341.1243+-1.0454       ?
   <harmonic>                           212.5037+-0.4490    !   214.1243+-0.4311       ! definitely 1.0076x slower

                                            TipOfTree            ValueProfileOff                                 
All benchmarks:
   <arithmetic>                         190.0611+-0.3985    ?   190.2034+-0.5963       ?
   <geometric>                           32.6614+-0.1152    ?    32.7347+-0.1047       ?
   <harmonic>                             8.8026+-0.0593    ?     8.8137+-0.0562       ?
Comment 8 Filip Pizlo 2011-08-31 22:41:18 PDT
This is the overhead of value profiling with this patch, with DFG turned off.  As in, it's the overhead that baseline (before tier-up) code will experience.


Benchmark report for SunSpider, V8, and Kraken.

VMs tested:
"TipOfTree" at /Volumes/Data/fromMiniMe/quinary/OpenSource/WebKitBuild/Release/jsc
"ValueProfileOff" at /Volumes/Data/fromMiniMe/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            ValueProfileOff                                 
SunSpider:
   3d-cube                                8.8595+-0.0179    !     9.2453+-0.0312       ! definitely 1.0435x slower
   3d-morph                               8.1416+-0.0307    ?     8.2134+-0.0447       ?
   3d-raytrace                            9.4134+-0.0248    !    10.4939+-0.0476       ! definitely 1.1148x slower
   access-binary-trees                    2.2512+-0.0197    !     2.5353+-0.0231       ! definitely 1.1262x slower
   access-fannkuch                       12.9424+-0.0835    !    13.6188+-0.0511       ! definitely 1.0523x slower
   access-nbody                           7.4584+-0.0102    !     8.0224+-0.0572       ! definitely 1.0756x slower
   access-nsieve                          3.8109+-0.0260    ?     3.8529+-0.0177       ? might be 1.0110x slower
   bitops-3bit-bits-in-byte               2.5576+-0.0398    !     2.6751+-0.0281       ! definitely 1.0459x slower
   bitops-bits-in-byte                    6.6623+-0.1132    ^     6.0954+-0.1115       ^ definitely 1.0930x faster
   bitops-bitwise-and                     4.1196+-0.0338    ?     4.1530+-0.0457       ?
   bitops-nsieve-bits                     5.6830+-0.0388    !     5.9572+-0.0279       ! definitely 1.0482x slower
   controlflow-recursive                  2.2253+-0.0410    !     2.5391+-0.0337       ! definitely 1.1410x slower
   crypto-aes                             7.1616+-0.0281    !     7.6889+-0.0542       ! definitely 1.0736x slower
   crypto-md5                             3.1589+-0.0465    !     3.4858+-0.0329       ! definitely 1.1035x slower
   crypto-sha1                            2.6565+-0.0305    !     2.9111+-0.0303       ! definitely 1.0958x slower
   date-format-tofte                     11.7682+-0.0902    !    12.1948+-0.0730       ! definitely 1.0362x slower
   date-format-xparb                      9.6990+-0.1297    ?    10.0291+-0.2465       ? might be 1.0340x slower
   math-cordic                            7.3453+-0.3298    ?     7.3599+-0.0501       ?
   math-partial-sums                     10.4958+-0.0438         10.4889+-0.0372       
   math-spectral-norm                     4.4546+-0.0169    !     4.7794+-0.0634       ! definitely 1.0729x slower
   regexp-dna                            12.0460+-0.1184    ?    12.0732+-0.1311       ?
   string-base64                          5.7189+-0.0666    !     6.0201+-0.0766       ! definitely 1.0527x slower
   string-fasta                           7.9478+-0.0248    !     8.0574+-0.0268       ! definitely 1.0138x slower
   string-tagcloud                       15.2231+-0.0732    ?    15.3147+-0.0470       ?
   string-unpack-code                    20.5997+-0.0690    !    20.9489+-0.1046       ! definitely 1.0170x slower
   string-validate-input                  7.0806+-0.0941    ?     7.2559+-0.1048       ? might be 1.0247x slower

   <arithmetic>                           7.6724+-0.0358    !     7.9235+-0.0279       ! definitely 1.0327x slower
   <geometric>                            6.5102+-0.0354    !     6.7849+-0.0279       ! definitely 1.0422x slower
   <harmonic>                             5.4322+-0.0355    !     5.7459+-0.0297       ! definitely 1.0577x slower

                                            TipOfTree            ValueProfileOff                                 
V8:
   crypto                               206.8537+-0.4849    !   221.1158+-0.9468       ! definitely 1.0689x slower
   deltablue                            267.2850+-0.8388    !   318.9918+-1.8254       ! definitely 1.1935x slower
   earley-boyer                         126.5721+-0.3546    !   139.5270+-0.5612       ! definitely 1.1024x slower
   raytrace                              86.8362+-1.7937    !    93.1722+-0.8733       ! definitely 1.0730x slower
   regexp                               131.3743+-0.5879    ?   132.2735+-0.5262       ?
   richards                             279.6937+-0.7931    !   312.9718+-1.1768       ! definitely 1.1190x slower
   splay                                155.0793+-1.1745    ?   157.0248+-1.3658       ? might be 1.0125x slower

   <arithmetic>                         179.0992+-0.4639    !   196.4396+-0.5115       ! definitely 1.0968x slower
   <geometric>                          165.9162+-0.6798    !   179.3075+-0.4987       ! definitely 1.0807x slower
   <harmonic>                           153.3871+-0.9578    !   163.9957+-0.5743       ! definitely 1.0692x slower

                                            TipOfTree            ValueProfileOff                                 
Kraken:
   ai-astar                            2072.9013+-30.9877   !  2196.3937+-15.3871      ! definitely 1.0596x slower
   audio-beat-detection                 539.6634+-2.2724    !   568.2251+-1.1907       ! definitely 1.0529x slower
   audio-dft                            454.8677+-4.2642    !   488.1209+-3.1658       ! definitely 1.0731x slower
   audio-fft                            421.0421+-0.9320    !   442.3705+-0.5048       ! definitely 1.0507x slower
   audio-oscillator                     379.6714+-5.3440    !   411.9139+-1.8822       ! definitely 1.0849x slower
   imaging-darkroom                     590.8199+-7.1784    ?   597.6895+-3.8731       ? might be 1.0116x slower
   imaging-desaturate                   617.4223+-2.4087    !   649.5490+-4.5528       ! definitely 1.0520x slower
   imaging-gaussian-blur               2176.1665+-7.1226    !  2388.3892+-8.3513       ! definitely 1.0975x slower
   json-parse-financial                  61.9086+-0.2256    !    62.5446+-0.2643       ! definitely 1.0103x slower
   json-stringify-tinderbox              76.1372+-0.2654         75.5563+-0.3188       
   stanford-crypto-aes                  166.0477+-0.6312    !   181.3763+-0.5867       ! definitely 1.0923x slower
   stanford-crypto-ccm                  132.4801+-1.1672    !   143.9892+-0.2739       ! definitely 1.0869x slower
   stanford-crypto-pbkdf2               434.5020+-2.4759    !   441.8006+-2.5230       ! definitely 1.0168x slower
   stanford-crypto-sha256-iterative     160.7851+-0.5561    !   165.1758+-0.5175       ! definitely 1.0273x slower

   <arithmetic>                         591.7440+-2.0751    !   629.5067+-1.1978       ! definitely 1.0638x slower
   <geometric>                          353.0441+-0.4950    !   370.7383+-0.4695       ! definitely 1.0501x slower
   <harmonic>                           216.3901+-0.4013    !   223.6948+-0.3026       ! definitely 1.0338x slower

                                            TipOfTree            ValueProfileOff                                 
All benchmarks:
   <arithmetic>                         207.1828+-0.6364    !   221.1528+-0.3620       ! definitely 1.0674x slower
   <geometric>                           34.6434+-0.1193    !    36.3830+-0.0929       ! definitely 1.0502x slower
   <harmonic>                             9.5985+-0.0615    !    10.1506+-0.0515       ! definitely 1.0575x slower
Comment 9 Filip Pizlo 2011-08-31 22:42:10 PDT
(In reply to comment #8)
>                                             TipOfTree            ValueProfileOff                                 

That's a typo - it should be ValueProfileOn.
Comment 10 Filip Pizlo 2011-08-31 22:59:16 PDT
Comment on attachment 105895 [details]
the patch - simplifications

Everything looks good.
Comment 11 Filip Pizlo 2011-08-31 23:41:09 PDT
Created attachment 105903 [details]
the patch - fix a silly bug

Fixed a problem where numberOfSamples() neglected to count buckets that got weakened.
Comment 12 Gavin Barraclough 2011-09-02 14:30:54 PDT
Comment on attachment 105903 [details]
the patch - fix a silly bug

Per discussion please fix the two ptr&flags types to work the same way, either +1/-1 or |1/&~1.
Comment 13 Filip Pizlo 2011-09-02 15:59:38 PDT
Created attachment 106213 [details]
the patch - fix review
Comment 14 Filip Pizlo 2011-09-02 19:42:48 PDT
Created attachment 106239 [details]
the patch - fix conflict
Comment 15 Filip Pizlo 2011-09-02 19:46:37 PDT
Comment on attachment 106239 [details]
the patch - fix conflict

Looks like tests are still passing and conflicts are resolved ... ready to land.
Comment 16 WebKit Review Bot 2011-09-02 21:02:29 PDT
Comment on attachment 106239 [details]
the patch - fix conflict

Rejecting attachment 106239 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1

Last 500 characters of output:
ommit-queue/Tools/Scripts/webkitpy/tool/commands/stepsequence.py", line 64, in _run
    step(tool, options).run(state)
  File "/mnt/git/webkit-commit-queue/Tools/Scripts/webkitpy/tool/steps/validatereviewer.py", line 57, in run
    if self._has_valid_reviewer(changelog_entry):
  File "/mnt/git/webkit-commit-queue/Tools/Scripts/webkitpy/tool/steps/validatereviewer.py", line 42, in _has_valid_reviewer
    if changelog_entry.reviewer():
AttributeError: 'NoneType' object has no attribute 'reviewer'

Full output: http://queues.webkit.org/results/9583839
Comment 17 Filip Pizlo 2011-09-02 21:06:46 PDT
Created attachment 106242 [details]
the patch - fix more conflicts
Comment 18 WebKit Review Bot 2011-09-02 22:13:12 PDT
Comment on attachment 106242 [details]
the patch - fix more conflicts

Clearing flags on attachment: 106242

Committed r94477: <http://trac.webkit.org/changeset/94477>
Comment 19 WebKit Review Bot 2011-09-02 22:13:17 PDT
All reviewed patches have been landed.  Closing bug.
Comment 22 Ryosuke Niwa 2011-09-02 23:59:52 PDT
(In reply to comment #21)
> (In reply to comment #20)
> > It seems like this broke a whole bunch of tests.
> > http://build.webkit.org/builders/SnowLeopard%20Intel%20Debug%20%28Tests%29/builds/1984
> > http://build.webkit.org/builders/GTK%20Linux%2064-bit%20Debug/builds/25709
> > http://build.webkit.org/builders/Qt%20Linux%20Release/builds/37117
> 
> It looks to me like the change started earlier, possibly r94457.
> http://build.webkit.org/builders/Qt%20Linux%20Release?numbuilds=50

You're right. I was fooled about build failures. It appears that the failure is caused by http://trac.webkit.org/changeset/94453.