Bug 182474 - Multiple bmalloc scavenger threads is unexpected
Summary: Multiple bmalloc scavenger threads is unexpected
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: bmalloc (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Joseph Pecoraro
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-02-04 23:22 PST by Joseph Pecoraro
Modified: 2018-02-05 11:58 PST (History)
8 users (show)

See Also:


Attachments
[PATCH] Proposed Fix (9.43 KB, patch)
2018-02-04 23:28 PST, Joseph Pecoraro
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.