Bug 76768 – DFG should not have code that directly decodes the states of old JIT inline cache data structures

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

[Filip Pizlo]

Doing so means that the DFG is far too tied to the old JIT being the one that does the profiling.

[Filip Pizlo]

<rdar://problem/10734723>

[Filip Pizlo]

Created attachment 123431 [details]
the patch

[WebKit Review Bot]

Attachment 123431 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/CMakeLists.txt', u'S..." exit_code: 1

Source/JavaScriptCore/bytecode/GetByIdStatus.h:40:  One space before end of line comments  [whitespace/comments] [5]
Source/JavaScriptCore/bytecode/GetByIdStatus.h:41:  One space before end of line comments  [whitespace/comments] [5]
Source/JavaScriptCore/bytecode/GetByIdStatus.h:42:  One space before end of line comments  [whitespace/comments] [5]
Source/JavaScriptCore/bytecode/GetByIdStatus.h:43:  One space before end of line comments  [whitespace/comments] [5]
Total errors found: 4 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Sam Weinig]

Comment on attachment 123431 [details]
the patch

View in context: https://bugs.webkit.org/attachment.cgi?id=123431&action=review

> Source/JavaScriptCore/bytecode/PutByIdStatus.h:44
> +        NoInformation,
> +        SimpleReplace,
> +        SimpleTransition,
> +        TakesSlowPath

Feed me comments like my brother!

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:-1879
> -            if (stubInfo.seen
> -                && !m_inlineStackTop->m_profiledBlock->likelyToTakeSlowCase(m_currentIndex)
> -                && !m_inlineStackTop->m_exitProfile.hasExitSite(m_currentIndex, BadCache)) {

You said this is a bugish thingy.

[Filip Pizlo]

(In reply to comment #4)
> (From update of attachment 123431 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=123431&action=review
> 
> > Source/JavaScriptCore/bytecode/PutByIdStatus.h:44
> > +        NoInformation,
> > +        SimpleReplace,
> > +        SimpleTransition,
> > +        TakesSlowPath
> 
> Feed me comments like my brother!
> 
> > Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:-1879
> > -            if (stubInfo.seen
> > -                && !m_inlineStackTop->m_profiledBlock->likelyToTakeSlowCase(m_currentIndex)
> > -                && !m_inlineStackTop->m_exitProfile.hasExitSite(m_currentIndex, BadCache)) {
> 
> You said this is a bugish thingy.

To elaborate: the old code was checking hasExitSite - a DFG-specific query that tells if the DFG's attempt to optimize this code failed last time.  The new code wasn't doing this, which was buggish and also a thingy.

[Filip Pizlo]

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

[Csaba Osztrogonác]

(In reply to comment #6)
> Landed in http://trac.webkit.org/changeset/105581

It broke Qt ARM build:
../../../../Source/JavaScriptCore/bytecode/CallLinkStatus.cpp: In static member function 'static JSC::CallLinkStatus JSC::CallLinkStatus::computeFor(JSC::CodeBlock*, unsigned int)':
../../../../Source/JavaScriptCore/bytecode/CallLinkStatus.cpp:38: error: 'class JSC::CodeBlock' has no member named 'couldTakeSlowCase'

And Qt Windows build:
../../../../Source/JavaScriptCore/bytecode/GetByIdStatus.cpp: In static member function 'static JSC::GetByIdStatus JSC::GetByIdStatus::computeFor(JSC::CodeBlock*, unsigned int, JSC::Identifier&)':
../../../../Source/JavaScriptCore/bytecode/GetByIdStatus.cpp:64:24: error: 'class JSC::CodeBlock' has no member named 'likelyToTakeSlowCase'
../../../../Source/JavaScriptCore/bytecode/PutByIdStatus.cpp: In static member function 'static JSC::PutByIdStatus JSC::PutByIdStatus::computeFor(JSC::CodeBlock*, unsigned int, JSC::Identifier&)':
../../../../Source/JavaScriptCore/bytecode/PutByIdStatus.cpp:38:24: error: 'class JSC::CodeBlock' has no member named 'likelyToTakeSlowCase'
../../../../Source/JavaScriptCore/bytecode/CallLinkStatus.cpp: In static member function 'static JSC::CallLinkStatus JSC::CallLinkStatus::computeFor(JSC::CodeBlock*, unsigned int)':
../../../../Source/JavaScriptCore/bytecode/CallLinkStatus.cpp:38:24: error: 'class JSC::CodeBlock' has no member named 'couldTakeSlowCase'

and Qt SL, MIPS and SH4 too ...

[Csaba Osztrogonác]

Could you guys respect the contributing rules, please? 

http://www.webkit.org/coding/contributing.html
"Keeping the tree green
...
There may be regressions from your change or additional feedback from reviewers after the patch has landed. You can watch the tree at build.webkit.org to make sure your patch builds and passes tests on all platforms.
...
Your change must at least compile on all platforms."

[Csaba Osztrogonác]

Fixed by http://trac.webkit.org/changeset/105586