Bug 142674

Summary: Replace TCSpinLock with a new WTF::SpinLock based on WTF::Atomic
Product: WebKit Reporter: Mark Lam <mark.lam>
Component: Web Template FrameworkAssignee: Mark Lam <mark.lam>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, cdumez, fpizlo, ggaren, mhahnenb, mmirman, msaboff
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
the patch.
none
patch 2: added some ChangeLog comment. fpizlo: review+

Description Mark Lam 2015-03-13 10:54:38 PDT
We no longer use TCMalloc in our code, and we now have C++11.  This replaces the TCMalloc_SpinLock with a new WTF::SpinLock based on WTF::Atomic (which is a wrapper around std::atomic).
Comment 1 Mark Lam 2015-03-13 11:25:54 PDT
Created attachment 248592 [details]
the patch.
Comment 2 Mark Lam 2015-03-13 11:38:07 PDT
Created attachment 248593 [details]
patch 2: added some ChangeLog comment.
Comment 3 Filip Pizlo 2015-03-13 12:14:45 PDT
Comment on attachment 248593 [details]
patch 2: added some ChangeLog comment.

Can you also change this SpinLock to using methods named according to our conventions (i.e. lock() instead of Lock())?  Then you could get rid of SpinLockHolder and use the existing WTF holder.
Comment 4 Mark Lam 2015-03-13 12:48:09 PDT
(In reply to comment #3)
> Comment on attachment 248593 [details]
> patch 2: added some ChangeLog comment.
> 
> Can you also change this SpinLock to using methods named according to our
> conventions (i.e. lock() instead of Lock())?  Then you could get rid of
> SpinLockHolder and use the existing WTF holder.

I did.  Look at SpinLock.h.  I think you may be looking at the deleted TCSpinLock.h.  SpinLockHolder is now implemented on top of WTF::Locker just like we do with MutexLocker.  I opted not to rename it to SpinLockLocker because this reduces unnecessary diffs.
Comment 5 Mark Lam 2015-03-13 13:06:30 PDT
Thanks for the review.  Landed in r181485: <http://trac.webkit.org/r181485>.
Comment 6 Chris Dumez 2015-03-14 10:58:12 PDT
This may have caused Speedometer to start crashing:
Running Speedometer/Full.html (144 of 144)
error: Speedometer/Full.html
1   0x113d6747e JSC::DFG::(anonymous namespace)::PutStackSinkingPhase::run()
2   0x113d5ded4 JSC::DFG::performPutStackSinking(JSC::DFG::Graph&)
3   0x113db9d99 JSC::DFG::Plan::compileInThreadImpl(JSC::DFG::LongLivedState&)
4   0x113db96dd JSC::DFG::Plan::compileInThread(JSC::DFG::LongLivedState&, JSC::DFG::ThreadData*)
5   0x113e45042 JSC::DFG::Worklist::runThread(JSC::DFG::ThreadData*)
6   0x1141cf833 WTF::threadEntryPoint(void*)
7   0x1141cfd1f WTF::wtfThreadEntryPoint(void*)
8   0x7fff8e583268 _pthread_body
9   0x7fff8e5831e5 _pthread_body
10  0x7fff8e58141d thread_start

It is consistently crashing on all perf bots:
https://build.webkit.org/builders/Apple%20Yosemite%20Release%20WK2%20%28Perf%29/builds/1263/steps/perf-test/logs/stdio

There are a few other commits in the range but this one seems the most likely cause.
Comment 7 Chris Dumez 2015-03-14 11:02:14 PDT
(In reply to comment #6)
> This may have caused Speedometer to start crashing:
> Running Speedometer/Full.html (144 of 144)
> error: Speedometer/Full.html
> 1   0x113d6747e JSC::DFG::(anonymous namespace)::PutStackSinkingPhase::run()
> 2   0x113d5ded4 JSC::DFG::performPutStackSinking(JSC::DFG::Graph&)
> 3   0x113db9d99
> JSC::DFG::Plan::compileInThreadImpl(JSC::DFG::LongLivedState&)
> 4   0x113db96dd JSC::DFG::Plan::compileInThread(JSC::DFG::LongLivedState&,
> JSC::DFG::ThreadData*)
> 5   0x113e45042 JSC::DFG::Worklist::runThread(JSC::DFG::ThreadData*)
> 6   0x1141cf833 WTF::threadEntryPoint(void*)
> 7   0x1141cfd1f WTF::wtfThreadEntryPoint(void*)
> 8   0x7fff8e583268 _pthread_body
> 9   0x7fff8e5831e5 _pthread_body
> 10  0x7fff8e58141d thread_start
> 
> It is consistently crashing on all perf bots:
> https://build.webkit.org/builders/
> Apple%20Yosemite%20Release%20WK2%20%28Perf%29/builds/1263/steps/perf-test/
> logs/stdio
> 
> There are a few other commits in the range but this one seems the most
> likely cause.

Filed https://bugs.webkit.org/show_bug.cgi?id=142697