Bug 142988 - [Darwin] Boost the web process QoS level while handling a synchronous IPC message.
Summary: [Darwin] Boost the web process QoS level while handling a synchronous IPC mes...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Andreas Kling
URL:
Keywords: InRadar, Performance
Depends on:
Blocks:
 
Reported: 2015-03-23 15:08 PDT by Andreas Kling
Modified: 2015-03-26 15:21 PDT (History)
3 users (show)

See Also:


Attachments
Patch (1.92 KB, patch)
2015-03-23 15:12 PDT, Andreas Kling
barraclough: review+
Details | Formatted Diff | Diff
Patch (6.35 KB, patch)
2015-03-25 16:33 PDT, Andreas Kling
koivisto: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andreas Kling 2015-03-23 15:08:19 PDT
<rdar://problem/20264346>

If the UIProcess is blocking on a synchronous IPC request to the WebProcess, we should boost the WebProcess's main thread for the duration, to ensure that the WebProcess gets appropriately scheduled by the kernel.
Comment 1 Andreas Kling 2015-03-23 15:12:06 PDT
Created attachment 249282 [details]
Patch
Comment 2 Andreas Kling 2015-03-25 16:33:17 PDT
Created attachment 249443 [details]
Patch

Better patch that doesn't wait for the main thread to complete whatever it's doing before setting the boost.
Comment 3 Antti Koivisto 2015-03-26 10:42:34 PDT
Comment on attachment 249443 [details]
Patch

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

> Source/WebKit2/Platform/IPC/Connection.cpp:262
> +    ASSERT(pthread_main_np());
> +    m_mainThread = pthread_self();

Don't we have any existing way to access the main pthread?

> Source/WebKit2/Platform/IPC/Connection.h:312
> +    pthread_t m_mainThread { 0 };

I think pthread_t is a pointer so nullptr (or just { }) would be more appropriate.

> Source/WebKit2/Platform/IPC/MessageDecoder.cpp:46
>  MessageDecoder::~MessageDecoder()
>  {
> +#if HAVE(QOS_CLASSES)
> +    if (m_qosClassOverride)
> +        pthread_override_qos_class_end_np(m_qosClassOverride);
> +#endif
>  }

It seems weird that this is part of the MessageDecoder destructor. I feel this type should be about decoding messages, not juggling thread priorities.
Comment 4 Andreas Kling 2015-03-26 12:19:30 PDT
(In reply to comment #3)
> Comment on attachment 249443 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=249443&action=review
> 
> > Source/WebKit2/Platform/IPC/Connection.cpp:262
> > +    ASSERT(pthread_main_np());
> > +    m_mainThread = pthread_self();
> 
> Don't we have any existing way to access the main pthread?

Sort-of. It will lie and give you the web thread on iOS, and I didn't wanna start pulling on that thread. Pun totally intended.

> > Source/WebKit2/Platform/IPC/Connection.h:312
> > +    pthread_t m_mainThread { 0 };
> 
> I think pthread_t is a pointer so nullptr (or just { }) would be more
> appropriate.

k

> > Source/WebKit2/Platform/IPC/MessageDecoder.cpp:46
> >  MessageDecoder::~MessageDecoder()
> >  {
> > +#if HAVE(QOS_CLASSES)
> > +    if (m_qosClassOverride)
> > +        pthread_override_qos_class_end_np(m_qosClassOverride);
> > +#endif
> >  }
> 
> It seems weird that this is part of the MessageDecoder destructor. I feel
> this type should be about decoding messages, not juggling thread priorities.

It's a bit awkward perhaps, but look at how MessageDecoder also owns importance assertions on OS X.
Comment 5 Anders Carlsson 2015-03-26 12:33:45 PDT
Comment on attachment 249443 [details]
Patch

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

> Source/WebKit2/Platform/IPC/Connection.cpp:690
> +        pthread_override_t o = pthread_override_qos_class_start_np(m_mainThread, QOS_CLASS_USER_INTERACTIVE, 0);

Please call this override instead of just "o".

> Source/WebKit2/Platform/IPC/MessageDecoder.h:63
> +    void setQOSClassOverride(pthread_override_t o) { m_qosClassOverride = o; }

No O.
Comment 6 Andreas Kling 2015-03-26 15:21:26 PDT
Committed r182028: <http://trac.webkit.org/changeset/182028>