Bug 205795 - [WTR] Use short heart beat timer as a TestOption
Summary: [WTR] Use short heart beat timer as a TestOption
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Service Workers (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-01-06 05:19 PST by youenn fablet
Modified: 2020-01-10 08:49 PST (History)
5 users (show)

See Also:


Attachments
Patch (1.55 KB, patch)
2020-01-06 05:24 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (5.59 KB, patch)
2020-01-06 06:50 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (6.12 KB, patch)
2020-01-07 01:25 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (3.19 KB, patch)
2020-01-09 06:17 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (7.09 KB, patch)
2020-01-10 01:38 PST, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description youenn fablet 2020-01-06 05:19:06 PST
Increase heart beat timer at service worker startup time
Comment 1 youenn fablet 2020-01-06 05:24:18 PST
Created attachment 386833 [details]
Patch
Comment 2 youenn fablet 2020-01-06 06:50:11 PST
Created attachment 386842 [details]
Patch
Comment 3 Alex Christensen 2020-01-06 13:45:28 PST
Could you provide a link to something you think is caused by this timer being too slow?
Comment 5 Alex Christensen 2020-01-06 21:03:04 PST
Comment on attachment 386842 [details]
Patch

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

> Source/WebCore/workers/service/context/ServiceWorkerThread.h:110
>      Seconds m_heartBeatTimeout { heartBeatTimeout };
> +    Seconds m_heartBeatStartTimeout { heartBeatTimeout };

Your ChangeLog entry indicates that you just replaced heartBeatTimeout's initializer from 60 to 300, but you did something more complicated than that.  What does heartBeatStartTimeout mean?
Comment 6 youenn fablet 2020-01-07 01:25:30 PST
Created attachment 386950 [details]
Patch
Comment 7 youenn fablet 2020-01-07 01:26:05 PST
(In reply to Alex Christensen from comment #5)
> Comment on attachment 386842 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=386842&action=review
> 
> > Source/WebCore/workers/service/context/ServiceWorkerThread.h:110
> >      Seconds m_heartBeatTimeout { heartBeatTimeout };
> > +    Seconds m_heartBeatStartTimeout { heartBeatTimeout };
> 
> Your ChangeLog entry indicates that you just replaced heartBeatTimeout's
> initializer from 60 to 300, but you did something more complicated than
> that.  What does heartBeatStartTimeout mean?

Right, I initially did that but this messed up with the sw spinning tests.
I beefed up the change log.
Comment 8 youenn fablet 2020-01-09 06:17:43 PST
Created attachment 387215 [details]
Patch
Comment 9 youenn fablet 2020-01-09 07:13:53 PST
(In reply to youenn fablet from comment #8)
> Created attachment 387215 [details]
> Patch

Reduced the patch size.
Comment 10 Dean Jackson 2020-01-09 10:32:06 PST
Comment on attachment 387215 [details]
Patch

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

> Source/WebCore/workers/service/context/ServiceWorkerThread.cpp:79
> +    , m_heartBeatTimeout(SWContextManager::singleton().connection()->shouldUseShortTimeout() ? heartBeatTimeoutForTest : heartBeatTimeout * 5)

Isn't this a bit confusing? The parameter you pass in isn't what is actually used. Wouldn't it be better to have the calling site pass in a value that is 5x bigger?
Comment 11 Chris Dumez 2020-01-09 14:58:53 PST
Comment on attachment 387215 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        As seen in the bots, especially slow bots, like ios sim debug, 60 seconds is not always sufficient to start service workers.

Isn't the issue that we're using the "short" timeout of 1 second when running the layout tests and 1 second is simply not enough time sometimes to start a service worker in iOS Debug?
Comment 12 youenn fablet 2020-01-10 00:50:34 PST
(In reply to Chris Dumez from comment #11)
> Comment on attachment 387215 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=387215&action=review
> 
> > Source/WebCore/ChangeLog:8
> > +        As seen in the bots, especially slow bots, like ios sim debug, 60 seconds is not always sufficient to start service workers.
> 
> Isn't the issue that we're using the "short" timeout of 1 second when
> running the layout tests and 1 second is simply not enough time sometimes to
> start a service worker in iOS Debug?

Oh right, let's only use the short timeout values for the spin service worker tests (and disable those tests in iOS sim debug).
Comment 13 youenn fablet 2020-01-10 01:38:24 PST
Created attachment 387320 [details]
Patch
Comment 14 WebKit Commit Bot 2020-01-10 08:48:46 PST
Comment on attachment 387320 [details]
Patch

Clearing flags on attachment: 387320

Committed r254341: <https://trac.webkit.org/changeset/254341>
Comment 15 WebKit Commit Bot 2020-01-10 08:48:48 PST
All reviewed patches have been landed.  Closing bug.
Comment 16 Radar WebKit Bug Importer 2020-01-10 08:49:16 PST
<rdar://problem/58480107>