X Tutup
The Wayback Machine - https://web.archive.org/web/20220328131648/https://github.com/MagicStack/uvloop/pull/428
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 support for <timer handle>.when() #428

Merged
merged 1 commit into from Aug 10, 2021

Conversation

jensbjorgensen
Copy link

@jensbjorgensen jensbjorgensen commented Jul 30, 2021

here's a simple implementation to add support for when() on a timer handle

cdef timespec ts

timespec_get(&ts, 1)
self.when = ts.tv_sec + ts.tv_nsec*1e-9 + self.timeout*1e-3
Copy link
Member

@1st1 1st1 Jul 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution! I'd make this calculation on demand. Add a method to the Timer handle get_when(). Also we need tests.

Copy link
Author

@jensbjorgensen jensbjorgensen Jul 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok instead of calculating the thole thing there you're thinking I should cache the start time (as double instead of the timespec?) and then calculate off of the self.time in get_when, do I understand correctly?

Copy link
Author

@jensbjorgensen jensbjorgensen Aug 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any update @1st1 ? Just looking for a little guidance before I reform my patch

Copy link
Member

@1st1 1st1 Aug 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay! My idea was to do as little work as possible in __cinit__ or __init__, offsetting the bulk of the work to when().

Copy link
Author

@jensbjorgensen jensbjorgensen Aug 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, so I need to at a minimum keep the current time I've sampled with timespec_get(...), so one option to avoid any calculation would be to just keep the timespec struct. I didn't go that route because it would require twice the storage as sizeof(timespec) == 16 vs. sizeof(self.when) == 8. Buf if it's your preference to cache the bytes and defer the calculation then I'll change it as you say. Is that your preference?

Copy link
Member

@1st1 1st1 Aug 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should call loop.time() when the handle is created and store that time in the handle. The method returns monotonic time that's cached for the current iteration and that's what libuv is using internally to compute the timeouts. Then you'd simply need to add the timeout to that captured creation time in .when()

Copy link
Member

@1st1 1st1 Aug 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also using loop.time() is the only way to get consistency with loop.call_at().

Copy link
Author

@jensbjorgensen jensbjorgensen Aug 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok sure, fair enough. That's certainly consistent with the documentation.

Copy link
Author

@jensbjorgensen jensbjorgensen Aug 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, pushed another commit. Works the way you mention and I added one test, not sure what other sort of testing will be worthwhile.

@@ -308,6 +308,9 @@ cdef class TimerHandle:
def cancel(self):
self._cancel()

def when(self):
return self.timer.get_when()*1e-3
Copy link
Member

@1st1 1st1 Aug 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return self.timer.get_when()*1e-3
return self.timer.get_when() * 1e-3

1st1
1st1 approved these changes Aug 9, 2021
Copy link
Member

@1st1 1st1 left a comment

Great work, please fix the one remaining nit

@jensbjorgensen
Copy link
Author

@jensbjorgensen jensbjorgensen commented Aug 9, 2021

ok, did that, so we're there?! :-D

@elprans
Copy link
Member

@elprans elprans commented Aug 10, 2021

tests/test_base.py Outdated Show resolved Hide resolved
@elprans elprans merged commit 62b2af9 into MagicStack:master Aug 10, 2021
0 of 8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
X Tutup