Bug 147891 – DFG::ByteCodeParser shouldn't call tryGetConstantProperty() with some StructureSet if it isn't checking that the base has a structure in that StructureSet

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

[Filip Pizlo]

Patch forthcoming.

[Filip Pizlo]

Created attachment 258735 [details]
the patch

[Mark Lam]

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.

[Filip Pizlo]

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

[Filip Pizlo]

Landed in http://trac.webkit.org/changeset/188292

[Ryosuke Niwa]

This appears to have caused 4% regression on Octane and 2% regression on JetStream.

[Filip Pizlo]

(In reply to comment #5)
> This appears to have caused 4% regression on Octane and 2% regression on
> JetStream.

Looking into it.

[Filip Pizlo]

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

[Filip Pizlo]

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

[Filip Pizlo]

(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