REGRESSION(r165407): DoYouEvenBench crashes in DRT

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Ryosuke Niwa]

After http://trac.webkit.org/changeset/165407, DYEBench has been crashing on the perf builders:
http://build.webkit.org/builders/Apple%20MountainLion%20Release%20%28Perf%29/builds/8249
http://build.webkit.org/builders/Apple%20Mavericks%20Release%20%28Perf%29/builds/843

While the blame list contains other changes, I've built r165406 and 165407 locally and only r165407 reproduces the crash quite reliably.

It appears, however, that this crash doesn't always occur. While

Tools/Scripts/run-perf-tests PerformanceTests/DoYouEvenBench/Full.html --test-runner-count=4 --reset-results

reproduces the crash quite reliably,

Tools/Scripts/run-perf-tests PerformanceTests/DoYouEvenBench/Full.html --test-runner-count=1 --reset-results

doesn't.

[Geoffrey Garen]

<rdar://problem/16285126>

[Mark Hahnenberg]

After testing with a debug build, I get a reliable ASSERT during compilation in the DFG, but no crash during GC.

[Ryosuke Niwa]

In the debug build, we're hitting the following assertion:

ASSERTION FAILED: node->op() == Phi || node->op() == SetArgument
/Volumes/Data/webkit/Source/JavaScriptCore/dfg/DFGDCEPhase.cpp(262) : void JSC::DFG::DCEPhase::cleanVariables(VariablesVectorType &) [VariablesVectorType = JSC::Operands<JSC::DFG::Node *, JSC::DFG::NodePointerTraits>]
1   0x1067dc750 WTFCrash
2   0x1062728e1 void JSC::DFG::DCEPhase::cleanVariables<JSC::Operands<JSC::DFG::Node*, JSC::DFG::NodePointerTraits> >(JSC::Operands<JSC::DFG::Node*, JSC::DFG::NodePointerTraits>&)
3   0x1062722d0 JSC::DFG::DCEPhase::fixupBlock(JSC::DFG::BasicBlock*)
4   0x106271e2e JSC::DFG::DCEPhase::run()
5   0x106271005 bool JSC::DFG::runAndLog<JSC::DFG::DCEPhase>(JSC::DFG::DCEPhase&)
6   0x106270f8e bool JSC::DFG::runPhase<JSC::DFG::DCEPhase>(JSC::DFG::Graph&)
7   0x106270f48 JSC::DFG::performDCE(JSC::DFG::Graph&)
8   0x106326f40 JSC::DFG::Plan::compileInThreadImpl(JSC::DFG::LongLivedState&)
9   0x106326674 JSC::DFG::Plan::compileInThread(JSC::DFG::LongLivedState&, JSC::DFG::ThreadData*)
10  0x1063c9ac0 JSC::DFG::Worklist::runThread(JSC::DFG::ThreadData*)
11  0x1063c8714 JSC::DFG::Worklist::threadFunction(void*)
12  0x10682bf70 WTF::threadEntryPoint(void*)
13  0x10682cbf8 WTF::wtfThreadEntryPoint(void*)
14  0x7fff8f8d4899 _pthread_body
15  0x7fff8f8d472a _pthread_struct_init
16  0x7fff8f8d8fc9 thread_start

[Mark Hahnenberg]

(In reply to comment #3)
> In the debug build, we're hitting the following assertion:
> 
> ASSERTION FAILED: node->op() == Phi || node->op() == SetArgument
> /Volumes/Data/webkit/Source/JavaScriptCore/dfg/DFGDCEPhase.cpp(262) : void JSC::DFG::DCEPhase::cleanVariables(VariablesVectorType &) [VariablesVectorType = JSC::Operands<JSC::DFG::Node *, JSC::DFG::NodePointerTraits>]
> 1   0x1067dc750 WTFCrash
> 2   0x1062728e1 void JSC::DFG::DCEPhase::cleanVariables<JSC::Operands<JSC::DFG::Node*, JSC::DFG::NodePointerTraits> >(JSC::Operands<JSC::DFG::Node*, JSC::DFG::NodePointerTraits>&)
> 3   0x1062722d0 JSC::DFG::DCEPhase::fixupBlock(JSC::DFG::BasicBlock*)
> 4   0x106271e2e JSC::DFG::DCEPhase::run()
> 5   0x106271005 bool JSC::DFG::runAndLog<JSC::DFG::DCEPhase>(JSC::DFG::DCEPhase&)
> 6   0x106270f8e bool JSC::DFG::runPhase<JSC::DFG::DCEPhase>(JSC::DFG::Graph&)
> 7   0x106270f48 JSC::DFG::performDCE(JSC::DFG::Graph&)
> 8   0x106326f40 JSC::DFG::Plan::compileInThreadImpl(JSC::DFG::LongLivedState&)
> 9   0x106326674 JSC::DFG::Plan::compileInThread(JSC::DFG::LongLivedState&, JSC::DFG::ThreadData*)
> 10  0x1063c9ac0 JSC::DFG::Worklist::runThread(JSC::DFG::ThreadData*)
> 11  0x1063c8714 JSC::DFG::Worklist::threadFunction(void*)
> 12  0x10682bf70 WTF::threadEntryPoint(void*)
> 13  0x10682cbf8 WTF::wtfThreadEntryPoint(void*)
> 14  0x7fff8f8d4899 _pthread_body
> 15  0x7fff8f8d472a _pthread_struct_init
> 16  0x7fff8f8d8fc9 thread_start

Yep, that's what I'm seeing.

[Mark Hahnenberg]

Hmm, seems like the DFG assert is just a red herring. Running the benchmark in a release build and disabling the DFG I hit the original issue.

[Mark Hahnenberg]

And disabling the baseline JIT avoids the crash entirely.

[Filip Pizlo]

I'd like to see the IR at the point of this crash.  Any chance you could convert the assertion so that if it fails, it dumps the graph (m_graph.dump()) and the node (dataLog("node = ", node, "\n"))?

(In reply to comment #4)
> (In reply to comment #3)
> > In the debug build, we're hitting the following assertion:
> > 
> > ASSERTION FAILED: node->op() == Phi || node->op() == SetArgument
> > /Volumes/Data/webkit/Source/JavaScriptCore/dfg/DFGDCEPhase.cpp(262) : void JSC::DFG::DCEPhase::cleanVariables(VariablesVectorType &) [VariablesVectorType = JSC::Operands<JSC::DFG::Node *, JSC::DFG::NodePointerTraits>]
> > 1   0x1067dc750 WTFCrash
> > 2   0x1062728e1 void JSC::DFG::DCEPhase::cleanVariables<JSC::Operands<JSC::DFG::Node*, JSC::DFG::NodePointerTraits> >(JSC::Operands<JSC::DFG::Node*, JSC::DFG::NodePointerTraits>&)
> > 3   0x1062722d0 JSC::DFG::DCEPhase::fixupBlock(JSC::DFG::BasicBlock*)
> > 4   0x106271e2e JSC::DFG::DCEPhase::run()
> > 5   0x106271005 bool JSC::DFG::runAndLog<JSC::DFG::DCEPhase>(JSC::DFG::DCEPhase&)
> > 6   0x106270f8e bool JSC::DFG::runPhase<JSC::DFG::DCEPhase>(JSC::DFG::Graph&)
> > 7   0x106270f48 JSC::DFG::performDCE(JSC::DFG::Graph&)
> > 8   0x106326f40 JSC::DFG::Plan::compileInThreadImpl(JSC::DFG::LongLivedState&)
> > 9   0x106326674 JSC::DFG::Plan::compileInThread(JSC::DFG::LongLivedState&, JSC::DFG::ThreadData*)
> > 10  0x1063c9ac0 JSC::DFG::Worklist::runThread(JSC::DFG::ThreadData*)
> > 11  0x1063c8714 JSC::DFG::Worklist::threadFunction(void*)
> > 12  0x10682bf70 WTF::threadEntryPoint(void*)
> > 13  0x10682cbf8 WTF::wtfThreadEntryPoint(void*)
> > 14  0x7fff8f8d4899 _pthread_body
> > 15  0x7fff8f8d472a _pthread_struct_init
> > 16  0x7fff8f8d8fc9 thread_start
> 
> Yep, that's what I'm seeing.

[Mark Hahnenberg]

I think I know what the issue is for the baseline JIT. The baseline JIT does a conditional store barrier for the put_by_id, but we need an unconditional store barrier so that we cover the butterfly case as well in emitPutTransitionStub.

[Mark Hahnenberg]

Created attachment 226387 [details]
Patch

[Ryosuke Niwa]

(In reply to comment #5)
> Hmm, seems like the DFG assert is just a red herring. Running the benchmark in a release build and disabling the DFG I hit the original issue.

Yeah, I hit the same assertion at r165407.

[Ryosuke Niwa]

(In reply to comment #7)
> I'd like to see the IR at the point of this crash.  Any chance you could convert the assertion so that if it fails, it dumps the graph (m_graph.dump()) and the node (dataLog("node = ", node, "\n"))?

Since this doesn't seem to be the crash culprit, I've filed https://bugs.webkit.org/show_bug.cgi?id=130069 to track the assertion failure.  I've posted the data there.

[Geoffrey Garen]

Comment on attachment 226387 [details]
Patch

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

r=me

> Source/JavaScriptCore/ChangeLog:9
> +        The baseline JIT does a conditional store barrier for the put_by_id, but we need 
> +        an unconditional store barrier so that we cover the butterfly case as well in emitPutTransitionStub.

I wonder if we should just remove conditional store barriers from the baseline JIT -- to avoid mismatches like this, and because it wasn't a regression in the optimizing JIT.

[Mark Hahnenberg]

Just to be sure, I ran benchmarks on this patch, and it's performance neutral.

[WebKit Commit Bot]

Comment on attachment 226387 [details]
Patch

Clearing flags on attachment: 226387

Committed r165435: <http://trac.webkit.org/changeset/165435>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.