1358476 - Add nsThread::idleDispatch(nsIRunnable, uint32_t aTimeout)

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement)

Comment 1

[Andreas Farre [:farre]]

Attachment: 0001-Bug-1358476-Add-New-NonOwning-IncrementalRunnableMet.patch

Comment 2

[Andreas Farre [:farre]]

Attachment: 0002-Bug-1358476-Add-timeout-to-idle-dispatch.patch

Comment 3

[Andreas Farre [:farre]]

Quick weekend hack to add timers with idleDispatchWithTimeout. Not really happy about names of methods, and there's still [noscript]. Also not sure about lifetimes of the 0002 patch. More confident about the 0001 patch.

Nathan, the 0001 patch would add New[NonOwning]IncrementalRunnableMethod that uses the same mechanisms that we have for New[NonOwning]CancelableRunnableMethod. With this we can have idle callbacks created with methods where the receiver implements a SetDeadline. Would you be ok with that?

Comment 4

[Nathan Froyd [:froydnj]]

Comment on attachment 8861290 [details] [diff] [review]
0001-Bug-1358476-Add-New-NonOwning-IncrementalRunnableMet.patch

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

I am not super excited about adding another axis for this stuff...but the only other suggestion I have would be some kind of wrapper, which would require multiple heap allocations, which is probably not desirable.

::: xpcom/threads/nsThreadUtils.h
@@ +937,5 @@
> +};
> +
> +template<typename PtrType, typename Method, bool Owning, bool Cancelable, typename... Storages>
> +class RunnableMethodImpl<PtrType, Method, Owning, Cancelable, false, Storages...> final
> +  : public ::nsRunnableMethodTraits<PtrType, Method, Owning, Cancelable, false>::base_type

Why do we need this specialization when we don't need one for Cancelable runnables?

@@ +988,5 @@
>  template<typename PtrType, typename Method, typename... Storages>
>  using CancelableRunnableMethodImpl = RunnableMethodImpl<
> +  typename RemoveReference<PtrType>::Type, Method, true, true, false, Storages...>;
> +
> +// Type aliases for NewIncrementalRunnableMethod.

This name does not make sense to me at first glance; can you explain the rationale here?

Comment 5

[Nathan Froyd [:froydnj]]

Responded in comment 4; I'm not sure I understand some of the bits in part 2, but maybe those will make sense later.

Comment 6

[Andreas Farre [:farre]]

(In reply to Nathan Froyd [:froydnj] from comment #4)
> Comment on attachment 8861290 [details] [diff] [review]
> 0001-Bug-1358476-Add-New-NonOwning-IncrementalRunnableMet.patch
> 
> Review of attachment 8861290 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I am not super excited about adding another axis for this stuff...but the
> only other suggestion I have would be some kind of wrapper, which would
> require multiple heap allocations, which is probably not desirable.

Yep, I'm that guy making complex code more complicated, sorry for that. Bad news is that the 0002 patch introduces a wrapper, and I'm not happy about that either, but now when I think about it more I have an idea of doing it wrapperless. I'll write it up and do another show and tell, hope that's ok.
 
> ::: xpcom/threads/nsThreadUtils.h
> @@ +937,5 @@
> > +};
> > +
> > +template<typename PtrType, typename Method, bool Owning, bool Cancelable, typename... Storages>
> > +class RunnableMethodImpl<PtrType, Method, Owning, Cancelable, false, Storages...> final
> > +  : public ::nsRunnableMethodTraits<PtrType, Method, Owning, Cancelable, false>::base_type
> 
> Why do we need this specialization when we don't need one for Cancelable
> runnables?

I want the specialization because of not wanting to do a QI on the result of nsRunnableMethodReceiver::Get(), but getting a compiler error if someone tries to create an Incremental runnable without a SetDeadline on the receiver. For Cancelable we do a null check on the result of nsRunnableMethodReceiver::Get() instead of specialization, so Incremenatal is more like Owning in that sense.

Would doing the specialization on nsRunnableMethodReceiver in that case be better, or would you prefer QI:ing? I tried out doing the specialization in nsRunnableMethodReceiver and it does look better and it becomes more understandable why, so if you're ok with that then I'm happy to change.

> @@ +988,5 @@
> >  template<typename PtrType, typename Method, typename... Storages>
> >  using CancelableRunnableMethodImpl = RunnableMethodImpl<
> > +  typename RemoveReference<PtrType>::Type, Method, true, true, false, Storages...>;
> > +
> > +// Type aliases for NewIncrementalRunnableMethod.
> 
> This name does not make sense to me at first glance; can you explain the
> rationale here?

Yeah, so the name is not that clear. Incremental runnables only serve a purpose if they're dispatched using nsIThread::idleDispatch which will call SetDeadline before calling runnables. The rationale for the name is that the deadline imposes a budget for the runnable which should be of the kind that it can chunk its work to be able to finish before the deadline and the reschedule itself. Hence the name. 

IdleRunnableMethod wouldn't be an alternative, because there is nothing stopping you from dispatching ordinary runnables or cancelable runnables using idleDispatch, so it hasn't got to do exclusively with idleness really.

Thanks for the feedback!

Comment 7

[Andreas Farre [:farre]]

Attachment: 0001-Bug-1358476-Add-New-NonOwning-IncrementalRunnableMet.patch

Moved specialization to nsRunnableMethodReceiver.

Comment 8

[Andreas Farre [:farre]]

Attachment: 0002-Bug-1358476-Add-timeout-to-idle-dispatch.patch

Made idleDispatchWithTimeout wrapper-less.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

I think this should be actually qf:p1, since this is blocking several other rather important issues.

Comment 10

[(no longer active)]

Agreed.

Comment 11

[Andreas Farre [:farre]]

Attachment: 0001-Bug-1358476-Rename-nsIIncrementalRunnable-to-nsIIdle.patch

Comment 12

[Andreas Farre [:farre]]

Attachment: 0002-Bug-1358476-Add-New-NonOwning-IncrementalRunnableMet.patch

This enables us to do:

class Foo
{
public:
  NS_INLINE_DECL_REFCOUNTING(Foo);
  virtual void SetDeadline(TimeStamp aTimeStamp) { ... }
  void Bar() {}
}

RefPtr<Foo> foo = new Foo;
NS_IdleDispatchToCurrentThread(NewIdleRunnableMethodWithTimeout(foo, &Foo::Bar), 500);

Comment 13

[Andreas Farre [:farre]]

Attachment: 0003-Bug-1358476-Add-tests-for-NewIdleRunnableMethod.patch

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8868512 [details] [diff] [review]
0001-Bug-1358476-Rename-nsIIncrementalRunnable-to-nsIIdle.patch

This one will apparently land in bug 1366750

Comment 15

[Andreas Farre [:farre]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1187468bb874e45e5a02127af46e7aef6873d4e8

Comment 16

[Andreas Farre [:farre]]

Comment on attachment 8868514 [details] [diff] [review]
0002-Bug-1358476-Add-New-NonOwning-IncrementalRunnableMet.patch

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

::: dom/base/nsGlobalWindow.cpp
@@ +585,4 @@
>    NS_DECL_NSINAMED
>    nsresult Cancel() override;
>    void SetDeadline(TimeStamp aDeadline) override;
> +  void SetTimer(uint32_t aDelay, nsIEventTarget* aTarget) {}

This is missing an override.

Comment 17

[Andreas Farre [:farre]]

(In reply to Andreas Farre [:farre] from comment #16)
>
> This is missing an override.

New try run with that fix:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=51b731bedb44baa657886f530904ccbe2e8e3b97

Comment 18

[Nathan Froyd [:froydnj]]

Comment on attachment 8868514 [details] [diff] [review]
0002-Bug-1358476-Add-New-NonOwning-IncrementalRunnableMet.patch

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

I have lots of comments. :)

::: xpcom/threads/nsThreadUtils.h
@@ +119,5 @@
>  extern nsresult
>  NS_IdleDispatchToCurrentThread(already_AddRefed<nsIRunnable>&& aEvent);
>  
> +extern nsresult
> +NS_IdleDispatchToCurrentThread(already_AddRefed<nsIRunnable>&& aEvent, uint32_t aDelay);

At least this function, but preferably this function and its non-delayed counterpart, need documentation.  In particular, it's not clear from looking at the prototype what `aDelay` is useful for, or why the non-delayed version is present.

@@ +411,4 @@
>    IdleRunnable& operator=(const IdleRunnable&&) = delete;
>  };
>  
> +class WithTimer

These classes should be in the mozilla::detail namespace.

@@ +417,5 @@
> +  nsITimer* GetTimer();
> +  void CancelTimer();
> +
> +protected:
> +  virtual ~WithTimer();

Does this destructor really need to be virtual?  We shouldn't even be deleting things through a `WithTimer*`, so we don't need a virtual destructor, right?

@@ +428,5 @@
> +public:
> +  nsITimer* GetTimer() { return nullptr; }
> +  void CancelTimer() {}
> +protected:
> +  virtual ~WithoutTimer() {}

Ditto here.  Especially here, since we'd be adding virtual functions to a great many Runnables that don't need them.

@@ +591,5 @@
> +                                typename mozilla::Conditional<
> +                                  Kind == mozilla::Cancelable,
> +                                  mozilla::CancelableRunnable,
> +                                  mozilla::IdleRunnable>::Type>::Type,
> +    public mozilla::Conditional<Kind == mozilla::IdleWithTimer,

I think this should probably use protected inheritance; we don't expect the interface exposed through inheriting this to be used publically...do we?

I'm inclined to not use Conditional here, but instead do something like:

template<RunnableKind K>
class TimerBehavior
{
public:
  nsITimer* GetTimer() { return nullptr; }
  void CancelTimer() {}
protected:
  ~WithoutTimer() = default;
};

template<>
class TimerBehavior<IdleWithTimer>
{
   ...
};

and then just |public TimerBehavior<Kind>|.

@@ +672,5 @@
>                  "Stored class must inherit from method's class");
>    typedef R return_type;
> +  typedef nsRunnableMethod<C, R, Owning, Kind> base_type;
> +  static const bool can_cancel = Kind == mozilla::Cancelable;
> +  static const bool is_incremental = Kind >= mozilla::Idle;

Consider using a constexpr function like:

static constexpr bool
IsIncremental(RunnableKind aKind)
{
  return aKind == Idle || aKind == IdleWithTimer;
}

If you think using >= tests is better, please add some static asserts that ensure that people adding kinds have to think a little bit, and not blindly add a new kind after IdleWithTimer and botch things.

@@ +1047,5 @@
> +      class_type ClassType;
> +  typedef typename ::
> +    nsRunnableMethodTraits<PtrType, Method, Owning, Kind>::
> +      base_type BaseType;
> +  ::nsRunnableMethodReceiver<ClassType, Owning, Kind >= Idle> mReceiver;

The Kind >= Idle kind of sneaks in here; if we could make the constexpr function approach above work, that would be nicer.  Or we could just get it from the traits class?

@@ +1103,5 @@
> +    }
> +
> +    if (nsCOMPtr<nsITimer> timer = GetTimer()) {
> +      timer->SetTarget(aTarget);
> +      timer->InitWithFuncCallback(TimedOut, this, aDelay, nsITimer::TYPE_ONE_SHOT);

Are you sure that the timer has run or been canceled by this point?  You're not permitted to reinitialize the timer otherwise.

Comment 19

[Nathan Froyd [:froydnj]]

Comment on attachment 8868516 [details] [diff] [review]
0003-Bug-1358476-Add-tests-for-NewIdleRunnableMethod.patch

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

I am skeptical that this does what it's supposed to.

::: xpcom/tests/gtest/TestThreadUtils.cpp
@@ +558,5 @@
> +    NS_IdleDispatchToCurrentThread(NewIdleRunnableMethod<const char*, uint32_t>(
> +      idle, &IdleObject::CheckExecutedMethods, "final", 7));
> +  }
> +
> +  NS_ProcessPendingEvents(nullptr);

Is this really correct?  This will process events until there aren't any events left in the current thread's event queue...which is liable to happen way before some of these timers fire.

Comment 20

[Andreas Farre [:farre]]

Attachment: 0001-Feedback-work.patch

Started working on feedback, :smaug is taking over.

Comment 21

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Nathan Froyd [:froydnj] from comment #19)
> Is this really correct?  This will process events until there aren't any
> events left in the current thread's event queue...which is liable to happen
> way before some of these timers fire.
How so? There is explicit sleep there, and not all timers are supposed to run.

Comment 22

[Olli Pettay [:smaug][[e-mail]]]

Hmm, but locally tests seem to pass even if I add explicit failure.

Comment 23

[Olli Pettay [:smaug][[e-mail]]]

Except that apparently no gtests are running on my machine :/

Comment 24

[Olli Pettay [:smaug][[e-mail]]]

Attachment: + some test changes from smaug

This is a bit less malloc heavy and should make use of nsIIdleRunnable a tad simpler.
Also some coding style nits fixed.

It is not clear to me whether we actually need the *WithTimer methods, but that should also reduce malloc usage when it is used.

Comment 25

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8870676 [details] [diff] [review]
+ some test changes from smaug

This is about tests.

Comment 26

[Olli Pettay [:smaug][[e-mail]]]

Attachment: + some changes from smaug

This is a bit less malloc heavy and should make use of nsIIdleRunnable a tad simpler.
Also some coding style nits fixed.

It is not clear to me whether we actually need the *WithTimer methods, but that should also reduce malloc usage when it is used.

Comment 27

[Olli Pettay [:smaug][[e-mail]]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b7e87038f671da4dd620962275741b574f6d6eef

Comment 28

[Andreas Farre [:farre]]

Comment on attachment 8870677 [details] [diff] [review]
+ some changes from smaug

Really happy about this, many thanks for help tidying up!

Comment 29

[Nathan Froyd [:froydnj]]

(In reply to Olli Pettay [:smaug] from comment #21)
> (In reply to Nathan Froyd [:froydnj] from comment #19)
> > Is this really correct?  This will process events until there aren't any
> > events left in the current thread's event queue...which is liable to happen
> > way before some of these timers fire.
> How so? There is explicit sleep there, and not all timers are supposed to
> run.

Hm?  There's no sleeping in:

http://searchfox.org/mozilla-central/source/xpcom/threads/nsThreadUtils.cpp#310

and passing `false` to nsIThread::ProcessNextEvent means that we're not going to wait for an event if there's nothing in the queue, which will cause us to break from the loop, because we didn't process an event.

If you're right, then adding an:

  idleObject->CheckExecutedMethods("final check", 8);

after the NS_ProcessPendingEvents call should succeed, no?

Comment 30

[Olli Pettay [:smaug][[e-mail]]]

The sleep is in the test.

Comment 31

[Nathan Froyd [:froydnj]]

Comment on attachment 8870676 [details] [diff] [review]
+ some test changes from smaug

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

::: xpcom/tests/gtest/TestThreadUtils.cpp
@@ +536,5 @@
> +  {
> +    CheckExecutedMethods("Method5", 5);
> +    mRunnableExecuted[5] = true;
> +  }
> + 

Nit: trailing whitespace.

Comment 32

[Nathan Froyd [:froydnj]]

Comment on attachment 8870677 [details] [diff] [review]
+ some changes from smaug

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

Bunch of minor things we can sort out before this is committed.

::: xpcom/threads/nsThreadUtils.h
@@ +124,5 @@
> +  *
> + * @returns NS_ERROR_INVALID_ARG
> + *   If event is null.
> + * @returns NS_ERROR_INVALID_ARG
> + *   If the thread is shutting down.

NS_ERROR_INVALID_ARG for the thread shutting down?  That doesn't seem right.

@@ +135,5 @@
> + *
> + * @param aEvent The event to dispatch. If the event implements
> + *   nsIIdleRunnable, it will receive a call on
> + *   nsIIdleRunnable::SetTimeout when dispatched, with the value of
> + *   aTimeout.

nsIIdleRunnable doesn't have a SetTimeout method, according to this patch.

@@ +146,5 @@
> + *
> + * @returns NS_ERROR_INVALID_ARG
> + *   If event is null.
> + * @returns NS_ERROR_INVALID_ARG
> + *   If the thread is shutting down.

Likewise here.

@@ +761,5 @@
> +  static constexpr bool
> +  IsIdle(mozilla::RunnableKind aKind)
> +  {
> +    return aKind == mozilla::Idle || aKind == mozilla::IdleWithTimer;
> +  }

Oh, I wasn't clear in my review comment earlier!  I thought we should do:

static inline constexpr IsIdle(...) {...}

outside of the class definition and then:

// maybe needs constexpr
static const bool is_idle = IsIdle(Kind);

inside the trait definition.  Or did you try that and it not work?

@@ +1134,5 @@
> +    static_assert(Traits::IsIdle(Kind), "Don't use me!");
> +    RefPtr<IdleRunnable> r = static_cast<IdleRunnable*>(aClosure);
> +    r->SetDeadline(TimeStamp());
> +    r->Run();
> +    r->Cancel();

The documentation states that the timeout is when the runnable is moved from the idle queue to the regular queue; this implementation just runs the event straightaway on the timeout.  That seems...a little aggressive.

@@ +1159,5 @@
>    }
> +
> +  nsresult Cancel()
> +  {
> +    static_assert(Kind >= Cancelable, "Don't use me!");

This needs some sort of static_assert on Kind to ensure that the values are in the order we expect; comments in Kind itself would be good.

Comment 33

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Nathan Froyd [:froydnj] from comment #32)
> @@ +135,5 @@
> > + *
> > + * @param aEvent The event to dispatch. If the event implements
> > + *   nsIIdleRunnable, it will receive a call on
> > + *   nsIIdleRunnable::SetTimeout when dispatched, with the value of
> > + *   aTimeout.
> 
> nsIIdleRunnable doesn't have a SetTimeout method, according to this patch.
Ah, SetTimer


> @@ +761,5 @@
> > +  static constexpr bool
> > +  IsIdle(mozilla::RunnableKind aKind)
> > +  {
> > +    return aKind == mozilla::Idle || aKind == mozilla::IdleWithTimer;
> > +  }
> 
> Oh, I wasn't clear in my review comment earlier!  I thought we should do:
> 
> static inline constexpr IsIdle(...) {...}
> 
> outside of the class definition and then:
> 
> // maybe needs constexpr
> static const bool is_idle = IsIdle(Kind);
> 
> inside the trait definition.  Or did you try that and it not work?
> 
I'll try that, and land whatever compiles.


> @@ +1134,5 @@
> > +    static_assert(Traits::IsIdle(Kind), "Don't use me!");
> > +    RefPtr<IdleRunnable> r = static_cast<IdleRunnable*>(aClosure);
> > +    r->SetDeadline(TimeStamp());
> > +    r->Run();
> > +    r->Cancel();
> 
> The documentation states that the timeout is when the runnable is moved from
> the idle queue to the regular queue; this implementation just runs the event
> straightaway on the timeout.  That seems...a little aggressive.
I don't know what this comment means. When the timeout fires, it posts the runnable from timer thread to target thread and that executes the thing here.

> > +  nsresult Cancel()
> > +  {
> > +    static_assert(Kind >= Cancelable, "Don't use me!");
> 
> This needs some sort of static_assert on Kind to ensure that the values are
> in the order we expect; comments in Kind itself would be good.
Not quite sure what kind of static_assert is expected, but I'll add a comment at least.

Comment 34

[Olli Pettay [:smaug][[e-mail]]]

Actually, using Cancel is perfectly ok. I don't know why the static_assert is needed at all.

Comment 35

[Olli Pettay [:smaug][[e-mail]]]

Attachment: tests

Comment 36

[Olli Pettay [:smaug][[e-mail]]]

Attachment: patch

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0ba27996847794bae707cdfb1ba97e9c15adc2c5

Comment 37

[Olli Pettay [:smaug][[e-mail]]]

Attachment: patch

Patch without unrelated changes to nsIThread.idl since some other bug changed the method names there.

Comment 38

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b131d13d4854
add support for timeout when doing idle dispatch, p=farre,smaug, r=nfroyd
https://hg.mozilla.org/integration/mozilla-inbound/rev/02dd110bd682
add support for timeout when doing idle dispatch, tests, p=farre,smaug, r=nfroyd

Comment 39

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b131d13d4854
https://hg.mozilla.org/mozilla-central/rev/02dd110bd682