We pin property maps when we do a structure transition that cannot be cached. But we forget to clear m_previous. This results in increased heap usage (more structures being kept alive), worse performance (more structures to trace on GC), and crashes when we try to rematerialize property maps (rematerialization asserts that m_previous is clear).
Performance improvement: Benchmark report for SunSpider, V8, and Kraken. VMs tested: "TipOfTree" at /Volumes/Data/pizlo/tertiary/OpenSource/WebKitBuild/Release/jsc "ResetPrevious" at /Volumes/Data/pizlo/secondary/OpenSource/WebKitBuild/Release/jsc Collected 60 samples per benchmark/VM, with 20 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 ResetPrevious SunSpider: 3d-cube 7.9923+-0.0130 7.9780+-0.0098 3d-morph 8.1031+-0.0331 ? 8.1492+-0.0341 ? 3d-raytrace 8.2257+-0.0343 ? 8.2682+-0.0505 ? access-binary-trees 1.8011+-0.0043 1.7997+-0.0024 access-fannkuch 7.7586+-0.0323 ^ 7.6967+-0.0071 ^ definitely 1.0080x faster access-nbody 4.1964+-0.0078 ? 4.2001+-0.0053 ? access-nsieve 3.2255+-0.0072 ? 3.2338+-0.0073 ? bitops-3bit-bits-in-byte 1.7317+-0.0031 ? 1.7322+-0.0028 ? bitops-bits-in-byte 5.1787+-0.0204 5.1730+-0.0166 bitops-bitwise-and 3.5043+-0.0181 3.5020+-0.0122 bitops-nsieve-bits 5.6592+-0.0136 5.6515+-0.0139 controlflow-recursive 2.2414+-0.0021 ? 2.2447+-0.0047 ? crypto-aes 6.8406+-0.0503 6.8229+-0.0252 crypto-md5 2.9937+-0.0095 2.9877+-0.0063 crypto-sha1 2.7357+-0.0070 ? 2.7383+-0.0066 ? date-format-tofte 10.6938+-0.0603 10.6338+-0.0312 date-format-xparb 9.9379+-0.0384 ^ 9.8098+-0.0470 ^ definitely 1.0131x faster math-cordic 7.3453+-0.0422 ^ 7.2226+-0.0149 ^ definitely 1.0170x faster math-partial-sums 10.4460+-0.0150 ? 10.5484+-0.1296 ? math-spectral-norm 3.2341+-0.0039 ? 3.2414+-0.0067 ? regexp-dna 12.6540+-0.0470 12.6458+-0.0433 string-base64 5.8271+-0.0270 ? 5.8331+-0.0274 ? string-fasta 7.4589+-0.0126 ! 7.5185+-0.0135 ! definitely 1.0080x slower string-tagcloud 13.3912+-0.0431 13.3597+-0.0351 string-unpack-code 24.1221+-0.0642 ! 24.2937+-0.0690 ! definitely 1.0071x slower string-validate-input 6.7240+-0.0456 ^ 6.5689+-0.0207 ^ definitely 1.0236x faster <arithmetic> * 7.0778+-0.0083 7.0713+-0.0096 <geometric> 5.8002+-0.0055 5.7924+-0.0058 <harmonic> 4.7164+-0.0041 4.7126+-0.0035 TipOfTree ResetPrevious V8: crypto 80.2459+-0.0724 ? 80.2807+-0.0756 ? deltablue 248.0011+-0.4368 ^ 246.6678+-0.3301 ^ definitely 1.0054x faster earley-boyer 106.3520+-0.1126 ? 106.4144+-0.1251 ? raytrace 65.4147+-0.1050 ^ 64.5689+-0.1345 ^ definitely 1.0131x faster regexp 125.9085+-0.2216 ^ 124.7037+-0.2061 ^ definitely 1.0097x faster richards 221.8685+-0.5928 221.5665+-0.4124 splay 120.6715+-0.4878 ^ 119.2831+-0.4481 ^ definitely 1.0116x faster <arithmetic> 138.3518+-0.1436 ^ 137.6407+-0.0928 ^ definitely 1.0052x faster <geometric> * 124.6138+-0.1146 ^ 123.9052+-0.0758 ^ definitely 1.0057x faster <harmonic> 113.0429+-0.0972 ^ 112.3343+-0.0700 ^ definitely 1.0063x faster TipOfTree ResetPrevious Kraken: ai-astar 827.2700+-3.6717 ? 827.5272+-3.5855 ? audio-beat-detection 212.9210+-0.9801 211.8126+-0.6916 audio-dft 271.3281+-0.9805 270.6702+-1.3024 audio-fft 136.9794+-0.6045 136.6605+-0.1803 audio-oscillator 274.6997+-1.3033 274.4814+-1.2433 imaging-darkroom 491.2881+-1.3201 ! 494.8206+-1.0534 ! definitely 1.0072x slower imaging-desaturate 244.7313+-0.0561 ^ 244.6392+-0.0259 ^ definitely 1.0004x faster imaging-gaussian-blur 642.0071+-0.4442 ? 643.4533+-1.1358 ? json-parse-financial 62.9808+-0.0678 ^ 62.1493+-0.0945 ^ definitely 1.0134x faster json-stringify-tinderbox 83.9892+-0.1675 ! 84.6927+-0.1737 ! definitely 1.0084x slower stanford-crypto-aes 150.4765+-0.6981 150.3589+-0.5822 stanford-crypto-ccm 116.4817+-0.2687 116.3700+-0.2979 stanford-crypto-pbkdf2 228.1530+-0.7043 ? 228.8031+-0.8950 ? stanford-crypto-sha256-iterative 85.5514+-0.1370 85.5489+-0.1752 <arithmetic> * 273.4898+-0.3527 ? 273.7134+-0.3297 ? <geometric> 206.1863+-0.2213 206.1096+-0.1912 <harmonic> 160.3540+-0.1548 160.0788+-0.1450 TipOfTree ResetPrevious All benchmarks: <arithmetic> 105.9860+-0.1094 105.9431+-0.1045 <geometric> 26.5331+-0.0203 ^ 26.4880+-0.0194 ^ definitely 1.0017x faster <harmonic> 8.3011+-0.0071 8.2937+-0.0061 TipOfTree ResetPrevious Geomean of preferred means: <scaled-result> 62.2490+-0.0479 ^ 62.1287+-0.0419 ^ definitely 1.0019x faster
Created attachment 110062 [details] the patch
Created attachment 110068 [details] improved patch
Landed in r96883.
Phil, does this fix bug 69555? If so, perhaps you can dupe that bug to this one.
(In reply to comment #5) > Phil, does this fix bug 69555? If so, perhaps you can dupe that bug to this one. Oh yeah, I think it does. Will do!
*** Bug 69555 has been marked as a duplicate of this bug. ***
Since we know this caused assertion failures (bug 69555), it seems like it should be possible to write a regression test for this fix.
(In reply to comment #8) > Since we know this caused assertion failures (bug 69555), it seems like it should be possible to write a regression test for this fix. I would have hoped so! This is the second bug I've fixed in our structure property map code. The last time, I tried really hard to do every possible permutation of structure transitions from JS code, and could not trigger a failure. Not this one, and not the previous one, either. I haven't tried to write a test case for this fail, because of the disappointment I had the last time around: https://bugs.webkit.org/show_bug.cgi?id=69102 The problem is that normal JS code execution calls materializePropertyMap() only in a few cases, and never in cases where the state of Structure* would have already been messed up (because of this bug and the other previously fixed one), and so never sees the fail. We do however call materializePropertyMap() from the tiered compilation infrastructure. But tiered compilation kicks in when a collection of heuristics decide that it's profitable to do so. That ends up being stochastic. And then it only calls materializePropertyMap() if its profiling (which is also stochastic) tells it that it's profitable. So for sufficiently complex code, you get calls to materializePropertyMap() at random times that don't fit the pattern that would be encountered from vanilla JS code. Forcing it to happen at exactly the right time so that materializePropertyMap() encounters the failing case, by trying to hand-write a JS test case, is rough. What would be good, is to write unit tests for JSC::Structure. I'm not sure if that's feasible at this time, since the unit test would have to try pretty hard to bring up a full JS environment with global datas, objects, a GC heap, etc., and then perform evil structure transitions whilst repeatedly calling materializePropertyMap(). But maybe if we're doing that then we might as well resort to doing tests using JS fuzzers. But in my past experience, the best test case for these chaotic interactions in a VM is just the application (in this case, the website) that caused them.
Thanks for the detailed explanation! Testing this does indeed sound tricky. Hopefully we'll eventually come up with a good way to test code like this in the future. It would be nice to hint at at least some of these difficulties in your ChangeLogs so that the uninitiated like myself aren't left wondering why no test was checked in.