Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add TimerWheel #8

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

add TimerWheel #8

wants to merge 2 commits into from

Conversation

flier
Copy link

@flier flier commented Dec 16, 2015

  1. new scheduler base on the TimeWheel algorithm for the massive timers scene
  2. add one time job with After(interval uint64) *Job
  3. add new time unit func (j *Job) Milliseconds() (job *Job)
  4. test cases and benchmark for performance
PASS
BenchmarkSchedule-8  2000000           607 ns/op
BenchmarkPending-8  10000000           391 ns/op
BenchmarkCancel-8    2000000          1007 ns/op

@marksalpeter
Copy link
Collaborator

@flier This isn't working. The interface you implemented is structured much better than the rest of this project and on all accounts the algorithm you're attempting to implement is way more efficient than what is currently implemented. But, when I try to schedule something simple like this your implementation clearly isn't working. Please let me know if you fix it.

    secondCount := 1
    scheduler.Every(1).Second().Do(func() {
        log.Printf("%d second", secondCount)
        secondCount++
    })
    scheduler.Every(1).Minute().Do(func() {
        log.Printf("%d minute", secondCount/60)
    })
    scheduler.Every(1).Hour().Do(func() {
        log.Printf("%d hour", secondCount/(60*60))
    })

marksalpeter added a commit that referenced this pull request Dec 20, 2015
* Scheduler Changes
  * refactored the Scheduler into an interface so that we can potentially swap out implementations (see pull #8)
  * changed `Scheduler.Start() chan bool` to `Scheduler.Start()`
  * added `Scheduler.IsRunning()` and `Scheduler.Stop()` to check if the scheduler is running and to stop it from ronning, respectively
  * Renamed `Scheduler.RunAllwithDelay` to `Scheduler.RunAllWithDelay` to match standard syntax
  * Added runPending(time.Time) in order to synchronize all rin loops on the ticker time
  * Added a mutex lock for concurrency support
  * Added default location support at the scheduler lever with `Scheduler.Location(...)`
* Job Changes
  * moved all initialization logic into `init(time.Time)` and `isInit() bool` for code clarity and testability
  * changed `shouldRun() bool` to `shouldRun(time.Time) bool` in order to accept a passed in time from the current ticker iteration, for run loop synchronicity
  * rewrote `At(...)` to parse into a time.Duration
  * added support for intervals greater than 1 for days of the week
  * added `Weekday(time.Weekday)` and made `Sunday()`, `Monday()`, ... alises of it
  * added `time.Location` support on a per job basis
* standardized the comment structure and added some better godoc documentation
* added support for intervals greater than 1 for any weekday with a strongly defined edge case behavior added to the docs
* fixed #4, by removing the panics in Day, Hour, Minute, Second, etc
* added basic test structure
* removed the global `ChangeLoc(...)` method. In favor of setting the default via the scheduler and on a per job baisis

* finish writing tests and make sure they all pass
* write examples
* update ReadMe.md
@marksalpeter
Copy link
Collaborator

@flier Also, I'm working on a version 1 refactor. Check out the Scheduler interface in the new v1 branch. If you can implement those methods with the timer wheel algorithm we can throw out the old scheduler and use your TimeWheel implementation instead.

@flier
Copy link
Author

flier commented Dec 21, 2015

For the TM scheduler, we need start the scheduler first

https://gist.github.com/flier/749b82879b48dbc739eb

On the other hand, I suggest we could split the Scheduler interface to two
layers, a BaseScheduler focus on the job itself, a Schduler interface to
provider more powerful methods like RunAll/RunAllWithDelay, because some
methods are very expensive or be used seldom.

https://gist.github.com/flier/7823f7ef02eff0058a7f

What's your opinion?

Mark Salpeter [email protected]于2015年12月21日周一 上午6:38写道:

@flier https://github.com/flier Also, I'm working on a version 1
refactor. Check out the Scheduler interface in the new v1 branch
https://github.com/jasonlvhit/gocron/tree/v1.0. If you can implement
those methods with the timer wheel algorithm we can throw out the old
scheduler and use your TimeWheel implementation instead.


Reply to this email directly or view it on GitHub
#8 (comment).

@marksalpeter
Copy link
Collaborator

I actually don't see the use cases for RunAll/RunAllWithDelay, but couldn't those be implemented by iterating over the backing slice of jobs? Is there a good resource for the timer wheel algorithm somewhere so that I can better understand the issue? Is it just a matter of keeping the job slice in sort order at all times?

@flier
Copy link
Author

flier commented Dec 22, 2015

In fact, I implement the time wheel scheduler for some internal networking
projects, we need maintain millions of timers/tickers, that's why I
must suppress all the routines in O(1) or O(log n) instead of lock a slot
too long. That's why I said it may too expensive or be used seldom.

Yes, we can locking and iterating all the slots one by one, if you believe
the unified API will be better, but I doubt it is a real requirements in
such kind of scene :)

Mark Salpeter [email protected]于2015年12月22日周二 上午12:52写道:

I actually don't see the use cases for RunAll/RunAllWithDelay, but
couldn't those be implemented by iterating over the backing slice of jobs?
Is there a good resource for the timer wheel algorithm somewhere so that I
can better understand the issue?


Reply to this email directly or view it on GitHub
#8 (comment).

@denkhaus
Copy link

any progress on that? sounds very promising.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants