Bug 182474

Summary: Multiple bmalloc scavenger threads is unexpected
Product: WebKit Reporter: Joseph Pecoraro <joepeck>
Component: bmallocAssignee: Joseph Pecoraro <joepeck>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, commit-queue, fpizlo, ggaren, joepeck, rniwa, saam, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
[PATCH] Proposed Fix none

Description Joseph Pecoraro 2018-02-04 23:22:10 PST
Multiple bmalloc scavenger threads is unexpected

Steps to reproduce:
1. Run with trunk
2. Load a webpage
3. `sample <pid>` the com.apple.WebKit.WebContent process
  => Multiple bmalloc Scavenger threads

Sample:
...
    830 Thread_242317
    + 830 thread_start  (in libsystem_pthread.dylib) + 13  [0x7fff636c3bf9]
    +   830 _pthread_start  (in libsystem_pthread.dylib) + 377  [0x7fff636c450d]
    +     830 _pthread_body  (in libsystem_pthread.dylib) + 340  [0x7fff636c4661]
    +       830 void* std::__1::__thread_proxy<std::__1::tuple<std::__1::unique_ptr<std::__1::__thread_struct, std::__1::default_delete<std::__1::__thread_struct> >, void (*)(bmalloc::Scavenger*), bmalloc::Scavenger*> >(void*)  (in JavaScriptCore) + 40  [0x7fff3e941b48]
    +         830 bmalloc::Scavenger::threadEntryPoint(bmalloc::Scavenger*)  (in JavaScriptCore) + 9  [0x7fff3e941809]
    +           830 bmalloc::Scavenger::threadRunLoop()  (in JavaScriptCore) + 251  [0x7fff3e941a0b]
    +             830 void std::__1::condition_variable_any::wait<std::__1::unique_lock<bmalloc::Mutex> >(std::__1::unique_lock<bmalloc::Mutex>&)  (in JavaScriptCore) + 86  [0x7fff3e941d56]
    +               830 std::__1::condition_variable::wait(std::__1::unique_lock<std::__1::mutex>&)  (in libc++.1.dylib) + 18  [0x7fff61317cb0]
    +                 830 _pthread_cond_wait  (in libsystem_pthread.dylib) + 732  [0x7fff636c5589]
    +                   830 __psynch_cvwait  (in libsystem_kernel.dylib) + 10  [0x7fff63508a1e]
...
    830 Thread_244724
    + 830 thread_start  (in libsystem_pthread.dylib) + 13  [0x7fff636c3bf9]
    +   830 _pthread_start  (in libsystem_pthread.dylib) + 377  [0x7fff636c450d]
    +     830 _pthread_body  (in libsystem_pthread.dylib) + 340  [0x7fff636c4661]
    +       830 void* std::__1::__thread_proxy<std::__1::tuple<std::__1::unique_ptr<std::__1::__thread_struct, std::__1::default_delete<std::__1::__thread_struct> >, void (*)(bmalloc::Scavenger*), bmalloc::Scavenger*> >(void*)  (in JavaScriptCore) + 40  [0x7fff3e941b48]
    +         830 bmalloc::Scavenger::threadEntryPoint(bmalloc::Scavenger*)  (in JavaScriptCore) + 9  [0x7fff3e941809]
    +           830 bmalloc::Scavenger::threadRunLoop()  (in JavaScriptCore) + 251  [0x7fff3e941a0b]
    +             830 void std::__1::condition_variable_any::wait<std::__1::unique_lock<bmalloc::Mutex> >(std::__1::unique_lock<bmalloc::Mutex>&)  (in JavaScriptCore) + 86  [0x7fff3e941d56]
    +               830 std::__1::condition_variable::wait(std::__1::unique_lock<std::__1::mutex>&)  (in libc++.1.dylib) + 18  [0x7fff61317cb0]
    +                 830 _pthread_cond_wait  (in libsystem_pthread.dylib) + 732  [0x7fff636c5589]
    +                   830 __psynch_cvwait  (in libsystem_kernel.dylib) + 10  [0x7fff63508a1e]

Notes:

- One is created via JavaScriptCore with PerProcess<Scavenger> for normal allocations

(lldb) bt
* thread #1, queue = 'com.apple.main-thread', stop reason = breakpoint 1.1
  * frame #0: 0x00007fff3e941810 JavaScriptCore`bmalloc::Scavenger::Scavenger(std::__1::lock_guard<bmalloc::StaticMutex>&)
    frame #1: 0x00007fff3e70e198 JavaScriptCore`bmalloc::PerProcess<bmalloc::Scavenger>::getSlowCase() + 72
    frame #2: 0x00007fff3e946db4 JavaScriptCore`bmalloc::Heap::Heap(bmalloc::HeapKind, std::__1::lock_guard<bmalloc::StaticMutex>&) + 1636
    ...

- Another is created later on via WebCore with PerProcess<Scavenger> for isoheap allocations

(lldb) bt
* thread #1, queue = 'com.apple.main-thread', stop reason = breakpoint 1.1
  * frame #0: 0x00007fff3e941810 JavaScriptCore`bmalloc::Scavenger::Scavenger(std::__1::lock_guard<bmalloc::StaticMutex>&)
    frame #1: 0x00007fff4922da88 WebCore`bmalloc::PerProcess<bmalloc::Scavenger>::getSlowCase() + 72
    frame #2: 0x00007fff49374400 WebCore`bmalloc::IsoDirectory<bmalloc::IsoConfig<1184u>, 32u>::takeFirstEligible() + 80
    ...

It seems these should be the same instance, but were unexpectedly different instances.
Comment 1 Joseph Pecoraro 2018-02-04 23:22:22 PST
<rdar://problem/37175526>
Comment 2 Joseph Pecoraro 2018-02-04 23:28:38 PST
Created attachment 333062 [details]
[PATCH] Proposed Fix

I didn't convert all PerProcess<T> => SafePerProcess<T> because it requires up-front knowledge of the type specializations. This falls over for IsoTLSDeallocatorEntry, which is constructed with many different configurations:

    IsoTLSDeallocatorEntry<IsoConfig<32>>
    IsoTLSDeallocatorEntry<IsoConfig<40>>
    IsoTLSDeallocatorEntry<IsoConfig<56>>
    IsoTLSDeallocatorEntry<IsoConfig<72>>
    IsoTLSDeallocatorEntry<IsoConfig<88>>
    IsoTLSDeallocatorEntry<IsoConfig<96>>
    IsoTLSDeallocatorEntry<IsoConfig<104>>
    IsoTLSDeallocatorEntry<IsoConfig<120>>
    ...

This also presents a case where there is another PerProcess<T> cross image issue. JavaScriptCore and WebCore each produce their own `IsoTLSDeallocatorEntry<IsoConfig<40>>` instance. So we may have to abandon the PerProcess<T> templated approach here.
Comment 3 Joseph Pecoraro 2018-02-04 23:31:03 PST
> This also presents a case where there is another PerProcess<T> cross image
> issue. JavaScriptCore and WebCore each produce their own
> `IsoTLSDeallocatorEntry<IsoConfig<40>>` instance. So we may have to abandon
> the PerProcess<T> templated approach here.

Or someone can figure out how to somehow export the templated symbols. A few of us looked at this and couldn't figure out how to make it happen generically, and fell back to the duplicate class above which explicitly creates the symbols and exports them.
Comment 4 WebKit Commit Bot 2018-02-05 11:18:49 PST
Comment on attachment 333062 [details]
[PATCH] Proposed Fix

Clearing flags on attachment: 333062

Committed r228107: <https://trac.webkit.org/changeset/228107>
Comment 5 WebKit Commit Bot 2018-02-05 11:18:50 PST
All reviewed patches have been landed.  Closing bug.
Comment 6 Saam Barati 2018-02-05 11:47:51 PST
Comment on attachment 333062 [details]
[PATCH] Proposed Fix

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

> Source/bmalloc/bmalloc/PerProcess.h:41
> +// WARNING: PerProcess<T> does not export its storage. So in actuality when

Maybe we should rename PerProcess?
Comment 7 Joseph Pecoraro 2018-02-05 11:58:08 PST
(In reply to Saam Barati from comment #6)
> Comment on attachment 333062 [details]
> [PATCH] Proposed Fix
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=333062&action=review
> 
> > Source/bmalloc/bmalloc/PerProcess.h:41
> > +// WARNING: PerProcess<T> does not export its storage. So in actuality when
> 
> Maybe we should rename PerProcess?

Yeah, I think any follow-ups can/should happen with bug 182496.