make setTimeout() low priority during page load

RESOLVED FIXED in Firefox 66

Status

()

defect
RESOLVED FIXED
3 years ago
3 months ago

People

(Reporter: cynthiatang, Assigned: jesup)

Tracking

(Depends on 4 bugs, {perf, site-compat})

unspecified
mozilla66
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(relnote-firefox 66+, firefox66+ fixed)

Details

(Whiteboard: [qf:p1:pageload])

Attachments

(3 attachments, 13 obsolete attachments)

54.06 KB, patch
smaug
: review+
Details | Diff | Splinter Review
2.09 KB, patch
smaug
: review+
Details | Diff | Splinter Review
2.18 KB, patch
davehunt
: review+
jmaher
: review+
Details | Diff | Splinter Review
In profiler result, we can see some heavy tasks come from setTimeOut() at the beginning of google doc loading. If the timer task could be handled later, then we can see the google doc toolbar much earlier. That could improve our user experience for google doc page loading time.


Profiler: https://cleopatra.io/#report=37b1b314384bfff047553f1995a62b608457f830&filter=%5B%7B%22type%22%3A%22RangeSampleFilter%22,%22start%22%3A60024,%22end%22%3A60477%7D,%7B%22type%22%3A%22RangeSampleFilter%22,%22start%22%3A60118,%22end%22%3A60182%7D%5D&selection=0,3902,117,2
Interesting idea. Do we know what should/could be processed before setTimeout callbacks?
Summary: make the setTimeOut() become low priority to speed up the google doc page load time → make the setTimeout() become low priority to speed up the google doc page load time
Blocks: 1264536
Component: DOM: Core & HTML → General
Ben, is this setTimeout bug still meaningful after all the timer work you did for Quantum?
Flags: needinfo?(bkelly)
Its really hard to say without another profile.  My setTimeout() changes will cause timeout callbacks to yield to other main thread work if they take more than 4ms of time.  This only really helps with large numbers of setTimeout() calls, though.  We don't interrupt individual long running callbacks.

In the comment 0 profile there is a single callback that seems to take a long time.  If that is still the case, then my changes won't help.

Its possible, though, that google docs has started using requestIdleCallback() for things that can delayed.  A new profile is probably necessary.
Flags: needinfo?(bkelly)
Assignee: nobody → rjesup
Component: General → Performance
Summary: make the setTimeout() become low priority to speed up the google doc page load time → make setTimeout() low priority during page load
Keywords: perf
Whiteboard: [qf]
Duplicate of this bug: 1488863
Whiteboard: [qf] → [qf:p1:pageload]
Attachment #9037382 - Flags: feedback?(bugs)

The general algorithm here (not written in stone - more like in putty) is to push content setTimeouts/Intervals to a list processed off the Idle queue during pageload. On end of pageload we move them all back to the 'normal' queue. (Note that a Reschedule on that is probably still missing from the patch.)

Chrome does something similar in their scheduling; they push all timers to a different queue which they deprioritize during load. Overall there's a bunch of complexity around this in chrome, which I'm investigating for ideas. They will run timeouts during mainthred-idle periods.

We could use a new idle queue with different parameters. We could handle relative timeouts differently than in this patch...

Comment on attachment 9037382 [details] [diff] [review]
WIP patch to defer setTimeout()s in content during pageload

Looking for some early feedback on the idea of deprioritizing setTimeout firings during load, thanks!
Attachment #9037382 - Flags: feedback?(bzbarsky)
Comment on attachment 9037382 [details] [diff] [review]
WIP patch to defer setTimeout()s in content during pageload


>+nsIEventTarget* TimeoutManager::IdleEventTarget() {
>+  return mWindow.EventTargetFor(TaskCategory::IdleCallback); // XXXX use a new one
>+}
er, hmm, does this actually end up returning anything useful or just leftover thing from Quantum DOM?
https://searchfox.org/mozilla-central/rev/dac799c9f4e9f5f05c1071cba94f2522aa31f7eb/dom/base/nsGlobalWindowInner.cpp#616-621 is how idle dispatching is done.
Attachment #9037382 - Flags: feedback?(bugs)
Fixed the IdleDispatch (though probabaly I can do a better refactoring) and also fixed cancel of a deferred settimeout/interval.  Also now reschedules the normal executor as needed after releasing timeouts on load-complete.  I think a remaining problem is that the timeouts for subdocuments (iframes) are beind released when the subdocument loads, not when the top-level document loads.  bz: just looking for feedback on the plan; not necessarily the code (though you're free to look).  Still a WIP note!
Attachment #9037472 - Flags: feedback?(bzbarsky)
Attachment #9037472 - Flags: feedback?(bugs)
Attachment #9037382 - Attachment is obsolete: true
Attachment #9037382 - Flags: feedback?(bzbarsky)

It's definitely deferring most timeouts now; for facebook typically it doesn't run them until ~load -- or iframe load, I'm checking on that. Nationalgeographic has some idle time, so lots of timeouts run before load.

https://faraday.basschouten.com/mozilla/executionorder/externaltimeout0.html now paints a waffle (in low-resolution....) before running the timeout! But a second run fired Load and released the timeout before we painted, so we'll want to think if there's any adjustment we want to do (small delay after load, or trigger off something later in the pipeline, or leave it). https://perfht.ml/2TXti2z -- in the marker chart under settimeout, you can see that it deferred the timer by 7ms (and ran it from the idle callback) detail: https://perfht.ml/2TYsA54 Second run: https://perfht.ml/2U1kDMO

Loading buzzfeed shows that no timeouts ran off the idle queue (no idle time) -- however, there are a bunch of settimeout release markers, mostly before the final load event, which implies to me that they're due to loads of iframes and their timeouts are being released before the document load: https://perfht.ml/2TYtSwW So it may need additional work on where the load signal comes from.

There are separate markers for setTimeout -- deferred and ran off idle queue -- and setTimeout release -- moved back to head of normal queue after isLoading moves to false (with a MaybeSchedule pass to reset wait time if needed). I'll tweak the markers to make it easier to compare, as well as add a pref to flip this on and off

It builds clean across the board, though tp6 had some sort of problem - https://treeherder.mozilla.org/#/jobs?repo=try&author=rjesup%40wgate.com&selectedJob=222662253

Comment on attachment 9037472 [details] [diff] [review]
WIP patch to defer setTimeout()s in content during pageload

>+  nsPIDOMWindowInner* inner = GetInnerWindow();
>+  if (READYSTATE_LOADING == rs || READYSTATE_INTERACTIVE == rs) {
>+    mLoadingTimeStamp = mozilla::TimeStamp::Now();
mLoadingtimeStamp is set with _LOADING and _INTERACTIVE

>   // At the time of loading start, we don't have timing object, record time.
>-  if (READYSTATE_LOADING == rs) {
>-    mLoadingTimeStamp = mozilla::TimeStamp::Now();
>-  }
But here only with _LOADING

Doesn't the change break all navigation timing handling?

 
>class TimeoutExecutor final : public nsIRunnable,
>                               public nsITimerCallback,
>                               public nsINamed {
>   TimeoutManager* mOwner;
>+  nsIEventTarget* mEventTarget;
>+  bool mIdleQueue;
mIsIdleQueue (when I saw the mIdleQueue usage elsewhere, I thought it was a queue.)


>+++ b/dom/base/Timeout.cpp
>@@ -15,17 +15,19 @@ namespace dom {
> Timeout::Timeout()
>     : mTimeoutId(0),
>       mFiringId(TimeoutManager::InvalidFiringId),
>       mPopupState(PopupBlocker::openAllowed),
>       mReason(Reason::eTimeoutOrInterval),
>       mNestingLevel(0),
>       mCleared(false),
>       mRunning(false),
>-      mIsInterval(false) {}
>+      mIsInterval(false) {
>+    mStart = TimeStamp();
TimeStamps' default ctor should work here.

mStart is a tad vague name. Is it more like... mSchedulingTime?

>+bool TimeoutManager::IsLoading() const {
>+  // A window is considered loading if:
>+  // * it hasn't fired the document load event
>+  // * it's not chrome
>+  // XXX Do we want to distinguish between before DCL and before load?
Do we need to?


>+void TimeoutManager::MoveIdleToActive() {
>+  int num = 0;
uint32_t


>+void TimeoutManager::ProcessIdleTimeouts() {
>+  TimeStamp now = TimeStamp::Now();
>+  RunTimeout(now, now, true); // XXX check
>+}
So nothing calls this method?
Attachment #9037472 - Flags: feedback?(bugs)
Cleaned up a bunch of things.  Needs more asserts and testing, and probably tweaking.  Handles subdocuments by mirroring the loading start down from the top-level document.  Example buzzfeed profile, showing timeouts run off idle ('settimeout') and ones released when we finished loading ('settimeout release') - tags in the textmarkers could be better, and we may not leave them in anyways). Clearly delays LOTS of timeouts; I saw 60 being released on an idle timeout.... (though it will limit the time spent executing timeouts before going through the idle queue again, so that's ok - we may want to tune the max time down though)
Attachment #9037670 - Flags: feedback?(bzbarsky)
Attachment #9037670 - Flags: feedback?(bugs)
Attachment #9037472 - Attachment is obsolete: true
Attachment #9037472 - Flags: feedback?(bzbarsky)
uncomment a line I temporarily commented to test something
Attachment #9037693 - Flags: feedback?(bzbarsky)
Attachment #9037693 - Flags: feedback?(bugs)
Attachment #9037670 - Attachment is obsolete: true
Attachment #9037670 - Flags: feedback?(bzbarsky)
Attachment #9037670 - Flags: feedback?(bugs)
Attachment #9037693 - Flags: feedback?(ehsan)
Updated for smaug's suggestions; added a dom.timeout.defer_during_load (defaults to true) which you can change live (no restart needed) to turn on and off the new settimeout behavior.  Also pushed a full try (without the pref and olli's changes) at https://treeherder.mozilla.org/#/jobs?repo=try&revision=7b22e05b3d82f07736c2ce6ff2e3c8d61b4119dd -- there's at least one UAF (https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=222834596&repo=try&lineNumber=5592) and I haven't look at most of the rest (others I looked t were timeouts), so (no surprise) there's more work to do here.  Ehsan (and bz) - I'm more interested in your thoughts on the strategy here, though I'm happy to have feedback on the implementation
Attachment #9037730 - Flags: feedback?(ehsan)
Attachment #9037693 - Attachment is obsolete: true
Attachment #9037693 - Flags: feedback?(ehsan)
Attachment #9037693 - Flags: feedback?(bzbarsky)
Attachment #9037693 - Flags: feedback?(bugs)

One thing need to be careful at is that if page loading takes forever, as it may, we probably don't want to slow down timers too long time.

And, need to ensure timers get still run before idle queue, since browser uses idle queue for several heavy operations, like GC and CC and object deletion.
As we discussed on IRC, perhaps we could add a new queue to the main thread, something which gets handled after normal priority queue but before idle queue..

Well, even when it does take forever, the timeouts will still run off idle time on mainthread - but they may be delayed more than normal. We could put a max time of say 60 or 120 seconds on the delaying period, or perhaps variable based on <something> - bandwidth, device type (mobile vs desktop), activity, etc. We could also put a max time on how long a timeout will be delayed, though I'm less in love with that idea.

And yes, we should get this ahead of generic idle queue users.

Much cleaner on Try now; mostly wpt1 and Reftest 'C' (and tp6)
Attachment #9037814 - Flags: feedback?(ehsan)
Attachment #9037730 - Attachment is obsolete: true
Attachment #9037730 - Flags: feedback?(ehsan)

Nathan, bz - a ping for your feedback on the approach here. The general idea is to address some of the issues around scheduling during a load, in particular scheduling of setTimeout calls - we've found that part of the difference between us and chrome is that often sites (or ads on sites) schedule a lot of timeouts that we fire before the load finishes - and chrome fires after it finishes (or much later in the load sequence, after all the important parts of the page have rendered). Chrome purposely deprioritizes timeouts during load. From a webcompat point-of-view, we're not required to run them at any particular time (witness what we do with background tabs, etc).

When they run before load, they not only can be long, but they can also start loads of more resources which end up blocking the load event (since they end up in the loadgroup). I've also toyed with making resources loaded from timeouts to be put in a separate loadgroup, so they don't block load from firing. That generally wouldn't affect the appearance of the page in most cases (except where the load event itself does), but we'd turn off the throbber earlier and metrics would show the page as loaded earlier. There's justification (if not compliant with the spec) in that the timeout could have run after the load event, and if so the resources wouldn't have blocked the load event.

I have not done anything along these lines so far; this bug is all about deferring timeouts during load, and only running them when we process the idle queue. (We'll probably run timeouts ahead of any other idle runnables, like CC, etc, but the patch doesn't do this yet).

Thanks for any feedback, or thoughts on heuristics here. (For example, we could be more aggressive at running settimeout's for 0 or not defer them at all.)

Flags: needinfo?(nfroyd)
Flags: needinfo?(bzbarsky)
Comment on attachment 9037814 [details] [diff] [review]
WIP patch to defer setTimeout()s in content during pageload

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

I think the general approach of lowering the priority of setTimeout() callbacks during high priority events such as pageloads makes a lot of sense so in general I think your patch is on the right track.  However I think it could improve in two possible ways:

1. I think the existing idle queue isn't a great place to add timeout handlers to.  Like smaug mentioned in comment 15, there may be other events in the idle queue before the timeout handlers which can be really expensive to run, and that is kind of the point of the idle queue.  I think we really want to add a new queue to the main thread called timer queue specifically for the purpose of storing low-priority timer handlers and nothing else and dispatch the timers during page load time to that queue instead of the idle queue.  Once we have that, there is no need to dispatch to that queue with a fixed timeout really, and we can get the best of both worlds, timeout handlers will have lower priority than all main event queue events during page loads and they won't be starved by the idle queue tasks.  We probably want to make sure that the timer queue is always serviced in order to avoid having to deal with the issues that not doing so would cause in the layers above IMO...

2. I think it would be nice if your patch didn't tie the lowering of the priority of timeouts merely to pageloads.  See this document from the Blink Scheduler docs if you haven't already <https://docs.google.com/document/d/163ow-1wjd6L0rAN3V_U6t12eqVkq4mXDDjVaA4OuvCA/edit> (note that it is rather old and possibly a bit out of date).  Blink lowers the priority of timeouts in several cases, for example during pageload, touch events, scrolling, etc.  I think we would want to also move towards a world where we can dynamically adjust the priority of timeouts as we enter various "use cases" (to borrow from Blink's scheduler vocabulary) in order to ensure that timeout handlers don't get the opportunity to jank the main thread during the times at which we expect to want to run code there.  Page load seems to be the first of these class of use cases, and it seems that with just a little bit of reworking of your patch you could for example tell the TimeoutManager that it's now time to enter low-priority mode etc. rather than the page is now loading.  I think that would be a more future-proof way of constructing this patch (even if the final outcome would be exactly the same functionality-wise.)

Thanks a lot for doing this work, I think the positive impact of this would be something users would appreciate!
Attachment #9037814 - Flags: feedback?(ehsan) → feedback+
  1. I think the existing idle queue isn't a great place to add timeout
    handlers to. Like smaug mentioned in comment 15, there may be other events
    in the idle queue before the timeout handlers which can be really expensive
    to run, and that is kind of the point of the idle queue.

Agreed, and I said something similar in comment 18. This is a starting point/WIP/proof-of-concept before doing the other corollary patches.

  1. I think it would be nice if your patch didn't tie the lowering of the
    priority of timeouts merely to pageloads. See this document from the Blink
    Scheduler docs if you haven't already
    <https://docs.google.com/document/d/163ow-
    1wjd6L0rAN3V_U6t12eqVkq4mXDDjVaA4OuvCA/edit> (note that it is rather old and
    possibly a bit out of date). Blink lowers the priority of timeouts in
    several cases, for example during pageload, touch events, scrolling, etc.

Yes; I started working on this patch after investigating Blink's timer behavior (which we could see was different in profiles). Once we have this, we can look at other inputs/controls; right now we just have inputs into the TimeoutManager; it decides how to schedule. We can certainly add more inputs - or move control of that outside of TimeoutManager as you suggest; that's largely a detail of where we want the scheduling control to live.

Thanks a lot for doing this work, I think the positive impact of this would
be something users would appreciate!

:-) thanks

Comment on attachment 9037814 [details] [diff] [review]
WIP patch to defer setTimeout()s in content during pageload

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

This all looks reasonable to me, insofar as this is code that I'm not really responsible for.

I am not super-excited about the possibility of adding more code to PrioritizedEventQueue, but I also don't see how to put some of the logic in SchedulerGroup/TabGroup/DocGroup or similar.

::: dom/base/TimeoutManager.h
@@ +127,5 @@
>  
>    TimeDuration MinSchedulingDelay() const;
>  
> +  nsresult MaybeSchedule(RefPtr<TimeoutExecutor>& aExecutor,
> +                         const TimeStamp& aWhen,

There's this weird asymmetry in the patch where this function has been seemingly generalized by taking a TimeoutExecutor, but it's only ever used with mExecutor, never with the idle executor.  I don't think that's a good idea; afaict, it's wrong to use it with mIdleExecutor.  Can we change the prototype here and/or change the name?
Flags: needinfo?(nfroyd)
Depends on: 1522150

I am not super-excited about the possibility of adding more code to
PrioritizedEventQueue, but I also don't see how to put some of the logic in
SchedulerGroup/TabGroup/DocGroup or similar.

Right; I think it has to live there somehow, and having a single prioritized queue and doing priority inserts doesn't seem like a great idea.

  • nsresult MaybeSchedule(RefPtr<TimeoutExecutor>& aExecutor,
  •                     const TimeStamp& aWhen,
    

There's this weird asymmetry in the patch where this function has been
seemingly generalized by taking a TimeoutExecutor, but it's only ever used
with mExecutor, never with the idle executor. I don't think that's a good
idea; afaict, it's wrong to use it with mIdleExecutor. Can we change the
prototype here and/or change the name?

Yeah, originally I had it handling both executors, but I don't want to update the budget for idleexecutor when we call MaybeSchedule. I can undo the API change to that method. Alternatively I could keep passing in the executor, and skip UpdateBudget if it's the idle queue. Or pass a boolean in and let MaybeScedule decide. Or have two MaybeSchedule methods,which imply the queue.

Personally, for now I'm just reverting the API change to MaybeSchedule.

Incorporated froyd's comments, removed unneeded event target getter
Attachment #9038629 - Flags: review?(bugs)
Attachment #9037814 - Attachment is obsolete: true
Attachment #9038629 - Attachment is obsolete: true
Attachment #9038629 - Flags: review?(bugs)
Comment on attachment 9038629 [details] [diff] [review]
WIP patch to defer setTimeout()s in content during pageload

># HG changeset patch
># User Randell Jesup <rjesup@jesup.org>
># Date 1548273415 18000
>#      Wed Jan 23 14:56:55 2019 -0500
># Node ID 34ff73c794b7f7cc329e1ab130e21e9674ed2586
># Parent  af55f215496f884325bbec9a5722b62c1c92c97f
>Bug 1270059: WIP patch to defer setTimeout()s in content during pageload
>
>diff --git a/dom/base/Document.cpp b/dom/base/Document.cpp
>--- a/dom/base/Document.cpp
>+++ b/dom/base/Document.cpp
>@@ -8100,24 +8100,58 @@ nsresult Document::CloneDocHelper(Docume
>   // State from Document
>   clone->mType = mType;
>   clone->mXMLDeclarationBits = mXMLDeclarationBits;
>   clone->mBaseTarget = mBaseTarget;
> 
>   return NS_OK;
> }
> 
>+static bool SetLoadingInSubDocument(Document* aDocument, void* aData) {
>+  nsPIDOMWindowInner* inner = aDocument->GetInnerWindow();
>+  if (inner) {
>+    inner->SetLoading(*(static_cast<bool*>(aData)));
>+  }
>+  return true;
>+}
>+
> void Document::SetReadyStateInternal(ReadyState rs) {
>-  mReadyState = rs;
>   if (rs == READYSTATE_UNINITIALIZED) {
>     // Transition back to uninitialized happens only to keep assertions happy
>     // right before readyState transitions to something else. Make this
>     // transition undetectable by Web content.
>-    return;
>-  }
>+    mReadyState = rs;
>+    return;
>+  }
>+
>+  // Mirror the top-level loading state down to all subdocuments
>+  bool set_state = true;
>+  bool new_state = false;
state of what? These variable should have names stating what they are for. So, wasLoading (or was_loading) and isLoading (is_loading) ?

>+  if (READYSTATE_LOADING == rs) {
>+    mLoadingTimeStamp = mozilla::TimeStamp::Now();
>+    new_state = true;
So, if one does document.open() we update state again. I don't think we want that.  It is not really a page load.


>+  } else if (READYSTATE_INTERACTIVE == rs) {
>+    new_state = true;
>+  } else if (mReadyState == READYSTATE_LOADING ||
>+             mReadyState == READYSTATE_INTERACTIVE) {
>+    new_state = false;
Why mReadyState == READYSTATE_LOADING causes new state to be false?


>+  } else {
>+    set_state = false;
>+  }
>+  if (IsTopLevelContentDocument()) {
>+    if (set_state && StaticPrefs::dom_timeout_defer_during_load()) {
>+      nsPIDOMWindowInner* inner = GetInnerWindow();
>+      if (inner) {
>+        inner->SetLoading(new_state);
>+      }
>+      EnumerateSubDocuments(SetLoadingInSubDocument, &new_state);


I don't understand how this stuff works when new iframes are added by scripts during page load.
How do they get loading flag set?
var ifr = document.createElement("iframe");
document.body.appendChild(ifr);
ifr.contentWindow.setTimeout("run some script here");
should still be deferred when top level page loads, I think.



>   enum class Reason : uint8_t {
>     eTimeoutOrInterval,
>     eIdleCallbackTimeout,
>+    eIdleWhileLoading,
I'd prefer some a tad more descriptive name.
eIdleTimeoutOrIntervalWhileLoading 
or something. It is long, but hints that it is about Timeout/Interval, not about the idle callbacks. 

> 
>+  // Time submitted
>+  TimeStamp mSubmitTime;
The comment doesn't really say what the member variable means. Want to clarify a bit.



>+  nsresult rv;
>+  RefPtr<TimeoutExecutor> runnable(this);
You seem to use runnable only when mIsIdleQueue is true. Move inside if() {} ?

>+  if (mIsIdleQueue) {
>+    // We can't just Dispatch() to the Idle eventtarget, since that doesn't
>+    // wrap it in an IdleRunnableWrapper
I don't know what this comment means. There is no idle eventtarget.



>-TimeoutExecutor::TimeoutExecutor(TimeoutManager* aOwner)
>-    : mOwner(aOwner), mMode(Mode::None) {
>+TimeoutExecutor::TimeoutExecutor(TimeoutManager* aOwner, bool aIsIdleQueue,
>+                                 uint32_t aMaxIdleDeferMS)
>+    : mOwner(aOwner),
>+      mIsIdleQueue(aIsIdleQueue),
>+      mMaxIdleDeferMS(aMaxIdleDeferMS),
>+      mMode(Mode::None) {
>   MOZ_DIAGNOSTIC_ASSERT(mOwner);
>+  mEventTarget =
>+      aIsIdleQueue ? mOwner->IdleEventTarget() : mOwner->EventTarget();
Is something missing from the patch. I don't see IdleEventTarget on TimeoutManager
Or is some other bug adding IdleEventTarget

>+  // Note: not related to IsLoading.  Set to false if there's an error, etc.
>+  void SetLoading(bool aIsLoading);
>+
Since this is so easy to confuse with IsLoading, could we perhaps use different name.
Like... AllowDeferredTimeoutHandling or some such.





>diff --git a/modules/libpref/init/all.js b/modules/libpref/init/all.js
>--- a/modules/libpref/init/all.js
>+++ b/modules/libpref/init/all.js
>@@ -1335,16 +1335,20 @@ pref("dom.timeout.foreground_budget_rege
> // Maximum value (in ms) for the background budget. Only valid for
> // values greater than 0.
> pref("dom.timeout.foreground_throttling_max_budget", -1);
> // The maximum amount a timeout can be delayed by budget throttling
> pref("dom.timeout.budget_throttling_max_delay", 15000);
> // Turn on budget throttling by default
> pref("dom.timeout.enable_budget_timer_throttling", true);
> 
>+// Deprioritize timeouts while loading
>+pref("dom.timeout.defer_during_load", true);
>+pref("dom.timeout.max_idle_defer", 10000);
>+
Please using StaticPrefList, also for the max_idle_defer. It is easier and faster to use.
I don't see reason to put anything to all.js



We need some tests ensuring that setTimeout(,0) set during page load fires before setTimeout(,0) set during load event firing. And other similar things.
Attachment #9038629 - Flags: review-
Comment on attachment 9038722 [details] [diff] [review]
WIP patch to defer setTimeout()s in content during pageload

I think my comments for the previous patch apply here too, except the use of IdleEventTarget()
Attachment #9038722 - Flags: review?(bugs) → review-
interdiffs for review changes
Attachment #9038979 - Flags: feedback?(bugs)
merged with review comments
Attachment #9038980 - Flags: review?(bugs)
Attachment #9038722 - Attachment is obsolete: true
our tests include settimeout(..., 0) ordering, and were intermittently failing.  Simple fix
Attachment #9039065 - Flags: review?(bugs)
Attachment #9039065 - Flags: review?(bugs) → review+
Comment on attachment 9038980 [details] [diff] [review]
WIP patch to defer setTimeout()s in content during pageload

>+++ b/dom/base/Document.cpp
>@@ -1290,16 +1290,17 @@ Document::Document(const char* aContentT
> #endif
>       mHasDelayedRefreshEvent(false),
>       mPendingFullscreenRequests(0),
>       mXMLDeclarationBits(0),
>       mOnloadBlockCount(0),
>       mAsyncOnloadBlockCount(0),
>       mCompatMode(eCompatibility_FullStandards),
>       mReadyState(ReadyState::READYSTATE_UNINITIALIZED),
>+      mParentIsLoading(false),
Call the variable mAncestorIsLoading


>+void Document::SetParentLoading(bool aParentIsLoading) {
And this could be then SetAncestorLoading


> nsresult TimeoutExecutor::ScheduleImmediate(const TimeStamp& aDeadline,
>                                             const TimeStamp& aNow) {
>   MOZ_DIAGNOSTIC_ASSERT(mDeadline.IsNull());
>   MOZ_DIAGNOSTIC_ASSERT(mMode == Mode::None);
>   MOZ_DIAGNOSTIC_ASSERT(aDeadline <= (aNow + mAllowedEarlyFiringTime));
> 
>-  nsresult rv =
>-      mOwner->EventTarget()->Dispatch(this, nsIEventTarget::DISPATCH_NORMAL);
>+  nsresult rv;
>+  if (mIsIdleQueue) {
>+    RefPtr<TimeoutExecutor> runnable(this);
>+    MOZ_LOG(gTimeoutLog, LogLevel::Debug, ("Starting IdleDispatch runnable"));
>+    rv = NS_IdleDispatchToCurrentThread(runnable.forget(), mMaxIdleDeferMS,
>+                                        EventQueuePriority::DeferredTimers);
I guess the exact syntax here will depend on the DeferredTimers patch, but the change will be trivial.

>   if (!mTimer) {
>-    mTimer = NS_NewTimer(mOwner->EventTarget());
>+    mTimer = NS_NewTimer(mEventTarget);
This looks like unneeded change. We can just keep using mOwner->EventTarget().
No need to add mEventTarget member variable


>+TimeoutExecutor::TimeoutExecutor(TimeoutManager* aOwner, bool aIsIdleQueue,
>+                                 uint32_t aMaxIdleDeferMS)
>+    : mOwner(aOwner),
>+      mIsIdleQueue(aIsIdleQueue),
>+      mMaxIdleDeferMS(aMaxIdleDeferMS),
>+      mMode(Mode::None) {
>   MOZ_DIAGNOSTIC_ASSERT(mOwner);
>+  mEventTarget = mOwner->EventTarget();
So this mEventTarget is useless


>+void TimeoutManager::SetLoading(bool value) {
>+  // When moving from loading to non-loading, we may need to
>+  // reschedule any existing timeouts from the idle timeout queue
>+  // to the normal queue.
>+  MOZ_LOG(gTimeoutLog, LogLevel::Debug, ("%p: SetLoading(%d)", this, value));
>+  if (mIsLoading && !value) {
>+    MoveIdleToActive();
>+  }
>+  // We don't move existing timeouts to the idle queue if we move to
>+  // loading (XXX if that ever happens?).
Ok, so this comments hints about the model I was expecting, that timeouts go to idle queue during load.
But apparently they go to idle queue only if timer fires during load. Please clarify the comment here.



>+        // MOZ_RELEASE_ASSERT(timeout->When() <= (TimeStamp::Now()));
>+        mIdleTimeouts.InsertBack(timeout);
>+        int num = 0;
>+        for (Timeout* t = mIdleTimeouts.GetFirst(); t != nullptr;
>+             t = t->getNext()) {
>+          num++;
>+        }
We don't want to itarate through idle timouts always, even when logging isn't enabled.
And don't use int, but uint32_t or so.



>diff --git a/modules/libpref/init/StaticPrefList.h b/modules/libpref/init/StaticPrefList.h
...
>+
>+// Maximum deferral time for setTimeout/Interval
... in seconds



Minor things, but I'd like to take a quick look still, since this isn't too simple stuff.

I'd really like to see some test. Something which tests both the pref on and off how timers work during/around page load. Something which has timers also in iframes.
We need such for tracking possible regressions in the future etc.
Attachment #9038980 - Flags: review?(bugs) → review-
Posted patch review.patch (obsolete) — Splinter Review
Attachment #9038979 - Attachment is obsolete: true
Attachment #9038979 - Flags: feedback?(bugs)
Attachment #9039225 - Flags: feedback?(bugs)
* * *
Bug 1270059: interdiffs for review comment changes
* * *
Bug 1270059: Fix timeout ordering when releasing timeouts on Load r=smaug
* * *
Bug 1270059: review response interdiffs
Attachment #9039226 - Flags: review?(bugs)
Attachment #9038980 - Attachment is obsolete: true
Attachment #9039065 - Attachment is obsolete: true
Attachment #9039226 - Flags: review?(bugs) → review+
Nothing should ever be in the mIdleTimeouts list if it wasn't going to fire immediately at the point where we move it from mTimeouts.  Should resolve the wpt test oranges
Attachment #9039251 - Flags: review?(bugs)
I'll take an r+ from anyone on the talos team; I want to land this today or this weekend
Attachment #9039254 - Flags: review?(rwood)
Attachment #9039254 - Flags: review?(dave.hunt)
Attachment #9039251 - Flags: review?(bugs) → review+
Flags: needinfo?(bzbarsky)
Attachment #9039225 - Attachment is obsolete: true
tweaked max extra delay to 5s; only delays if the hero element is late showing up (which appears to only be on tp6-google). Green on try (see above)
Attachment #9039306 - Flags: review?(rwood)
Attachment #9039306 - Flags: review?(jmaher)
Attachment #9039306 - Flags: review?(dave.hunt)
Attachment #9039254 - Attachment is obsolete: true
Attachment #9039254 - Flags: review?(rwood)
Attachment #9039254 - Flags: review?(dave.hunt)
Comment on attachment 9039306 [details] [diff] [review]
fix talos to handle hero elements that are gated on timeouts

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

This makes sense and looks good to me. I’m still not very familiar with this code, so I’ll leave the other review requests open in case you prefer to wait for them before landing.
Attachment #9039306 - Flags: review?(dave.hunt) → review+
Comment on attachment 9039306 [details] [diff] [review]
fix talos to handle hero elements that are gated on timeouts

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

any thoughts on making similar changes to raptor?
Attachment #9039306 - Flags: review?(jmaher) → review+
Pushed by rjesup@wgate.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7f8594d21c51
Defer setTimeout/Intervals()s in content during pageload r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/eec1a6963a9c
fix talos to handle hero elements that are gated on timeouts r=davehunt,jmaher

jmaher - I'm still getting intermittent oranges on Talos TP6 in
https://hg.mozilla.org/integration/mozilla-inbound/rev/cec0a3a47cdb
It was green on my trys. See: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6d624f892c917f19d4dc432c03a07f1dab994ab7

Thoughts? Just increase the time we wait for the Hero? (perhaps it's another item, not the hero, which wasn't done by load time?) Runs fine locally now.

Flags: needinfo?(jmaher)
Depends on: 1523131
Pushed by rjesup@wgate.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/925fed5e69e7
Disable test_ext_async_clipboard.html on android only r=nika

Randell, fyi
Coverity thinks that a part of the code could be simplified

dead_error_condition: The condition aProcessIdle must be true.

https://searchfox.org/mozilla-central/source/dom/base/TimeoutManager.cpp#924

as we do if (aProcessIdle) { a few lines above:
https://searchfox.org/mozilla-central/source/dom/base/TimeoutManager.cpp#917

Flags: needinfo?(rjesup)

Thanks, I'll add it to some of my cleanup (likely I'll remove the outer if() and add markers for all setTimeouts)

Flags: needinfo?(rjesup)
Depends on: 1523189
Depends on: 1523212

as discussed on irc- this is only affecting tp6 stylo-threads=1 which is a test we are not planning to run long term on talos, so disabling the test is fine (in this case tp6-google)

Flags: needinfo?(jmaher)
Comment on attachment 9039306 [details] [diff] [review]
fix talos to handle hero elements that are gated on timeouts

We are planning to turn off Talos tp6 tests completely in favour of Raptor (there is only one remaining Talos tp6 job in production - stylo threads - see Bug 1504251).

:jesup, is this hero element change something that you are planning to make in Raptor also?
Flags: needinfo?(rjesup)
Attachment #9039306 - Flags: review?(rwood)

please create a separate bug for that :)

Depends on: 1523506
Depends on: 1524952
Blocks: 1525337

:jesup, is this hero element change something that you are planning to make
in Raptor also?

Only if we see oranges from it missing the hero; given how raptor works (and that it measures ttfi, which requires more time) I doubt it will.

Flags: needinfo?(rjesup)
Keywords: site-compat
Depends on: 1528336
Depends on: 1528349
Depends on: 1527414

Can you suggest a release note? Is there going to be any sort of blog post describing this that we can link to?

Flags: needinfo?(rjesup)
Flags: needinfo?(rjesup)
You need to log in before you can comment on or make changes to this bug.