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

multi: mandate use "expire ID" for timers #1472

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@bagder
Member

bagder commented May 8, 2017

[internals only, no user-visible change]

This allows us to remove running timers without having them expire "in vain". It makes the typical list of "running timers" much shorter (now usually just 0-2 entries, from previously when it could easily grow to 6-10). It then also allows us to use a fixed array of possible timers so we can do them without using malloc.

@mention-bot

This comment has been minimized.

mention-bot commented May 8, 2017

@bagder, thanks for your PR! By analyzing the history of the files in this pull request, we identified @tatsuhiro-t, @yangtse and @dfandrich to be potential reviewers.

bagder added some commits May 8, 2017

multi: assign IDs to all timers and make each timer singleton
 A) reduces the timeout lists drastically

 B) prevents a lot of superfluous loops for timers that expires "in vain"
    when it has actually already been extended to fire later on
multi: use a fixed array of timers instead of malloc
... since the total amount is low this is faster, easier and reduces
memory overhead.

Also, Curl_expire_done() can now mark an expire timeout as done so that
it never times out.

@bagder bagder closed this in 31b39c4 May 10, 2017

@bagder bagder deleted the bagder/timer-id branch May 11, 2017

@paulharris

This comment has been minimized.

paulharris commented Jun 8, 2017

Hello there,

I have a question about this change. I have only dug into the code a little, so this could be a silly question.

The timers all now have IDs, so you now have a fixed set of timer "categories".
These timers can be cleared/expired early if the code desires (eg no need to wait and check for something).
I get the impression that no matter which timer goes off, the same code will be executed.

If we need to test for something (eg speedtest), the code calls
Curl_expire_latest(id) with the particular timer ID.
IF Curl_expire_latest detects that ANY timer will expire before its expiry, it will NOT update its own timer.
If then, Curl_expire_done() is called and clears the first timer, and there is no other timer that will expire in time, then the speedtest call will miss its deadline.

In code... something like... hmm... make up a scenario....

// task1 needs to be called 10 secs from now.
Curl_expire_latest( 10 secs, EXPIRE_TASK1 );
// result: timer[EXPIRE_TASK1] is set for 10 secs from now
...
// task2 needs to be called 20 secs from now.
Curl_expire_latest( 20 secs, EXPIRE_TASK2 );
// result: nothing.  _latest has detected another timer will expire in 10 secs.
...
// task1 no longer required, mark as done and remove timer
Curl_expire_done( EXPIRE_TASK1 );
// result: no more timers remain.  task2 will not be called before its deadline.

I suggest that Curl_expire_latest become simply Curl_expire(id), and always adjust the timer.

Without digging into the code any more, I would guess that this will not affect performance, because (in my scenario) task1 and task2 are going to be called no matter what timer goes off (my assumption at the top).
When task1 runs, it will be skipped because its no longer needed.
When task2 runs, it will finish by adjusting its timer to fire the next time its needed (or, it should mark it as done and cancel its own timer).

Thoughts?
cheers!

@bagder

This comment has been minimized.

Member

bagder commented Jun 8, 2017

suggest that Curl_expire_latest become simply Curl_expire(id), and always adjust the timer.

Thanks a lot for your valuable thoughts and feedback. Yes I can totally see how you're right and that this change sort of "broke" Curl_expire_latest() for exactly the reasons you state. I intend to proceed as you suggest and replace Curl_expire_latest() with plain Curl_expire()... Thanks again. I'll follow up with a separate PR.

@bagder

This comment has been minimized.

Member

bagder commented Jun 8, 2017

Curl_expire_latest() removal in PR #1555

@paulharris

This comment has been minimized.

paulharris commented Jun 8, 2017

Thanks 👍

@lock lock bot locked as resolved and limited conversation to collaborators May 13, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.