andrewjstone / ferris

A hierarchical timer wheel in Rust
Apache License 2.0
17 stars 2 forks source link

Name of `expire` is misleading #6

Open jonhoo opened 6 years ago

jonhoo commented 6 years ago

I completely misunderstood the semantics of expire. I assumed that every time I call it, it will give me any expired timers based on the current time. However, after reading the code (and re-reading the top-level documentation), this does not seem to be the case. Instead, it seems like expire is more like a tick function that must be called exactly every resolution time units, and will return any timers that expired that time unit, which is a very different interface from what I had in mind.

For me personally, this probably means that ferris isn't for me; I specifically want it to work with wall-clock time without requiring exactly regular calls to expire. That doesn't mean that ferris is bad in any way (I partially understand why the interface is this way), but I do think this fact need to be clearly stated, and be reinforced by the method names and type signatures. For example, using Duration is misleading when the methods (again, unless I've misunderstood) do not actually use wall-clock time for anything. Instead, it's more like "number of ticks", which is just a usize.

andrewjstone commented 6 years ago

I'm sorry for the misunderstanding. The implementation is based off section 6B in this paper, and in general is how timer wheels work. The paper talks about a number of different strategies and it's possible that a timer wheel may not be what you want. It sounds to me like what your looking for is a heap or list based priority queue for timers. Then you can call it at arbitrary times and get all expired timers in an iteration over the front of the queue. The problem with this in general is that there is no real accuracy in when the timers should expire and when they do. They are just guaranteed to expire some time after the duration of the timer.

The purpose of the timer wheel is to allow a minimal amount of overhead for determining when a large amount of timers can expire simultaneously. Specifically I use it to track network read or write timeouts. A large number of processes can set a timeout of 5 seconds and get notified very shortly after five seconds that they have expired, with a precision of 10ms or so. Without migration as in #1 there is no work that must be done on expiration other than to alert the process of the timeout and cleanup any references in the cancel hashmap. There's also no need to make a syscall to get the current time, which can be a somewhat expensive operation. But most importantly it's cheap to create and stop a timer, as it doesn't require traversing a list or heap.

It may be that the expire method isn't a precise enough name, but I felt the name in the paper'PerTickBookeeping combined with expiryProcessing to be a little verbose. Perhaps it should be renamed to tick.

I actually disagree that using Duration is misleading. A user wants to schedule a timeout for 5 seconds. They don't want to have to remember how many ticks that is and whether it requires migration. Once a wheel is constructed they just want to start a timer. The timer will infact expire (within some precision) after that Duration. There isn't a duration passed to the expiry call, and there are no references to wall clock time anywhere.

jonhoo commented 6 years ago

Yeah, I don't blame ferris at all! It just didn't do what I at first expected it to do, and I wanted to point out that the interface is part of what led me to believe that it worked this way. Unfortunately, I can't use just a heap because I also want to be able to efficiently cancel a timer by key, which is what attracted me to ferris in the first place.

I think tick is a much better name, because it implies that you need to call it regularly. This is also what the other two Rust libraries for timer wheels use: wheel_timer and pendulum.

As for using Duration, I see the attraction, but I also believe it to be misleading for those not familiar with how timer wheels work internally. Perhaps if there was a multi_tick method that took a Duration and ticked as many times as appropriate for the given duration (the duration would be time since the last call to tick) the connection to "real" time would be clearer. I push on this because if I set a timer for, say, 5ms, and then have a main loop like:

loop {
    for expired in wheel.expire() {
        // do something with expired
    }
    do_some_work();
}

If do_some_work occasionally takes a while (say, 10ms), then my timer won't actually expire until much later than in 5ms. Specifically, even if expire() is called long after 5ms has expired, it is not guaranteed to return that timer. Consider:

let mut wheel = CopyWheel::new(vec![Resolution::ms]);
wheel.start("foo", time::Duration::from_millis(5));
thread::sleep(time::Duration::from_secs(1));
println!("{:?}", wheel.expire().next());

This code will not print Some("foo"), even though the timer duration has obviously expired when expire is called. If "number of ticks" was used on the other hand, then it would make complete sense that start("foo", 5) wouldn't trigger after calling tick() only once.

andrewjstone commented 6 years ago

Ah! I see where you are coming from. Yes, this is a very good point, that I did realize when building this. I didn't worry too much about this in general because I have a thread where all it does is wait for epoll timeouts then places the expired timer ids on a channel where the real work is done elsewhere. That way timers fire on time. There can of course be exceptions but it seems good enough for my use case. I will change the name to tick and add some more docs to make this more clear. Thanks for all the feedback. Once again I mostly concentrated on my use case ignoring some larger issues people may struggle with, so I appreciate the input.