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 hubs.default_clock and use time.monotonic #303

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
2 participants
@vstinner
Contributor

vstinner commented Mar 3, 2016

  • Add a new eventlet.hubs.default_clock variable to be able to choose
    the default clock for hubs.
  • Modify hub classes to use default_clock as the default clock
  • default_clock uses time.monotonic if available (Python 3.3 or
    newer), or time.time

The asyncio project also uses time.monotonic() by default.

For example information of monotonic clock, see the PEP 418:
https://www.python.org/dev/peps/pep-0418/

Note: I wrote and implemented the PEP 418 in Python 3.3.

Add hubs.default_clock and use time.monotonic
* Add a new eventlet.hubs.default_clock variable to be able to choose
  the default clock for hubs.
* Modify hub classes to use default_clock as the default clock
* default_clock uses time.monotonic if available (Python 3.3 or
  newer), or time.time

The asyncio project also uses time.monotonic() by default.

For example information of monotonic clock, see the PEP 418:
https://www.python.org/dev/peps/pep-0418/

Note: I wrote and implemented the PEP 418 in Python 3.3.
@vstinner

This comment has been minimized.

Show comment
Hide comment
@vstinner

vstinner Mar 3, 2016

Contributor

Workaround proposed in OpenStack (in oslo.service): https://review.openstack.org/286838

Contributor

vstinner commented Mar 3, 2016

Workaround proposed in OpenStack (in oslo.service): https://review.openstack.org/286838

@temoto

This comment has been minimized.

Show comment
Hide comment
@temoto

temoto Mar 3, 2016

Member

👍

Relevant issues: #270 #132

Do you have info about monotonic for 2.7? I only found these:

Monotonic clock is of course the only way to go. Huge thanks for bringing this up, and patch.
I disagree with default_clock though, it's an additional proxy for Python kwarg default value Hub(clock=X). I will rewrite it, please don't be angry. :-)

Member

temoto commented Mar 3, 2016

👍

Relevant issues: #270 #132

Do you have info about monotonic for 2.7? I only found these:

Monotonic clock is of course the only way to go. Huge thanks for bringing this up, and patch.
I disagree with default_clock though, it's an additional proxy for Python kwarg default value Hub(clock=X). I will rewrite it, please don't be angry. :-)

@vstinner

This comment has been minimized.

Show comment
Hide comment
@vstinner

vstinner Mar 3, 2016

Contributor

Do you have info about monotonic for 2.7?

OpenStack uses https://pypi.python.org/pypi/monotonic

I backported the PEP 418 to Python 2, but I didn't publish it on PyPI, since most users only care of time.monotonic() and there are already projects providing a monotonic clock for Python 2.
https://bitbucket.org/haypo/misc/src/tip/python/pep418.py

I disagree with default_clock though, it's an additional proxy for Python kwarg default value Hub(clock=X).

Sorry, I don't understand. The idea of default_clock is to not add a new dependency to eventlet, and let users choose the clock.

Right now, there is no way to choose the clock for all threads. See for example https://review.openstack.org/#/c/286838/ which looks a little bit to a hack to me, that's why I proposed to add default_clock option.

My default_clock option is a compromise for a practical issue, the need of an external dependency.

Well. Another option is to integrate a monotonic clock to eventlet directly. But I have no preference, they are various implementations which advantages and drawbacks. So I prefer to continue to use a 3rd party module for that.

Contributor

vstinner commented Mar 3, 2016

Do you have info about monotonic for 2.7?

OpenStack uses https://pypi.python.org/pypi/monotonic

I backported the PEP 418 to Python 2, but I didn't publish it on PyPI, since most users only care of time.monotonic() and there are already projects providing a monotonic clock for Python 2.
https://bitbucket.org/haypo/misc/src/tip/python/pep418.py

I disagree with default_clock though, it's an additional proxy for Python kwarg default value Hub(clock=X).

Sorry, I don't understand. The idea of default_clock is to not add a new dependency to eventlet, and let users choose the clock.

Right now, there is no way to choose the clock for all threads. See for example https://review.openstack.org/#/c/286838/ which looks a little bit to a hack to me, that's why I proposed to add default_clock option.

My default_clock option is a compromise for a practical issue, the need of an external dependency.

Well. Another option is to integrate a monotonic clock to eventlet directly. But I have no preference, they are various implementations which advantages and drawbacks. So I prefer to continue to use a 3rd party module for that.

@vstinner

This comment has been minimized.

Show comment
Hide comment
@temoto

This comment has been minimized.

Show comment
Hide comment
@temoto

temoto Mar 3, 2016

Member

There is a way to choose clock for all threads with a small risk.

# somewhere close to program initialization, where they usually put monkey_patch()
import time, eventlet.hubs
if hasattr(time, 'monotonic') and eventlet.hubs.get_hub().clock is time.time:
  eventlet.hubs.get_hub().clock = time.monotonic

There's no way you could set eventlet.hubs.default_clock and be sure that incorrect clock wasn't used already. So you always take risk, but putting runtime assign workaround at top or in __main__ is lowest risk.

Another way to restate my previous attack on default_clock: it gives programmer a way to customise default clock function. Immediately, you can see oxymoron here. You don't customise defaults. It works backwards. Default is used when you didn't customise.

Better option to let users choose clock, without risk, with hard guarantee that incorrect clock wasn't used is environment variable.

EVENTLET_CLOCK=time.monotonic python ...

And your version with EVENTLET_HUB=service_hub in Openstack is exactly this, working great. It uses documented environment API to set hub's public attribute. Perfectly correct code. And kudos for service_hub being just a function.

Thanks for monotonic libraries list. I will insert one into Eventlet source, the practice is already established with mock and six.

Member

temoto commented Mar 3, 2016

There is a way to choose clock for all threads with a small risk.

# somewhere close to program initialization, where they usually put monkey_patch()
import time, eventlet.hubs
if hasattr(time, 'monotonic') and eventlet.hubs.get_hub().clock is time.time:
  eventlet.hubs.get_hub().clock = time.monotonic

There's no way you could set eventlet.hubs.default_clock and be sure that incorrect clock wasn't used already. So you always take risk, but putting runtime assign workaround at top or in __main__ is lowest risk.

Another way to restate my previous attack on default_clock: it gives programmer a way to customise default clock function. Immediately, you can see oxymoron here. You don't customise defaults. It works backwards. Default is used when you didn't customise.

Better option to let users choose clock, without risk, with hard guarantee that incorrect clock wasn't used is environment variable.

EVENTLET_CLOCK=time.monotonic python ...

And your version with EVENTLET_HUB=service_hub in Openstack is exactly this, working great. It uses documented environment API to set hub's public attribute. Perfectly correct code. And kudos for service_hub being just a function.

Thanks for monotonic libraries list. I will insert one into Eventlet source, the practice is already established with mock and six.

@temoto

This comment has been minimized.

Show comment
Hide comment
@temoto

temoto Mar 3, 2016

Member

Do you practical experience on CLOCK_MONOTONIC vs CLOCK_MONOTONIC_RAW? From what I read on stackoverflow, the RAW version should be used for timers.

Member

temoto commented Mar 3, 2016

Do you practical experience on CLOCK_MONOTONIC vs CLOCK_MONOTONIC_RAW? From what I read on stackoverflow, the RAW version should be used for timers.

@vstinner

This comment has been minimized.

Show comment
Hide comment
@vstinner

vstinner Apr 19, 2016

Contributor

Do you practical experience on CLOCK_MONOTONIC vs CLOCK_MONOTONIC_RAW? From what I read on stackoverflow, the RAW version should be used for timers.

Please don't use CLOCK_MONOTONIC_RAW. You really want CLOCK_MONOTONIC which is adjusted to use seconds. For time.monotonic(), it was important that the unit is second, since users expect this unit. It's a good thing to adjust CLOCK_MONOTONIC with NTP to really measure seconds and not an arbitrary unit (slower or faster than seconds).

CLOCK_MONOTONIC_RAW must be reserved for low-level applications synchronizing time between multiple computers.

Contributor

vstinner commented Apr 19, 2016

Do you practical experience on CLOCK_MONOTONIC vs CLOCK_MONOTONIC_RAW? From what I read on stackoverflow, the RAW version should be used for timers.

Please don't use CLOCK_MONOTONIC_RAW. You really want CLOCK_MONOTONIC which is adjusted to use seconds. For time.monotonic(), it was important that the unit is second, since users expect this unit. It's a good thing to adjust CLOCK_MONOTONIC with NTP to really measure seconds and not an arbitrary unit (slower or faster than seconds).

CLOCK_MONOTONIC_RAW must be reserved for low-level applications synchronizing time between multiple computers.

@vstinner

This comment has been minimized.

Show comment
Hide comment
@vstinner

vstinner Apr 19, 2016

Contributor

When I wrote this issue, I was thinking at asyncio where the event loop is created explicitly. It looks like the creation of the eventlet is more implicit, and it's harder to control when it is created. So I'm not sure that my design (default_clock) works to configure a clock.

The problem is to set the clock function before the first event loop is created.

Contributor

vstinner commented Apr 19, 2016

When I wrote this issue, I was thinking at asyncio where the event loop is created explicitly. It looks like the creation of the eventlet is more implicit, and it's harder to control when it is created. So I'm not sure that my design (default_clock) works to configure a clock.

The problem is to set the clock function before the first event loop is created.

@temoto

This comment has been minimized.

Show comment
Hide comment
@temoto

temoto Apr 25, 2016

Member

Where did you get this anti-raw information from?

Member

temoto commented Apr 25, 2016

Where did you get this anti-raw information from?

@vstinner

This comment has been minimized.

Show comment
Hide comment
@vstinner

vstinner Apr 25, 2016

Contributor

Linux clock_gettime() manual page:

       CLOCK_MONOTONIC_RAW (since Linux 2.6.28; Linux-specific)
              Similar to CLOCK_MONOTONIC, but provides access to a raw
              hardware-based time that is not subject to NTP adjustments or
              the incremental adjustments performed by adjtime(3).

NTP is used to fix (adjust) clocks to get seconds instead of a clock slower or faster than atomic clocks using seconds.

Other explanations: http://stackoverflow.com/questions/14270300/what-is-the-difference-between-clock-monotonic-clock-monotonic-raw

NTP adjustement is a good thing ;-)

Or: "[CLOCK_MONOTONIC] ... the timestamps from the hardware clock source are being scaled, per adjustments made via adjtime/NTP, to correct for imprecision in the hardware clock rate that would otherwise make it drift relative to real time." http://stackoverflow.com/questions/25583498/clock-monotonic-vs-clock-monotonic-raw-truncated-values

Contributor

vstinner commented Apr 25, 2016

Linux clock_gettime() manual page:

       CLOCK_MONOTONIC_RAW (since Linux 2.6.28; Linux-specific)
              Similar to CLOCK_MONOTONIC, but provides access to a raw
              hardware-based time that is not subject to NTP adjustments or
              the incremental adjustments performed by adjtime(3).

NTP is used to fix (adjust) clocks to get seconds instead of a clock slower or faster than atomic clocks using seconds.

Other explanations: http://stackoverflow.com/questions/14270300/what-is-the-difference-between-clock-monotonic-clock-monotonic-raw

NTP adjustement is a good thing ;-)

Or: "[CLOCK_MONOTONIC] ... the timestamps from the hardware clock source are being scaled, per adjustments made via adjtime/NTP, to correct for imprecision in the hardware clock rate that would otherwise make it drift relative to real time." http://stackoverflow.com/questions/25583498/clock-monotonic-vs-clock-monotonic-raw-truncated-values

@temoto

This comment has been minimized.

Show comment
Hide comment
@temoto

temoto Apr 25, 2016

Member

Seems true, thank you for educating me on this matter.

Member

temoto commented Apr 25, 2016

Seems true, thank you for educating me on this matter.

@vstinner

This comment has been minimized.

Show comment
Hide comment
@vstinner

vstinner Dec 5, 2016

Contributor

Sorry, I'm not interested to update this old pull request anymore, so I just close it.

Contributor

vstinner commented Dec 5, 2016

Sorry, I'm not interested to update this old pull request anymore, so I just close it.

@vstinner vstinner closed this Dec 5, 2016

temoto added a commit that referenced this pull request Feb 15, 2017

hubs: use monotonic clock by default (bundled package); Thanks to Rom…
…an Podoliaka and Victor Stinner

eventlet/support/monotonic.py is copied by curl from specific version on Github.
Change and run bin/pull-monotonic script to update to newer version.

#388
#303

temoto added a commit that referenced this pull request Feb 16, 2017

hubs: use monotonic clock by default (bundled package); Thanks to Rom…
…an Podoliaka and Victor Stinner

eventlet/support/monotonic.py is copied by curl from specific version on Github.
Change and run bin/pull-monotonic script to update to newer version.

#388
#303
@temoto

This comment has been minimized.

Show comment
Hide comment
@temoto

temoto Feb 16, 2017

Member

@haypo similar patch was merged, Eventlet is monotonic by default now. I hope it can shred some burden of patching hubs in OpenStack too.

Member

temoto commented Feb 16, 2017

@haypo similar patch was merged, Eventlet is monotonic by default now. I hope it can shred some burden of patching hubs in OpenStack too.

@vstinner

This comment has been minimized.

Show comment
Hide comment
@vstinner

vstinner Feb 16, 2017

Contributor
Contributor

vstinner commented Feb 16, 2017

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