... like GC and CC

Feedback-ish review request. Nothing fancy. Just a wrapper on top of timer which works in main thread and workers and where tick() can come also from non-timers. The change to nsCycleCollector.h is something we'll need to schedule CC. ExternalRunnableWrapper is just moved to a header. I have the nsJSEnvironment converted to use this stuff (using the same scheduling there is now), but looking at workers now and then factoring out stuff out from nsJSEnvironment to somewhere else.

Some early superficial comments: I'm surprised the changes to nsCycleCollector.cpp compile! I filed a bug for adding an IsIdle() method on mIncrementalPhase and also for making the nsCycleCollector methods private. They are accidentally public at the moment. The "thing" in SlowAsynchronousThingScheduler is not great. Task instead of thing maybe?

I could go with Task I guess, though the reason to not use that was that Task in web platform has certain meaning and this is something else. But I guess SlowAsynchronousTask is clear enough

Comment on attachment 8665511 [details] [diff] [review] basic stuff Review of attachment 8665511 [details] [diff] [review]: ----------------------------------------------------------------- Some incomplete comments for now. I'll look at it more tomorrow. ::: dom/workers/RuntimeService.cpp @@ +997,5 @@ > + uint32_t aDelay) override > + { > + NS_ENSURE_STATE(mWorkerPrivate); > + nsRefPtr<ExternalRunnableWrapper> wrapper = > + new ExternalRunnableWrapper(mWorkerPrivate, aRunnable); Somebody else should look at this timer stuff. Or I can try doing some research to figure out how it works. ::: js/xpconnect/src/XPCJSRuntime.cpp @@ +660,5 @@ > + : mRunnable(aRunnable) > + {} > + > + NS_DECL_ISUPPORTS > + trailing whitespace! ::: xpcom/base/SlowAsynchronousThingScheduler.h @@ +3,5 @@ > +/* This Source Code Form is subject to the terms of the Mozilla Public > + * License, v. 2.0. If a copy of the MPL was not distributed with this > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +#ifndef mozilla_SlowAsynchronousThingScheduler_h__ nit: please remove the trailing __ @@ +19,5 @@ > + > +struct DependentSlowThing > +{ > + uint32_t mID; > + uint32_t mMillis; The field name mMillis is not descriptive. What exactly is it for? I haven't yet figured that out from the code. @@ +20,5 @@ > +struct DependentSlowThing > +{ > + uint32_t mID; > + uint32_t mMillis; > + SlowThingCallback mCallback; This feels pretty C-style. Why can't this just be a virtual method? That would let you get rid of mData, too. I'm still trying to figure out how mID and mMillis thing work so I don't have anything more specific to suggest yet. It seems a little odd to expose the internal mID state like this, but maybe it will make sense once I figure the code out. @@ +25,5 @@ > + void* mData; > +}; > + > +// If one has several things which need to run occasionally and those things > +// depend on the each others, one can pass pointer to an array of nit: "the each others" should be "each other", I think, and "pointer" should be "a pointer". @@ +40,5 @@ > +// { eIDOfState1, 10000, TriggerState1, nullptr }, > +// { eIdOfState2, 5000, TriggerState2, nullptr }, > +// { eIdOfFinishState, 0, nullptr, nullptr} > +// }; > +// nit: trailing whitespace @@ +49,5 @@ > +public: > + SlowAsynchronousThingScheduler(); > + ~SlowAsynchronousThingScheduler(); > + > + // Call ExpectedTick() to give SATS hint when the next non-timer based Tick "SATS hint" -> "SATS a hint about". Also, I think it should be "non-timer-based". @@ +50,5 @@ > + SlowAsynchronousThingScheduler(); > + ~SlowAsynchronousThingScheduler(); > + > + // Call ExpectedTick() to give SATS hint when the next non-timer based Tick > + // might be called. For example refresh driver could call this. "For example refresh" -> "For example, the refresh" @@ +82,5 @@ > + CancelTimer(); > + mTimer = nullptr; > + mShuttingDown = true; > + } > +private: nit: Please add a blank line before private. ::: xpcom/base/nsCycleCollector.cpp @@ +4114,5 @@ > PROFILER_LABEL("nsCycleCollector", "collectSlice", > js::ProfileEntry::Category::CC); > > data->mCollector->Collect(SliceCC, budget, nullptr, aPreferShorterSlices); > + return data->mCollector->mIncrementalPhase == IdlePhase; I'm surprised this compiles! I filed a bug for adding an IsIdle() method on mIncrementalPhase and also for making the nsCycleCollector methods private. They are accidentally public at the moment.

Haven't yet figured out how to make your suggestion (class with Run() which returns the next timer value) to nicely create self-documenting code. But I tweaked to array based setup a bit and removed the mData field. Now the scheduling in nsJSEnvironment looks like class CollectorSchedule { public: enum { eInitialGC, eGC, eVariableScheduledGC, eGCSlice, eFullGC, eShrinkGCBuffers, eShrinkingGC, eForgetSkippable, eCCSlice, eNone }; }; static DependentSlowTask sMainThreadCollectorScheduling[] { { CollectorSchedule::eInitialGC, 10000, TriggerGCOrGCSlice }, { CollectorSchedule::eGC, 4000, TriggerGCOrGCSlice }, { CollectorSchedule::eVariableScheduledGC, 0, TriggerGCOrGCSlice }, { CollectorSchedule::eGCSlice, 100, TriggerGCSlice }, { CollectorSchedule::eFullGC, 60000, TriggerFullGC }, { CollectorSchedule::eShrinkGCBuffers, 4000, ShrinkGCBuffers }, { CollectorSchedule::eShrinkingGC, 300000, TriggerShrinkingGC }, { CollectorSchedule::eForgetSkippable, 250, TriggerForgetSkippable }, { CollectorSchedule::eCCSlice, 32, TriggerICCSlice }, { CollectorSchedule::eNone, 0, nullptr } }; One could read each line: Enter state X in Y milliseconds to run Y. Sure, it doesn't tell what Y causes (how to get from Y to some new state), but at least high level scheduling is rather clear. I'll think about the class based approach tomorrow.

Another high-level question that is separate from the task representation: how many tasks do you expect there to be at once? Many of these operations, like Cancel, IsScheduled, and Add are going to take time linear in the number of tasks. If there aren't going to be more than like 20 it probably doesn't matter, but if there are going to be a lot it may start to.

I guess the regular nsITimers just use an array with linear time operations so it should be fine for this, too.

Well, the timer thread at least uses sorted insertion which would be faster than what this is doing.

I expect there to be < 5 tasks

I think this should just use an nsTArray like TimerThread does. Then all of these insert and remove operations can just use existing methods instead of implementing them by hand. For instance, this is how Cancel gets implemented: http://mxr.mozilla.org/mozilla-central/source/xpcom/threads/TimerThread.cpp#658