Bug 69583 – Structure does not reset m_previous when pinning the property map

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Filip Pizlo]

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).

[Filip Pizlo]

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

[Filip Pizlo]

Created attachment 110062 [details]
the patch

[Filip Pizlo]

Created attachment 110068 [details]
improved patch

[Filip Pizlo]

Landed in r96883.

[Adam Roben (:aroben)]

Phil, does this fix bug 69555? If so, perhaps you can dupe that bug to this one.

[Filip Pizlo]

(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!

[Filip Pizlo]

*** Bug 69555 has been marked as a duplicate of this bug. ***

[Adam Roben (:aroben)]

Since we know this caused assertion failures (bug 69555), it seems like it should be possible to write a regression test for this fix.

[Filip Pizlo]

(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.

[Adam Roben (:aroben)]

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.