bugzilla.mozilla.org has resumed normal operation. Attachments prior to 2014 will be unavailable for a few days. This is tracked in Bug 1475801.
Please report any other irregularities here.

Add nsThread::idleDispatch(nsIRunnable, uint32_t aTimeout)

RESOLVED FIXED in Firefox 55

Status

()

Core
DOM
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: farre, Assigned: farre)

Tracking

(Blocks: 3 bugs)

unspecified
mozilla55
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [qf:p1])

Attachments

(2 attachments, 11 obsolete attachments)

3.59 KB, patch
Details | Diff | Splinter Review
34.00 KB, patch
Details | Diff | Splinter Review
Comment hidden (empty)
(Assignee)

Updated

a year ago
Blocks: 1355746
Summary: Add nsThread::idleDispatch(nsIRunnable, uint32_t) → Add nsThread::idleDispatch(nsIRunnable, uint32_t aTimeout)
(Assignee)

Comment 1

a year ago
Created attachment 8861290 [details] [diff] [review]
0001-Bug-1358476-Add-New-NonOwning-IncrementalRunnableMet.patch
Assignee: nobody → afarre
(Assignee)

Comment 2

a year ago
Created attachment 8861291 [details] [diff] [review]
0002-Bug-1358476-Add-timeout-to-idle-dispatch.patch
(Assignee)

Updated

a year ago
Blocks: 1353206
(Assignee)

Comment 3

a year ago
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?
Flags: needinfo?(nfroyd)
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?
Responded in comment 4; I'm not sure I understand some of the bits in part 2, but maybe those will make sense later.
Flags: needinfo?(nfroyd)
(Assignee)

Comment 6

a year ago
(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!
(Assignee)

Comment 7

a year ago
Created attachment 8861962 [details] [diff] [review]
0001-Bug-1358476-Add-New-NonOwning-IncrementalRunnableMet.patch

Moved specialization to nsRunnableMethodReceiver.
Attachment #8861290 - Attachment is obsolete: true
(Assignee)

Comment 8

a year ago
Created attachment 8861963 [details] [diff] [review]
0002-Bug-1358476-Add-timeout-to-idle-dispatch.patch

Made idleDispatchWithTimeout wrapper-less.
Attachment #8861291 - Attachment is obsolete: true

Updated

a year ago
Blocks: 1352205
Blocks: 1361461
No longer depends on: 1361461
I think this should be actually qf:p1, since this is blocking several other rather important issues.
Whiteboard: [qf]

Comment 10

a year ago
Agreed.
Whiteboard: [qf] → [qf:p1]
Flags: needinfo?(afarre)
(Assignee)

Updated

a year ago
Attachment #8861962 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Attachment #8861963 - Attachment is obsolete: true
(Assignee)

Comment 11

a year ago
Created attachment 8868512 [details] [diff] [review]
0001-Bug-1358476-Rename-nsIIncrementalRunnable-to-nsIIdle.patch
(Assignee)

Comment 12

a year ago
Created attachment 8868514 [details] [diff] [review]
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);
Flags: needinfo?(afarre)
(Assignee)

Comment 13

a year ago
Created attachment 8868516 [details] [diff] [review]
0003-Bug-1358476-Add-tests-for-NewIdleRunnableMethod.patch
(Assignee)

Updated

a year ago
Attachment #8868514 - Flags: feedback?(nfroyd)
(Assignee)

Updated

a year ago
Attachment #8868514 - Flags: feedback?(nfroyd)
Comment on attachment 8868512 [details] [diff] [review]
0001-Bug-1358476-Rename-nsIIncrementalRunnable-to-nsIIdle.patch

This one will apparently land in bug 1366750
Attachment #8868512 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Attachment #8868514 - Flags: review?(nfroyd)
(Assignee)

Updated

a year ago
Attachment #8868516 - Flags: review?(nfroyd)
(Assignee)

Comment 16

a year ago
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.
(Assignee)

Comment 17

a year ago
(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 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.
Attachment #8868514 - Flags: review?(nfroyd) → feedback+
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.
Attachment #8868516 - Flags: review?(nfroyd) → feedback+
(Assignee)

Comment 20

a year ago
Created attachment 8870466 [details] [diff] [review]
0001-Feedback-work.patch

Started working on feedback, :smaug is taking over.
Flags: needinfo?(bugs)
(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.
Flags: needinfo?(bugs)
Hmm, but locally tests seem to pass even if I add explicit failure.
Except that apparently no gtests are running on my machine :/
Created attachment 8870676 [details] [diff] [review]
+ 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.
Attachment #8870676 - Flags: review?(nfroyd)
Attachment #8870676 - Flags: feedback?(afarre)
Comment on attachment 8870676 [details] [diff] [review]
+ some test changes from smaug

This is about tests.
Attachment #8870676 - Attachment description: + some changes from smaug → + some test changes from smaug
Attachment #8870676 - Flags: feedback?(afarre)
Created attachment 8870677 [details] [diff] [review]
+ 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.
Attachment #8870677 - Flags: review?(nfroyd)
Attachment #8870677 - Flags: feedback?(afarre)
Attachment #8868514 - Attachment is obsolete: true
Attachment #8870466 - Attachment is obsolete: true
Attachment #8868516 - Attachment is obsolete: true
(Assignee)

Comment 28

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

Really happy about this, many thanks for help tidying up!
Attachment #8870677 - Flags: feedback?(afarre) → feedback+
(Assignee)

Updated

a year ago
Blocks: 1367173
(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?
The sleep is in the test.
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.
Attachment #8870676 - Flags: review?(nfroyd) → review+
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.
Attachment #8870677 - Flags: review?(nfroyd) → review+
(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.
Actually, using Cancel is perfectly ok. I don't know why the static_assert is needed at all.
Created attachment 8871023 [details] [diff] [review]
patch

Patch without unrelated changes to nsIThread.idl since some other bug changed the method names there.
Attachment #8870677 - Attachment is obsolete: true
Attachment #8870973 - Attachment is obsolete: true

Comment 38

a year ago
Pushed by opettay@mozilla.com:
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

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b131d13d4854
https://hg.mozilla.org/mozilla-central/rev/02dd110bd682
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox55: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla55

Updated

a year ago
No longer blocks: 1357114
You need to log in before you can comment on or make changes to this bug.