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

Remove the C and Java extensions #73

Merged
merged 23 commits into from
Sep 23, 2019
Merged

Remove the C and Java extensions #73

merged 23 commits into from
Sep 23, 2019

Conversation

copiousfreetime
Copy link
Owner

No description provided.

HISTORY.md Outdated
# Hitimes Changeloga
## Version 2.0.0 2019-02-XX

* Remove the C and Ruby extensions as `Process.clock_gettime()` has the same
Copy link
Contributor

Choose a reason for hiding this comment

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

C and Java*

# assuming they all have the same resolution.
#
# On OSX we probably waant to use the MACH time first, and then fall back to
# CLOCK_... Constants on other machines.
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't MRI simply use MACH_ABSOLUTE_TIME_BASED_CLOCK_MONOTONIC when passing Process::CLOCK_MONOTONIC on macOS and so it's the same?

Would it be enough to just always use Process::CLOCK_MONOTONIC for Hitimes? I think the Ruby API already makes a good job at finding the most precise clock for Process::CLOCK_MONOTONIC.

Copy link
Owner Author

Choose a reason for hiding this comment

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

As far as I can tell MRI doesn't map the emulations to constant that they emulate. Looking at https://github.com/ruby/ruby/blob/23a8183bea8dda2d7528b57f2e0c8bc31162db3e/process.c#L7867-L7879 both Process.clock_gettime and Process.clock_getres both have big if/else statements that switch on the clk_id to the appropriate code based for that clk_id - if its a symbol - it does a bunch of the custom code, if its a constant it just sends it to the system clock_gettime

I think the only actual emulation that is done is for win32, where Process::CLOCK_MONOTONIC is mapped to using the QueryPerformance* method: https://github.com/ruby/ruby/blob/23a8183bea8dda2d7528b57f2e0c8bc31162db3e/win32/win32.c#L4685-L4721

Hitimes can't just use CLOCK_MONOTONIC as it varies by machine - As a concrete example:

On OSX:

irb(main):001:0> RUBY_DESCRIPTION
=> "ruby 2.6.1p33 (2019-01-30 revision 66950) [x86_64-darwin18]"
irb(main):005:0> Process.clock_getres(Process::CLOCK_MONOTONIC)
=> 1.0e-06
irb(main):006:0> Process.clock_getres(Process::CLOCK_MONOTONIC_RAW)
=> 1.0e-09
irb(main):007:0> Process.clock_getres(:MACH_ABSOLUTE_TIME_BASED_CLOCK_MONOTONIC)
=> 1.0e-09

On Linux:

irb(main):001:0> RUBY_DESCRIPTION
=> "ruby 2.5.1p57 (2018-03-29 revision 63029) [x86_64-linux-gnu]"
irb(main):002:0> Process.clock_getres(Process::CLOCK_MONOTONIC)
=> 1.0e-09
irb(main):003:0> Process.clock_getres(Process::CLOCK_MONOTONIC_RAW)
=> 1.0e-09

In hitimes I want to provide the highest resolution value possible, so i'm using Process.clock_getres to test the various candidate clock ids to see which one provides the highest resolution and go from there.

Previously with the C extension hitimes just new which method to use based upon the extension that was used - so I had some guarantees. But I may have had a bug too, the linux/bsd version of hitimes was using clock_gettime(CLOCK_MONOTONIC) across the board, and I think that may have been incorrect as the resolution of CLOCK_MONOTONIC isn't guaranteed across systems.

Make sense? I figured testing a few different clock ids's, sorting based upon their resolution, and picking the best one was the best solution.

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense. I didn't know CLOCK_MONOTONIC only has microsecond resolution on macOS.

Maybe for macOS, checking CLOCK_MONOTONIC and CLOCK_MONOTONIC_RAW would be enough instead of using an emulation symbol? The docs say:

Emulations for CLOCK_MONOTONIC:
:MACH_ABSOLUTE_TIME_BASED_CLOCK_MONOTONIC:
  Use mach_absolute_time(), available on Darwin. The resolution is CPU
  dependent.

So either there is CLOCK_MONOTONIC, and hopefully CLOCK_MONOTONIC_RAW too, or there isn't and then CLOCK_MONOTONIC==MACH_ABSOLUTE_TIME_BASED_CLOCK_MONOTONIC.
It's still unclear whether having CLOCK_MONOTONIC implies there is CLOCK_MONOTONIC_RAW too though.

Copy link
Contributor

@eregon eregon May 27, 2019

Choose a reason for hiding this comment

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

A note relevant to this: @chrisseaton and I have been writing specs for clock_getres, and it seems almost every OS returns values for clock_getres() that don't match the actual precision for at least some of the clocks:
https://github.com/ruby/ruby/blob/74c88e7c/spec/ruby/core/process/fixtures/clocks.rb#L19
https://github.com/ruby/ruby/blob/74c88e7c/spec/ruby/core/process/clock_getres_spec.rb#L4-L33

So maybe one workaround is verifying the actual precision returned by clock_gettime for a given CLOCK_ constant.

Otherwise I think using similar logic based on the platform as previously, such as CLOCK_MONOTONIC_RAW on macOS and CLOCK_MONOTONIC on everything else would be good enough, and less subject to OS bugs (or CLOCK_MONOTONIC_RAW if available and CLOCK_MONOTONIC otherwise, these are typically the two clocks used for precise times and benchmarking AFAIK).

Copy link
Contributor

Choose a reason for hiding this comment

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

As @chrisseaton pointed out, the documentation of Process.clock_getres actually acknowledges:

However the result may not be accurate. For example, Process.clock_getres(:GETTIMEOFDAY_BASED_CLOCK_REALTIME) returns 1.0e-06 which means 1 microsecond, but actual resolution can be more coarse.

Copy link
Contributor

Choose a reason for hiding this comment

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

FYI, I filed an issue to remove the broken Process.clock_getres API: https://bugs.ruby-lang.org/issues/16740
I would suggest to choose CLOCK_MONOTONIC and CLOCK_MONOTONIC_RAW by calling Process.clock_gettime a few times and see which one is more precise in practice, as clock_getres lies about the resolution on so many platforms.

Copy link
Owner Author

Choose a reason for hiding this comment

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

@eregon I'll track this over in #76

@eregon
Copy link
Contributor

eregon commented Sep 22, 2019

@copiousfreetime What is the status of this PR? Do you think it could be merged?

FWIW rerunning the CI should let it pass on TruffleRuby.

@copiousfreetime
Copy link
Owner Author

@eregon cool - will rerun and merge - no reason to delay if all is good.

@copiousfreetime copiousfreetime merged commit 477efd3 into master Sep 23, 2019
@eregon
Copy link
Contributor

eregon commented Sep 23, 2019

@copiousfreetime Thanks a lot!

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.

None yet

2 participants