Bug 237782 - Delay responsiveness checks for the Network Process until it has finished initialization
Summary: Delay responsiveness checks for the Network Process until it has finished ini...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2022-03-11 09:18 PST by Chris Dumez
Modified: 2022-03-11 12:23 PST (History)
7 users (show)

See Also:


Attachments
Patch (10.04 KB, patch)
2022-03-11 09:22 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (10.11 KB, patch)
2022-03-11 10:26 PST, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2022-03-11 09:18:42 PST
Delay responsiveness checks for the Network Process until it has finished initialization.

Network process initialization can be slow but we have evidence that it is true truly hung since we see network process terminations right as the network process is initiating the WebProcess connection later on.
Comment 1 Chris Dumez 2022-03-11 09:18:53 PST
<rdar://88226412>
Comment 2 Chris Dumez 2022-03-11 09:22:45 PST
Created attachment 454489 [details]
Patch
Comment 3 Geoffrey Garen 2022-03-11 10:18:02 PST
Comment on attachment 454489 [details]
Patch

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

r=me

> Source/WebKit/ChangeLog:10
> +        Network process initialization can be slow but we have evidence that it is true truly

true truly -> not truly?

> Source/WebKit/UIProcess/AuxiliaryProcessProxy.cpp:371
> +        m_delayedResponsivenessCheck = useLazyStop;

When m_didBeginResponsivenessChecks is true, the *first* useLazyStop value wins, and all subsequent values are ignored so long as the timer is still active.

When m_didBeginResponsivenessChecks is false, the *last* useLazyStop value wins, and all previous values are overwritten.

Is this something we should care about? -- Maybe we don't care because useLazyStop is just a performance optimization for the case where we do schedule the timer?
Comment 4 Chris Dumez 2022-03-11 10:21:05 PST
Comment on attachment 454489 [details]
Patch

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

>> Source/WebKit/ChangeLog:10
>> +        Network process initialization can be slow but we have evidence that it is true truly
> 
> true truly -> not truly?

Oops :)

>> Source/WebKit/UIProcess/AuxiliaryProcessProxy.cpp:371
>> +        m_delayedResponsivenessCheck = useLazyStop;
> 
> When m_didBeginResponsivenessChecks is true, the *first* useLazyStop value wins, and all subsequent values are ignored so long as the timer is still active.
> 
> When m_didBeginResponsivenessChecks is false, the *last* useLazyStop value wins, and all previous values are overwritten.
> 
> Is this something we should care about? -- Maybe we don't care because useLazyStop is just a performance optimization for the case where we do schedule the timer?

Right, it is an optimization for repeated start calls so I don't think it is critical here. That said, I think you're right it's good to be consistent so I'll fix.
Comment 5 Chris Dumez 2022-03-11 10:26:32 PST
Created attachment 454495 [details]
Patch
Comment 6 Sihui Liu 2022-03-11 10:41:34 PST
Do we know the cause of the slowness during initialization (I guess not all crashes point to random call stack )? If we do, could you add comment about the cause to the radar or FIXME in the code so that maybe we can make optimization in the future?
Comment 7 Chris Dumez 2022-03-11 10:58:40 PST
(In reply to Sihui Liu from comment #6)
> Do we know the cause of the slowness during initialization (I guess not all
> crashes point to random call stack )? If we do, could you add comment about
> the cause to the radar or FIXME in the code so that maybe we can make
> optimization in the future?

The comments are already in the radar and as far as I can tell, it is already the same stack we're aware of.
Comment 8 EWS 2022-03-11 12:23:12 PST
Committed r291182 (248341@main): <https://commits.webkit.org/248341@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 454495 [details].