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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implementation of Process.clock_getres and thoughts on clock_ids. #1594

Merged
merged 7 commits into from
Mar 18, 2019

Conversation

copiousfreetime
Copy link
Contributor

I am removing the C and Java extensions from the hitimes gem mostly because of #1436. In my pure-ruby update I am using Process.clock_getres. And I added truffleruby as a Travis CI test ruby version to make sure the pure ruby version of hitimes will work with truffleruby - and it immediately failed

It turns out, an implementation of Process.clock_getres is missing from truffleruby.

As a result - i've implemented a version that passes all the *clock_getres* mri tests via jt test mri ./test/mri/tests/ruby/test_process.rb.

I don't particularly like my implementation and I think part of that is all of the emulation symbols that truffleruby is faking out. Its a bit messy. The resolution of various emulation symbols does not match their implementation resolution. Which may be appropriate - since it is an emulation 馃槃 . I've mapped the resolution of the various symbols to what the documentation says it should be, not what it is implemented as.

For instance :TIME_BASED_CLOCK_REALTIME says in the ruby documentation that its resolution is 1 second:

Use time() defined by ISO C. The resolution is 1 second.

And in truffle ruby this is emulated via Truffle.invoke_primitive(:process_time_currenttimemillis) which technically has a millisecond resolution.

I'm not actually sure that it is necessary for truffleruby to implement all the emulation symbols. All the clock_id constants do not exist on all the platforms, or even different MRI builds. An MRI built on OSX will have :MACH_ABSOLUTE_TIME_BASED_CLOCK_MONOTONIC defined but it won't exist on an MRI that is built on Linux

According to the documentation - http://ruby-doc.org/core-2.4.5/Process.html#method-c-clock_getres

clock_id specifies a kind of clock. It is specified as a constant which begins with Process::CLOCK_ such as Process::CLOCK_REALTIME and Process::CLOCK_MONOTONIC.

The supported constants depends on OS and version. Ruby provides following types of clock_id if available.

I'm probably missing something, but I think it would be reasonable for truffleruby to ignore all the emulations as those are system/platform/engine dependent things.

In any case, I'm happy to contribute this pull request - and receive any and all feedback. If you want me to - I'll be happy to fill out and send in the Oracle Contributor Agreement.

@graalvmbot
Copy link

Hello copiousfreetime, thanks for contributing a PR to our project!

We use the Oracle Contributor Agreement to make the copyright of contributions clear. We don't have a record of you having signed this yet, based on your email address jeremy -(at)- copiousfreetime -(dot)- org. You can sign it at that link.

If you think you've already signed it, please comment below and we'll check.

@chrisseaton
Copy link
Collaborator

Thanks very much for the contribution. We'll look over it and think about the points you raise today. Please start the OCA process though, as we should be able to merge this, as that can take a while.

@eregon
Copy link
Member

eregon commented Feb 25, 2019

@copiousfreetime This looks great, thank you for creating the PR.

I'm probably missing something, but I think it would be reasonable for truffleruby to ignore all the emulations as those are system/platform/engine dependent things.

That sounds reasonable, as end users should ideally not refer to these constants directly.
But, OTOH, copiousfreetime/hitimes#73 does refer to :MACH_ABSOLUTE_TIME_BASED_CLOCK_MONOTONIC, and the documentation mentions them, so it sounds better to support them.

So I think it's good to support the emulation symbols in this PR. If we remove them, we should also remove them for clock_gettime, and that seems out of scope of this PR.

@eregon
Copy link
Member

eregon commented Feb 25, 2019

As a result - i've implemented a version that passes all the *clock_getres* mri tests via jt test mri ./test/mri/tests/ruby/test_process.rb.

@copiousfreetime Nice! Could you enable those tests in CI?
This can be done by jt retag test/mri/tests/ruby/test_process.rb, or removing lines related to clock_getres tests in test/mri/excludes/TestProcess.rb.

@eregon eregon self-requested a review February 25, 2019 12:25
@copiousfreetime
Copy link
Contributor Author

@chrisseaton I've submitted by OCA.

@chrisseaton
Copy link
Collaborator

Thanks you've appeared in the list of OCA signatories. I'll rebase this and run CI.

@chrisseaton
Copy link
Collaborator

I've added a changelog entry, and enabled the MRI tests (they all seem to pass, and you mention they are so you must have tested them but not enabled them?).

@chrisseaton
Copy link
Collaborator

Thanks this passes all tests. I'll just get the team to review and then we'll merge.

@dougxc dougxc merged commit 968f995 into oracle:master Mar 18, 2019
@chrisseaton
Copy link
Collaborator

Thanks very much for this contribution!

I had to modify the formatting of the Ruby code a bit to pass our linter - you can run this yourself as jt rubocop.

This is the final merge.

a8ae4d3

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

5 participants