Patch forthcoming.
Created attachment 258735 [details] the patch
Comment on attachment 258735 [details] the patch View in context: https://bugs.webkit.org/attachment.cgi?id=258735&action=review r=me with suggested removal of StructureRegistrationState and m_structureRegistrationState. > Source/JavaScriptCore/ChangeLog:13 > + (JSC::DFG::Graph::assertIsRegistered): Make this always assert even before the StructureRegistrationPhase. I presume we'll deal with the fall out if this actually does assert because that would be bad, and we want to catch those cases? > Source/JavaScriptCore/dfg/DFGGraph.cpp:-1285 > - if (m_structureRegistrationState == HaveNotStartedRegistering) > - return; > - Does it make sense to still keep m_structureRegistrationState, and the StructureRegistrationState enum around? From the code, it looks like this check was their sole purpose.
(In reply to comment #2) > Comment on attachment 258735 [details] > the patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=258735&action=review > > r=me with suggested removal of StructureRegistrationState and > m_structureRegistrationState. > > > Source/JavaScriptCore/ChangeLog:13 > > + (JSC::DFG::Graph::assertIsRegistered): Make this always assert even before the StructureRegistrationPhase. > > I presume we'll deal with the fall out if this actually does assert because > that would be bad, and we want to catch those cases? Yeah, that's the idea. Making this assert do things is what caught the bug in ByteCodeParser that this fixes, but I anticipate it will catch more bugs too. > > > Source/JavaScriptCore/dfg/DFGGraph.cpp:-1285 > > - if (m_structureRegistrationState == HaveNotStartedRegistering) > > - return; > > - > > Does it make sense to still keep m_structureRegistrationState, and the > StructureRegistrationState enum around? From the code, it looks like this > check was their sole purpose. We should get rid of all of this nonsense. This patch keeps it so that we have a quick gardening fix for now. For example, I could roll out just the change to assertIsRegistered if we get a lot of bot assertions due to this changeset.
Landed in http://trac.webkit.org/changeset/188292
This appears to have caused 4% regression on Octane and 2% regression on JetStream.
(In reply to comment #5) > This appears to have caused 4% regression on Octane and 2% regression on > JetStream. Looking into it.
(In reply to comment #6) > (In reply to comment #5) > > This appears to have caused 4% regression on Octane and 2% regression on > > JetStream. > > Looking into it. Curiously the SunSpider regression doesn't repro on the command line. But the Octane one certainly does. I'm going to assume that whatever is happening on Octane on command line is the same as what is going on in SunSpider in-browser.
Benchmark report for Octane on shakezilla (MacBookPro11,3). VMs tested: "TipOfTree" at /Volumes/Data/secondary/OpenSource/WebKitBuild/Release/jsc (r188348) "FixGetByOffset" at /Volumes/Data/primary/OpenSource/WebKitBuild/Release/jsc (r188348) Collected 6 samples per benchmark/VM, with 6 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 FixGetByOffset encrypt 0.20010+-0.00681 ? 0.20493+-0.00225 ? might be 1.0242x slower decrypt 3.29443+-0.01743 ? 3.31550+-0.02264 ? deltablue x2 0.15490+-0.00419 0.15388+-0.00284 earley 0.27660+-0.00404 0.27627+-0.00071 boyer 4.13365+-0.03254 4.13169+-0.02740 navier-stokes x2 4.97103+-0.04141 4.92903+-0.02771 raytrace x2 1.02157+-0.06426 ! 1.87176+-0.06099 ! definitely 1.8322x slower richards x2 0.10082+-0.00120 ? 0.10092+-0.00144 ? splay x2 0.33858+-0.00391 ? 0.34019+-0.00946 ? regexp x2 24.95480+-0.30264 24.64851+-0.31738 might be 1.0124x faster pdfjs x2 37.27119+-0.25281 ? 37.48371+-0.30576 ? mandreel x2 44.23657+-0.44643 ? 44.41746+-0.73037 ? gbemu x2 34.26305+-0.57741 ? 35.12642+-2.88043 ? might be 1.0252x slower closure 0.56191+-0.00496 0.55811+-0.00253 jquery 7.11414+-0.10498 7.06744+-0.05498 box2d x2 10.15449+-0.09647 10.01343+-0.15831 might be 1.0141x faster zlib x2 372.81171+-20.02247 ? 386.93382+-10.61960 ? might be 1.0379x slower typescript x2 667.24166+-15.58583 657.88420+-14.08641 might be 1.0142x faster <geometric> 5.59631+-0.04191 ! 5.83831+-0.04758 ! definitely 1.0432x slower
(In reply to comment #8) > Benchmark report for Octane on shakezilla (MacBookPro11,3). > > VMs tested: > "TipOfTree" at /Volumes/Data/secondary/OpenSource/WebKitBuild/Release/jsc > (r188348) > "FixGetByOffset" at /Volumes/Data/primary/OpenSource/WebKitBuild/Release/jsc > (r188348) > > Collected 6 samples per benchmark/VM, with 6 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 FixGetByOffset > > > encrypt 0.20010+-0.00681 ? 0.20493+-0.00225 ? > might be 1.0242x slower > decrypt 3.29443+-0.01743 ? 3.31550+-0.02264 ? > deltablue x2 0.15490+-0.00419 0.15388+-0.00284 > earley 0.27660+-0.00404 0.27627+-0.00071 > boyer 4.13365+-0.03254 4.13169+-0.02740 > navier-stokes x2 4.97103+-0.04141 4.92903+-0.02771 > raytrace x2 1.02157+-0.06426 ! 1.87176+-0.06099 ! > definitely 1.8322x slower > richards x2 0.10082+-0.00120 ? 0.10092+-0.00144 ? > splay x2 0.33858+-0.00391 ? 0.34019+-0.00946 ? > regexp x2 24.95480+-0.30264 24.64851+-0.31738 > might be 1.0124x faster > pdfjs x2 37.27119+-0.25281 ? 37.48371+-0.30576 ? > mandreel x2 44.23657+-0.44643 ? 44.41746+-0.73037 ? > gbemu x2 34.26305+-0.57741 ? 35.12642+-2.88043 ? > might be 1.0252x slower > closure 0.56191+-0.00496 0.55811+-0.00253 > jquery 7.11414+-0.10498 7.06744+-0.05498 > box2d x2 10.15449+-0.09647 10.01343+-0.15831 > might be 1.0141x faster > zlib x2 372.81171+-20.02247 ? 386.93382+-10.61960 ? > might be 1.0379x slower > typescript x2 667.24166+-15.58583 657.88420+-14.08641 > might be 1.0142x faster > > <geometric> 5.59631+-0.04191 ! 5.83831+-0.04758 ! > definitely 1.0432x slower Fixed in http://trac.webkit.org/changeset/188357