Bug 177839

Summary: bmalloc mutex should be adaptive
Product: WebKit Reporter: Filip Pizlo <fpizlo>
Component: bmallocAssignee: Filip Pizlo <fpizlo>
Status: REOPENED ---    
Severity: Normal CC: benjamin, buildbot, cdumez, cmarcelo, commit-queue, dbates, ews-watchlist, ggaren, jfbastien, jlewis3, keith_miller, mark.lam, msaboff, rmorisset, saam, webkit-bug-importer, ysuzuki
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Bug Depends on: 178818, 183542    
Bug Blocks: 177856    
Attachments:
Description Flags
the patch
msaboff: review+
patch for landing
none
patch for landing
none
patch for landing
none
crashlog
none
patch for landing none

Description Filip Pizlo 2017-10-03 14:58:29 PDT
Patch forthcoming.
Comment 1 Filip Pizlo 2017-10-03 15:01:05 PDT
@Yusuke: I was originally going to use os_unfair_lock and suggest futexes for Linux.  Unfortunately, using os_unfair_lock was a regression, but using WordLock was fine.  So, I'm about to post a patch that pulls WTF's WordLock into bmalloc.  The code is horribly duplicated, but I don't want this change to be blocked on the larger question of how to layer WTF and bmalloc.

I still think that the most performant solution for Linux is futexes.  I think we should explore making WTF::WordLock/bmalloc::Mutex use futexes on Linux.  I don't think we could do the same with WTF::Lock, since WTF::Lock has some fairness guarantees that futexes can't easily give you.
Comment 2 Filip Pizlo 2017-10-03 16:53:07 PDT
Created attachment 322615 [details]
the patch
Comment 3 Build Bot 2017-10-03 16:54:29 PDT
Attachment 322615 [details] did not pass style-queue:


ERROR: Source/bmalloc/bmalloc/BPlatform.h:218:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/bmalloc/bmalloc/StaticMutex.h:61:  try_lock is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 2 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Filip Pizlo 2017-10-03 16:54:37 PDT
Comment on attachment 322615 [details]
the patch

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

> Source/bmalloc/bmalloc/BPlatform.h:218
> +//#define BUSE_OS_UNFAIR_LOCK 1

Ooops!  I will remove.
Comment 5 Michael Saboff 2017-10-03 17:50:20 PDT
Comment on attachment 322615 [details]
the patch

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

r=me

> Source/WTF/wtf/LockAlgorithmInlines.h:36
> +// NOTE: It's a bug to us any memory order other than seq_cst in this code. The cost of seq_cst fencing

*use*

> Source/WTF/wtf/WordLock.cpp:79
> +// NOTE: It's a bug to us any memory order other than seq_cst in this code. The cost of seq_cst fencing

*use*

> Source/bmalloc/bmalloc/StaticMutex.cpp:69
> +// NOTE: It's a bug to us any memory order other than seq_cst in this code. The cost of seq_cst fencing

*use*
Comment 6 Geoffrey Garen 2017-10-03 17:56:25 PDT
Comment on attachment 322615 [details]
the patch

What are the MallocBench results for this change?

Or other benchmark results?

What's the motivation here?
Comment 7 Filip Pizlo 2017-10-04 15:01:06 PDT
(In reply to Michael Saboff from comment #5)
> Comment on attachment 322615 [details]
> the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=322615&action=review
> 
> r=me
> 
> > Source/WTF/wtf/LockAlgorithmInlines.h:36
> > +// NOTE: It's a bug to us any memory order other than seq_cst in this code. The cost of seq_cst fencing
> 
> *use*
> 
> > Source/WTF/wtf/WordLock.cpp:79
> > +// NOTE: It's a bug to us any memory order other than seq_cst in this code. The cost of seq_cst fencing
> 
> *use*
> 
> > Source/bmalloc/bmalloc/StaticMutex.cpp:69
> > +// NOTE: It's a bug to us any memory order other than seq_cst in this code. The cost of seq_cst fencing
> 
> *use*

Fixed.  Thanks!
Comment 8 Filip Pizlo 2017-10-04 15:02:59 PDT
(In reply to Geoffrey Garen from comment #6)
> Comment on attachment 322615 [details]
> the patch
> 
> What are the MallocBench results for this change?

How do I run it?

> 
> Or other benchmark results?

Neutral on in-browser JetStream.

> 
> What's the motivation here?

1) Don't waste CPU cycles if the lock is held for a while for whatever reason.
2) Reduce the amount of sched_yield activity we see when profiling.
Comment 9 Filip Pizlo 2017-10-04 15:05:56 PDT
Created attachment 322731 [details]
patch for landing
Comment 10 Build Bot 2017-10-04 15:08:52 PDT
Attachment 322731 [details] did not pass style-queue:


ERROR: Source/bmalloc/bmalloc/StaticMutex.h:61:  try_lock is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 11 Filip Pizlo 2017-10-04 15:31:52 PDT
Created attachment 322733 [details]
patch for landing
Comment 12 Build Bot 2017-10-04 15:33:22 PDT
Attachment 322733 [details] did not pass style-queue:


ERROR: Source/bmalloc/bmalloc/StaticMutex.h:61:  try_lock is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 13 Filip Pizlo 2017-10-04 18:50:37 PDT
Created attachment 322760 [details]
patch for landing

maybe it will build now?
Comment 14 Build Bot 2017-10-04 18:53:47 PDT
Attachment 322760 [details] did not pass style-queue:


ERROR: Source/bmalloc/bmalloc/StaticMutex.h:61:  try_lock is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 15 Filip Pizlo 2017-10-04 20:06:14 PDT
Landed in https://trac.webkit.org/changeset/222893/webkit
Comment 16 Radar WebKit Bug Importer 2017-10-04 20:07:47 PDT
<rdar://problem/34826960>
Comment 17 Matt Lewis 2017-10-05 10:04:07 PDT
This caused multiple API Crashes on macOS testers:

  WTF_ParkingLot.UnparkAllHundred
  WTF_ParkingLot.UnparkAllHundredFast
  WTF_ParkingLot.UnparkOneFiftyThenFiftyAll
  WTF_ParkingLot.UnparkOneFiftyThenFiftyAllFast

https://build.webkit.org/builders/Apple%20El%20Capitan%20Debug%20WK2%20%28Tests%29/builds/3443/steps/run-api-tests/logs/stdio
https://build.webkit.org/builders/Apple%20El%20Capitan%20Debug%20WK2%20%28Tests%29/builds/3443




Crash Thread:
Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   libsystem_pthread.dylib             0x00007fff94386c21 _pthread_cond_updateval + 43
1   libsystem_pthread.dylib             0x00007fff94384540 _pthread_cond_signal + 633
2   libc++.1.dylib                      0x00007fff97ebe641 std::__1::condition_variable::notify_one() + 9
3   TestWTF                             0x0000000104e81b6e bmalloc::StaticMutex::unlockSlow() + 1582 (StaticMutex.cpp:261)
4   TestWTF                             0x0000000104df1894 bmalloc::StaticMutex::unlock() + 68 (StaticMutex.h:103)
5   TestWTF                             0x0000000104e6df71 bmalloc::Deallocator::deallocateSlowCase(void*) + 321 (Deallocator.cpp:94)
6   TestWTF                             0x0000000104df1d14 bmalloc::Deallocator::deallocate(void*) + 68 (Deallocator.h:81)
7   TestWTF                             0x0000000104df1cba bmalloc::Cache::deallocate(bmalloc::HeapKind, void*) + 90 (Cache.h:104)
8   TestWTF                             0x0000000104df103b bmalloc::api::free(void*, bmalloc::HeapKind) + 27 (bmalloc.h:79)
9   TestWTF                             0x0000000104df0aa7 WTF::fastFree(void*) + 23 (FastMalloc.cpp:281)
10  TestWTF                             0x0000000104e193b5 WTF::(anonymous namespace)::ThreadData::operator delete(void*) + 21 (ParkingLot.cpp:49)
11  TestWTF                             0x0000000104e19372 WTF::ThreadSafeRefCounted<WTF::(anonymous namespace)::ThreadData>::deref() const + 82 (ThreadSafeRefCounted.h:71)
12  TestWTF                             0x0000000104e19314 void WTF::derefIfNotNull<WTF::(anonymous namespace)::ThreadData>(WTF::(anonymous namespace)::ThreadData*) + 52 (RefPtr.h:46)
13  TestWTF                             0x0000000104e192d3 WTF::RefPtr<WTF::(anonymous namespace)::ThreadData>::~RefPtr() + 83 (RefPtr.h:69)
14  TestWTF                             0x0000000104e14105 WTF::RefPtr<WTF::(anonymous namespace)::ThreadData>::~RefPtr() + 21 (RefPtr.h:69)
15  TestWTF                             0x0000000104e1a08f WTF::VectorDestructor<true, WTF::RefPtr<WTF::(anonymous namespace)::ThreadData> >::destruct(WTF::RefPtr<WTF::(anonymous namespace)::ThreadData>*, WTF::RefPtr<WTF::(anonymous namespace)::ThreadData>*) + 47 (Vector.h:59)
16  TestWTF                             0x0000000104e19ffd WTF::VectorTypeOperations<WTF::RefPtr<WTF::(anonymous namespace)::ThreadData> >::destruct(WTF::RefPtr<WTF::(anonymous namespace)::ThreadData>*, WTF::RefPtr<WTF::(anonymous namespace)::ThreadData>*) + 29 (Vector.h:225)
17  TestWTF                             0x0000000104e19fb3 WTF::Vector<WTF::RefPtr<WTF::(anonymous namespace)::ThreadData>, 8ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc>::~Vector() + 67 (Vector.h:631)
18  TestWTF                             0x0000000104e14845 WTF::Vector<WTF::RefPtr<WTF::(anonymous namespace)::ThreadData>, 8ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc>::~Vector() + 21 (Vector.h:634)
19  TestWTF                             0x0000000104e145c4 WTF::ParkingLot::unparkCount(void const*, unsigned int) + 420 (ParkingLot.cpp:789)
20  TestWTF                             0x0000000104e1486a WTF::ParkingLot::unparkAll(void const*) + 26 (ParkingLot.cpp:794)
21  TestWTF                             0x0000000104c702bf TestWebKitAPI::(anonymous namespace)::SingleLatchTest::finish(unsigned int) + 303 (ParkingLot.cpp:112)
22  TestWTF                             0x0000000104c6f9e2 TestWebKitAPI::(anonymous namespace)::runParkingTest(unsigned int, unsigned int, unsigned int, unsigned int) + 1122 (ParkingLot.cpp:199)
23  TestWTF                             0x0000000104c6f312 TestWebKitAPI::(anonymous namespace)::repeatParkingTest(unsigned int, unsigned int, unsigned int, unsigned int, unsigned int) + 66 (ParkingLot.cpp:205)
24  TestWTF                             0x0000000104c6f34a TestWebKitAPI::WTF_ParkingLot_UnparkAllHundredFast_Test::TestBody() + 42 (ParkingLot.cpp:219)
25  TestWTF                             0x0000000104e8c48a testing::Test::Run() + 154 (gtest.cc:2101)
26  TestWTF                             0x0000000104e8cd6d testing::internal::TestInfoImpl::Run() + 189 (gtest.cc:2318)
27  TestWTF                             0x0000000104e8d90d testing::TestCase::Run() + 205 (gtest.cc:2419)
28  TestWTF                             0x0000000104e93892 testing::internal::UnitTestImpl::RunAllTests() + 898 (gtest.cc:4023)
29  TestWTF                             0x0000000104e93509 testing::UnitTest::Run() + 25 (gtest.cc:3687)
30  TestWTF                             0x0000000104d4aece TestWebKitAPI::TestsController::run(int, char**) + 174 (TestsController.cpp:84)
31  TestWTF                             0x0000000104e82e92 main + 450 (mainMac.mm:52)
32  libdyld.dylib                       0x00007fff957745ad start + 1
Comment 18 Filip Pizlo 2017-10-05 10:36:15 PDT
(In reply to Matt Lewis from comment #17)
> This caused multiple API Crashes on macOS testers:
> 
>   WTF_ParkingLot.UnparkAllHundred
>   WTF_ParkingLot.UnparkAllHundredFast
>   WTF_ParkingLot.UnparkOneFiftyThenFiftyAll
>   WTF_ParkingLot.UnparkOneFiftyThenFiftyAllFast
> 
> https://build.webkit.org/builders/
> Apple%20El%20Capitan%20Debug%20WK2%20%28Tests%29/builds/3443/steps/run-api-
> tests/logs/stdio
> https://build.webkit.org/builders/
> Apple%20El%20Capitan%20Debug%20WK2%20%28Tests%29/builds/3443
> 
> 
> 
> 
> Crash Thread:
> Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
> 0   libsystem_pthread.dylib             0x00007fff94386c21
> _pthread_cond_updateval + 43
> 1   libsystem_pthread.dylib             0x00007fff94384540
> _pthread_cond_signal + 633
> 2   libc++.1.dylib                      0x00007fff97ebe641
> std::__1::condition_variable::notify_one() + 9
> 3   TestWTF                             0x0000000104e81b6e
> bmalloc::StaticMutex::unlockSlow() + 1582 (StaticMutex.cpp:261)
> 4   TestWTF                             0x0000000104df1894
> bmalloc::StaticMutex::unlock() + 68 (StaticMutex.h:103)
> 5   TestWTF                             0x0000000104e6df71
> bmalloc::Deallocator::deallocateSlowCase(void*) + 321 (Deallocator.cpp:94)
> 6   TestWTF                             0x0000000104df1d14
> bmalloc::Deallocator::deallocate(void*) + 68 (Deallocator.h:81)
> 7   TestWTF                             0x0000000104df1cba
> bmalloc::Cache::deallocate(bmalloc::HeapKind, void*) + 90 (Cache.h:104)
> 8   TestWTF                             0x0000000104df103b
> bmalloc::api::free(void*, bmalloc::HeapKind) + 27 (bmalloc.h:79)
> 9   TestWTF                             0x0000000104df0aa7
> WTF::fastFree(void*) + 23 (FastMalloc.cpp:281)
> 10  TestWTF                             0x0000000104e193b5 WTF::(anonymous
> namespace)::ThreadData::operator delete(void*) + 21 (ParkingLot.cpp:49)
> 11  TestWTF                             0x0000000104e19372
> WTF::ThreadSafeRefCounted<WTF::(anonymous namespace)::ThreadData>::deref()
> const + 82 (ThreadSafeRefCounted.h:71)
> 12  TestWTF                             0x0000000104e19314 void
> WTF::derefIfNotNull<WTF::(anonymous namespace)::ThreadData>(WTF::(anonymous
> namespace)::ThreadData*) + 52 (RefPtr.h:46)
> 13  TestWTF                             0x0000000104e192d3
> WTF::RefPtr<WTF::(anonymous namespace)::ThreadData>::~RefPtr() + 83
> (RefPtr.h:69)
> 14  TestWTF                             0x0000000104e14105
> WTF::RefPtr<WTF::(anonymous namespace)::ThreadData>::~RefPtr() + 21
> (RefPtr.h:69)
> 15  TestWTF                             0x0000000104e1a08f
> WTF::VectorDestructor<true, WTF::RefPtr<WTF::(anonymous
> namespace)::ThreadData> >::destruct(WTF::RefPtr<WTF::(anonymous
> namespace)::ThreadData>*, WTF::RefPtr<WTF::(anonymous
> namespace)::ThreadData>*) + 47 (Vector.h:59)
> 16  TestWTF                             0x0000000104e19ffd
> WTF::VectorTypeOperations<WTF::RefPtr<WTF::(anonymous
> namespace)::ThreadData> >::destruct(WTF::RefPtr<WTF::(anonymous
> namespace)::ThreadData>*, WTF::RefPtr<WTF::(anonymous
> namespace)::ThreadData>*) + 29 (Vector.h:225)
> 17  TestWTF                             0x0000000104e19fb3
> WTF::Vector<WTF::RefPtr<WTF::(anonymous namespace)::ThreadData>, 8ul,
> WTF::CrashOnOverflow, 16ul, WTF::FastMalloc>::~Vector() + 67 (Vector.h:631)
> 18  TestWTF                             0x0000000104e14845
> WTF::Vector<WTF::RefPtr<WTF::(anonymous namespace)::ThreadData>, 8ul,
> WTF::CrashOnOverflow, 16ul, WTF::FastMalloc>::~Vector() + 21 (Vector.h:634)
> 19  TestWTF                             0x0000000104e145c4
> WTF::ParkingLot::unparkCount(void const*, unsigned int) + 420
> (ParkingLot.cpp:789)
> 20  TestWTF                             0x0000000104e1486a
> WTF::ParkingLot::unparkAll(void const*) + 26 (ParkingLot.cpp:794)
> 21  TestWTF                             0x0000000104c702bf
> TestWebKitAPI::(anonymous namespace)::SingleLatchTest::finish(unsigned int)
> + 303 (ParkingLot.cpp:112)
> 22  TestWTF                             0x0000000104c6f9e2
> TestWebKitAPI::(anonymous namespace)::runParkingTest(unsigned int, unsigned
> int, unsigned int, unsigned int) + 1122 (ParkingLot.cpp:199)
> 23  TestWTF                             0x0000000104c6f312
> TestWebKitAPI::(anonymous namespace)::repeatParkingTest(unsigned int,
> unsigned int, unsigned int, unsigned int, unsigned int) + 66
> (ParkingLot.cpp:205)
> 24  TestWTF                             0x0000000104c6f34a
> TestWebKitAPI::WTF_ParkingLot_UnparkAllHundredFast_Test::TestBody() + 42
> (ParkingLot.cpp:219)
> 25  TestWTF                             0x0000000104e8c48a
> testing::Test::Run() + 154 (gtest.cc:2101)
> 26  TestWTF                             0x0000000104e8cd6d
> testing::internal::TestInfoImpl::Run() + 189 (gtest.cc:2318)
> 27  TestWTF                             0x0000000104e8d90d
> testing::TestCase::Run() + 205 (gtest.cc:2419)
> 28  TestWTF                             0x0000000104e93892
> testing::internal::UnitTestImpl::RunAllTests() + 898 (gtest.cc:4023)
> 29  TestWTF                             0x0000000104e93509
> testing::UnitTest::Run() + 25 (gtest.cc:3687)
> 30  TestWTF                             0x0000000104d4aece
> TestWebKitAPI::TestsController::run(int, char**) + 174
> (TestsController.cpp:84)
> 31  TestWTF                             0x0000000104e82e92 main + 450
> (mainMac.mm:52)
> 32  libdyld.dylib                       0x00007fff957745ad start + 1

I know what’s wrong. I can fix it in a couple hours. I’m ok with this being rolled out.
Comment 19 Matt Lewis 2017-10-05 10:58:59 PDT
Reverted r222893 for reason:

This caused multiple API failures.

Committed r222919: <http://trac.webkit.org/changeset/222919>
Comment 20 Filip Pizlo 2017-10-05 16:49:24 PDT
(In reply to Matt Lewis from comment #19)
> Reverted r222893 for reason:
> 
> This caused multiple API failures.
> 
> Committed r222919: <http://trac.webkit.org/changeset/222919>

Thanks for fixing that.  The problem was an ancient WordLock bug.  We never fixed it because WordLock is used so sparingly in WTF.  The fix was just to avoid doing the notify_one-after-unlock optimization in unlockSlow.
Comment 21 Filip Pizlo 2017-10-05 16:49:52 PDT
Relanded in https://trac.webkit.org/changeset/222945/webkit
Comment 22 WebKit Commit Bot 2017-10-25 11:30:17 PDT
Re-opened since this is blocked by bug 178818
Comment 23 Saam Barati 2017-10-25 12:32:07 PDT
Created attachment 324866 [details]
crashlog

This made WasmBench crash. Attaching crashlog.
Comment 24 Filip Pizlo 2017-10-25 13:13:26 PDT
(In reply to Saam Barati from comment #23)
> Created attachment 324866 [details]
> crashlog
> 
> This made WasmBench crash. Attaching crashlog.

Wow. That’s a crazy crash!

I wonder why the crash log has no crash address. Am I just not reading it right?
Comment 25 Saam Barati 2017-10-25 13:18:35 PDT
(In reply to Filip Pizlo from comment #24)
> (In reply to Saam Barati from comment #23)
> > Created attachment 324866 [details]
> > crashlog
> > 
> > This made WasmBench crash. Attaching crashlog.
> 
> Wow. That’s a crazy crash!
> 
> I wonder why the crash log has no crash address. Am I just not reading it
> right?

Nope, I was wondering the same thing about it. Let me see if I can grab a different crashtrace
Comment 26 Saam Barati 2017-10-25 14:02:06 PDT
(In reply to Saam Barati from comment #25)
> (In reply to Filip Pizlo from comment #24)
> > (In reply to Saam Barati from comment #23)
> > > Created attachment 324866 [details]
> > > crashlog
> > > 
> > > This made WasmBench crash. Attaching crashlog.
> > 
> > Wow. That’s a crazy crash!
> > 
> > I wonder why the crash log has no crash address. Am I just not reading it
> > right?
> 
> Nope, I was wondering the same thing about it. Let me see if I can grab a
> different crashtrace

All the crash traces I have show SEGV with no address. Very weird indeed.
Comment 27 Filip Pizlo 2018-03-08 11:55:29 PST
(In reply to Saam Barati from comment #26)
> (In reply to Saam Barati from comment #25)
> > (In reply to Filip Pizlo from comment #24)
> > > (In reply to Saam Barati from comment #23)
> > > > Created attachment 324866 [details]
> > > > crashlog
> > > > 
> > > > This made WasmBench crash. Attaching crashlog.
> > > 
> > > Wow. That’s a crazy crash!
> > > 
> > > I wonder why the crash log has no crash address. Am I just not reading it
> > > right?
> > 
> > Nope, I was wondering the same thing about it. Let me see if I can grab a
> > different crashtrace
> 
> All the crash traces I have show SEGV with no address. Very weird indeed.

I just ran on my own machine.  No crashes!
Comment 28 Filip Pizlo 2018-03-08 13:33:30 PST
I'm going to reland this based on the fact that I cannot repro any WasmBench crash.  I've tried both with and without HAVE_PTHREAD_MACHDEP_H.
Comment 29 Filip Pizlo 2018-03-08 13:36:01 PST
Created attachment 335336 [details]
patch for landing
Comment 30 EWS Watchlist 2018-03-08 13:39:32 PST
Attachment 335336 [details] did not pass style-queue:


ERROR: Source/bmalloc/bmalloc/StaticMutex.h:61:  try_lock is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 31 Filip Pizlo 2018-03-08 14:58:48 PST
Landed in https://trac.webkit.org/changeset/229436/webkit
Comment 32 WebKit Commit Bot 2018-03-10 09:21:08 PST
Re-opened since this is blocked by bug 183542
Comment 33 Filip Pizlo 2018-03-10 09:30:24 PST
What seems to be happening is that there is both:

- particularly bad bmalloc lock contention when running parallel compiler threads, and
- the WordLock algorithm is significantly worse than a spinlock at handling high contention.

Alternatively, it may mean that B3 compilation calls bmalloc so much that the uncontended lock+unlock speed difference between an adaptive lock and a spinlock matters.