Closed Bug 1425301 Opened 2 years ago Closed 2 years ago

Progressively shorten the 6 seconds max tailing delay with time since nav start (may cause indefinite tailing)

Categories

(Core :: Networking, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox57 --- wontfix
firefox58 --- wontfix
firefox59 --- fixed

People

(Reporter: mayhemer, Assigned: mayhemer)

References

Details

(Whiteboard: [necko-triaged])

Attachments

(1 file, 2 obsolete files)

Discovered when investigating bug 1423373.  The tailing delay of tracker requests is restarted on every non-tailed request opening or finishing.  If there is a repetitive non-tailed request we may never untail.

the "quantum" I'm calculating the delay with may be divided by (or subtracted) the number of seconds (or some (semi)monotonic function of it) from the load start time.  This would keep tailed requests tailed when there is a lot of activity, but also would make sure we untail at a certain moment.
P2 since in combo with 1425296 it may cause an indefinite page load
Priority: -- → P2
Assignee: nobody → honzab.moz
Status: NEW → ASSIGNED
Attached patch wip1 (obsolete) — Splinter Review
backup, I'm also thinking about dropping to zero when the calculated maximum drops below certain threshold of say like 200ms.
Attachment #8938399 - Flags: feedback?(dd.mozilla)
Comment on attachment 8938399 [details] [diff] [review]
wip1

Review of attachment 8938399 [details] [diff] [review]:
-----------------------------------------------------------------

looks good. one question:

do we want a real hard deadline? like a 1min?

::: netwerk/base/RequestContextService.cpp
@@ +261,5 @@
>  
> +  if (!mBeginLoadTime.IsNull()) {
> +    // This seems to me like a reasonable heuristic.  When the loading time
> +    // is twice the max delay (12s), we shorten the tail timer delay to only
> +    // 1 second on every new or finished regular request.  In 24 seconds

the explanation is a bit confusing. you mean 24 after the first 24 second...
please rephrase.
Attachment #8938399 - Flags: feedback?(dd.mozilla) → feedback+
Whiteboard: [necko-triaged]
(In reply to Dragana Damjanovic [:dragana] from comment #3)
> Comment on attachment 8938399 [details] [diff] [review]
> wip1
> 
> Review of attachment 8938399 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> looks good. one question:
> 
> do we want a real hard deadline? like a 1min?

Probably even shorter...  45 seconds maybe?  I think 45 secs is already quite a degraded performance, so we probably can start loading trackers after that time.  Note that this is a totally arbitrarily chosen number.

Let's have "network.http.tailing.total-delay-max" then?

> 
> ::: netwerk/base/RequestContextService.cpp
> @@ +261,5 @@
> >  
> > +  if (!mBeginLoadTime.IsNull()) {
> > +    // This seems to me like a reasonable heuristic.  When the loading time
> > +    // is twice the max delay (12s), we shorten the tail timer delay to only
> > +    // 1 second on every new or finished regular request.  In 24 seconds
> 
> the explanation is a bit confusing. you mean 24 after the first 24 second...
> please rephrase.

I am about to change the calculation so the comment will change as well.

Thanks!
Attached patch v1 (obsolete) — Splinter Review
So, I simply linearly go to 0 with the maxDelay value between load begin and 45 seconds (configurable).  Hence, in ~23 seconds after load start the max delay will be 3 seconds, after 45 seconds and on will be 0, means to untail immediately.  I think this is a clear and well predictable heuristic.
Attachment #8938399 - Attachment is obsolete: true
Attachment #8939898 - Flags: review?(dd.mozilla)
Comment on attachment 8939898 [details] [diff] [review]
v1

Review of attachment 8939898 [details] [diff] [review]:
-----------------------------------------------------------------

Yes, this is a better heuristic. Thanks.

::: netwerk/base/RequestContextService.cpp
@@ +267,5 @@
> +
> +    uint32_t sinceBeginLoad = static_cast<uint32_t>(
> +      (TimeStamp::NowLoRes() - mBeginLoadTime).ToMilliseconds());
> +    uint32_t tillTotal = totalMax - std::min(sinceBeginLoad, totalMax);
> +    uint32_t proportion = (delayMax * tillTotal) / totalMax; // values clamped to 60'000

totalMax can be set to zero, please check before the calculation.
Attachment #8939898 - Flags: review?(dd.mozilla) → review+
Attached patch v1.1Splinter Review
Attachment #8939898 - Attachment is obsolete: true
Attachment #8940699 - Flags: review+
Keywords: checkin-needed
Pushed by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cf08f8b8d13b
Progressively decrease the 6 seconds maximum quantum delay of tracker script tailing, r=dragana
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/cf08f8b8d13b
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
It's too late for 58. Mark 58 won't fix.
You need to log in before you can comment on or make changes to this bug.