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.

Sign up for GitHub

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

Jump to bottom

Concurrent.monotonic_time accept an optional unit parameter #915

Merged
merged 1 commit into from
Jul 30, 2021
Merged

Concurrent.monotonic_time accept an optional unit parameter #915

merged 1 commit into from
Jul 30, 2021

Conversation

casperisfine
Copy link

This is a direct transposition of MRI's clock_gettime unit option.

Tis is a parameter I use quite a lot when writing MRI specific code, but that I miss when using Concurrent.monotonic_time for portability.

More often than not, when you reach for monotonic time, you want to minimize the overhead, so it's appreciable to be able to avoid a / 1_000 when you want milliseconds in the first place.

NB: I'm not too sure how a good way to test the scale of the return value would be.

@chrisseaton
Copy link
Member

When is defined?(Process::CLOCK_MONOTONIC) false?

It's not false on JRuby 9.2.17.0 for example. Process.clock_gettime(Process::CLOCK_MONOTONIC, :millisecond) works fine on JRuby, TruffleRuby, and I'm guessing not even Rubinius but it's hard to build these days.

@headius do you know if this is some kind of ancient fallback?

@casperisfine
Copy link
Author

Older MRI I think. Process.clock_gettime was added in 2.3 if my memory serves me right.

@chrisseaton
Copy link
Member

Oh right didn't realise it was so recent. I will check against older versions and build up a matrix of what is supported where.

@byroot byroot mentioned this pull request Jun 26, 2021
Merged
@chrisseaton chrisseaton self-requested a review June 28, 2021 16:18
@chrisseaton chrisseaton self-assigned this Jun 28, 2021
chrisseaton
chrisseaton reviewed Jun 28, 2021
View reviewed changes
lib/concurrent-ruby/concurrent/utility/monotonic_time.rb Outdated
end
elsif Concurrent.on_jruby?
# @!visibility private
def get_time
java.lang.System.nanoTime() / 1_000_000_000.0
self::TIME_UNITS = Hash.new { |_hash, key| raise ArgumentError, "unexpected unit: #{key}" }.compare_by_identity
Copy link
Member

Choose a reason for hiding this comment

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

Looking at CI I don't think this syntax worked the way it does now on Ruby 1.9. Do you want to look at that and refactor so it works on older Rubies?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, definitely plan to, it's just getting hard to have such old rubies working locally.

I'll fix that tomorrow and ping you back.

Copy link
Member

Choose a reason for hiding this comment

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

I don't know if supporting Ruby 1.9 is an intelligent thing to do anymore?

Copy link
Author

Choose a reason for hiding this comment

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

I'd say no, but I have no idea what the support policy of the gem is.

@chrisseaton
Copy link
Member

Seems good to merge when CI passes on older Rubies.

I'll look at the TruffleRuby failure that's unrelated.

@byroot
Concurrent.monotonic_time accept an optional unit parameter
12e76cd 
This is a direct transposition of MRI's `clock_gettime` unit option.
@casperisfine
Copy link
Author

@chrisseaton I believe I fixed the CI (well it has various failures but unrelated to the current PR).

I also think the implementation is now simpler, as for the simple case of Process.clock_getime being present we save one method call.

@chrisseaton
Copy link
Member

I'm sorry that CI is in such a mess. We really need to get a grip of it.

@chrisseaton chrisseaton merged commit 737da67 into ruby-concurrency:master Jul 30, 2021
@chrisseaton chrisseaton deleted the monotonic-time-ms branch July 30, 2021 22:44
@casperisfine casperisfine mentioned this pull request Oct 21, 2021
Merged
koic added a commit to koic/rubocop-performance that referenced this pull request Oct 30, 2021
@koic
Add new Performance/ConcurrentMonotonicTime cop
9194d50 
Follow up to rails/rails#43502.

This PR adds new `Performance/ConcurrentMonotonicTime` cop.

This cop identifies places where `Concurrent.monotonic_time`
can be replaced by `Process.clock_gettime(Process::CLOCK_MONOTONIC)`.

```ruby
# bad
Concurrent.monotonic_time

# good
Process.clock_gettime(Process::CLOCK_MONOTONIC)
```

Below is a benchmark.

```console
% ruby monotonic_time.rb
"ruby 3.0.2p107 (2021-07-07 revision 0db68f0233) [x86_64-darwin19]\n"
Warming up --------------------------------------
Concurrent.monotonic_time
                       607.064k i/100ms
Process.clock_gettime
                       730.862k i/100ms
Calculating -------------------------------------
Concurrent.monotonic_time
                          5.695M (± 4.4%) i/s -     28.532M in
                          5.019798s
Process.clock_gettime
                          7.398M (± 2.5%) i/s -     37.274M in
                          5.041776s

Comparison:
Process.clock_gettime:  7397700.9 i/s
Concurrent.monotonic_time:  5695385.0 i/s - 1.30x  (± 0.00) slower
```

And this cop is compatible with ruby-concurrency/concurrent-ruby#915.
koic added a commit to koic/rubocop-performance that referenced this pull request Oct 30, 2021
@koic
Add new Performance/ConcurrentMonotonicTime cop
b0cae18 
Follow up to rails/rails#43502.

This PR adds new `Performance/ConcurrentMonotonicTime` cop.

This cop identifies places where `Concurrent.monotonic_time`
can be replaced by `Process.clock_gettime(Process::CLOCK_MONOTONIC)`.

```ruby
# bad
Concurrent.monotonic_time

# good
Process.clock_gettime(Process::CLOCK_MONOTONIC)
```

Below is a benchmark.

```console
% ruby monotonic_time.rb
"ruby 3.0.2p107 (2021-07-07 revision 0db68f0233) [x86_64-darwin19]\n"
Warming up --------------------------------------
Concurrent.monotonic_time
                       607.064k i/100ms
Process.clock_gettime
                       730.862k i/100ms
Calculating -------------------------------------
Concurrent.monotonic_time
                          5.695M (± 4.4%) i/s -     28.532M in
                          5.019798s
Process.clock_gettime
                          7.398M (± 2.5%) i/s -     37.274M in
                          5.041776s

Comparison:
Process.clock_gettime:  7397700.9 i/s
Concurrent.monotonic_time:  5695385.0 i/s - 1.30x  (± 0.00) slower
```

And this cop is compatible with ruby-concurrency/concurrent-ruby#915.
@koic koic mentioned this pull request Oct 30, 2021
Merged
8 tasks
koic added a commit to koic/rubocop-performance that referenced this pull request Oct 30, 2021
@koic
Add new Performance/ConcurrentMonotonicTime cop
69d541a 
Follow up to rails/rails#43502.

This PR adds new `Performance/ConcurrentMonotonicTime` cop.

This cop identifies places where `Concurrent.monotonic_time`
can be replaced by `Process.clock_gettime(Process::CLOCK_MONOTONIC)`.

```ruby
# bad
Concurrent.monotonic_time

# good
Process.clock_gettime(Process::CLOCK_MONOTONIC)
```

Below is a benchmark.

```ruby
% cat monotonic_time.rb
#!/usr/local/bin/ruby

p `ruby -v`

require 'concurrent'
require 'benchmark/ips'

Benchmark.ips do |x|
  x.report('Concurrent.monotonic_time') { Concurrent.monotonic_time }
  x.report('Process.clock_gettime')     {
  Process.clock_gettime(Process::CLOCK_MONOTONIC) }

  x.compare!
end
```

```console
% ruby monotonic_time.rb
"ruby 3.0.2p107 (2021-07-07 revision 0db68f0233) [x86_64-darwin19]\n"
Warming up --------------------------------------
Concurrent.monotonic_time
                       607.064k i/100ms
Process.clock_gettime
                       730.862k i/100ms
Calculating -------------------------------------
Concurrent.monotonic_time
                          5.695M (± 4.4%) i/s -     28.532M in
                          5.019798s
Process.clock_gettime
                          7.398M (± 2.5%) i/s -     37.274M in
                          5.041776s

Comparison:
Process.clock_gettime:  7397700.9 i/s
Concurrent.monotonic_time:  5695385.0 i/s - 1.30x  (± 0.00) slower
```

And this cop is compatible with ruby-concurrency/concurrent-ruby#915.
@JuanitoFatas JuanitoFatas mentioned this pull request Sep 13, 2022
Merged
7 tasks
patrickm53 pushed a commit to patrickm53/performance-develop-rubyonrails that referenced this pull request Sep 23, 2022
Add new Performance/ConcurrentMonotonicTime cop
2a13bf1 
Follow up to rails/rails#43502.

This PR adds new `Performance/ConcurrentMonotonicTime` cop.

This cop identifies places where `Concurrent.monotonic_time`
can be replaced by `Process.clock_gettime(Process::CLOCK_MONOTONIC)`.

```ruby
# bad
Concurrent.monotonic_time

# good
Process.clock_gettime(Process::CLOCK_MONOTONIC)
```

Below is a benchmark.

```ruby
% cat monotonic_time.rb
#!/usr/local/bin/ruby

p `ruby -v`

require 'concurrent'
require 'benchmark/ips'

Benchmark.ips do |x|
  x.report('Concurrent.monotonic_time') { Concurrent.monotonic_time }
  x.report('Process.clock_gettime')     {
  Process.clock_gettime(Process::CLOCK_MONOTONIC) }

  x.compare!
end
```

```console
% ruby monotonic_time.rb
"ruby 3.0.2p107 (2021-07-07 revision 0db68f0233) [x86_64-darwin19]\n"
Warming up --------------------------------------
Concurrent.monotonic_time
                       607.064k i/100ms
Process.clock_gettime
                       730.862k i/100ms
Calculating -------------------------------------
Concurrent.monotonic_time
                          5.695M (± 4.4%) i/s -     28.532M in
                          5.019798s
Process.clock_gettime
                          7.398M (± 2.5%) i/s -     37.274M in
                          5.041776s

Comparison:
Process.clock_gettime:  7397700.9 i/s
Concurrent.monotonic_time:  5695385.0 i/s - 1.30x  (± 0.00) slower
```

And this cop is compatible with ruby-concurrency/concurrent-ruby#915.
richardstewart0213 added a commit to richardstewart0213/performance-build-Performance-optimization-analysis- that referenced this pull request Nov 4, 2022
@richardstewart0213
Add new Performance/ConcurrentMonotonicTime cop
92886b9 
Follow up to rails/rails#43502.

This PR adds new `Performance/ConcurrentMonotonicTime` cop.

This cop identifies places where `Concurrent.monotonic_time`
can be replaced by `Process.clock_gettime(Process::CLOCK_MONOTONIC)`.

```ruby
# bad
Concurrent.monotonic_time

# good
Process.clock_gettime(Process::CLOCK_MONOTONIC)
```

Below is a benchmark.

```ruby
% cat monotonic_time.rb
#!/usr/local/bin/ruby

p `ruby -v`

require 'concurrent'
require 'benchmark/ips'

Benchmark.ips do |x|
  x.report('Concurrent.monotonic_time') { Concurrent.monotonic_time }
  x.report('Process.clock_gettime')     {
  Process.clock_gettime(Process::CLOCK_MONOTONIC) }

  x.compare!
end
```

```console
% ruby monotonic_time.rb
"ruby 3.0.2p107 (2021-07-07 revision 0db68f0233) [x86_64-darwin19]\n"
Warming up --------------------------------------
Concurrent.monotonic_time
                       607.064k i/100ms
Process.clock_gettime
                       730.862k i/100ms
Calculating -------------------------------------
Concurrent.monotonic_time
                          5.695M (± 4.4%) i/s -     28.532M in
                          5.019798s
Process.clock_gettime
                          7.398M (± 2.5%) i/s -     37.274M in
                          5.041776s

Comparison:
Process.clock_gettime:  7397700.9 i/s
Concurrent.monotonic_time:  5695385.0 i/s - 1.30x  (± 0.00) slower
```

And this cop is compatible with ruby-concurrency/concurrent-ruby#915.
Cute0110 added a commit to Cute0110/Rubocop-Performance that referenced this pull request Sep 28, 2023
@Cute0110
Add new Performance/ConcurrentMonotonicTime cop
c04299b 
Follow up to rails/rails#43502.

This PR adds new `Performance/ConcurrentMonotonicTime` cop.

This cop identifies places where `Concurrent.monotonic_time`
can be replaced by `Process.clock_gettime(Process::CLOCK_MONOTONIC)`.

```ruby
# bad
Concurrent.monotonic_time

# good
Process.clock_gettime(Process::CLOCK_MONOTONIC)
```

Below is a benchmark.

```ruby
% cat monotonic_time.rb
#!/usr/local/bin/ruby

p `ruby -v`

require 'concurrent'
require 'benchmark/ips'

Benchmark.ips do |x|
  x.report('Concurrent.monotonic_time') { Concurrent.monotonic_time }
  x.report('Process.clock_gettime')     {
  Process.clock_gettime(Process::CLOCK_MONOTONIC) }

  x.compare!
end
```

```console
% ruby monotonic_time.rb
"ruby 3.0.2p107 (2021-07-07 revision 0db68f0233) [x86_64-darwin19]\n"
Warming up --------------------------------------
Concurrent.monotonic_time
                       607.064k i/100ms
Process.clock_gettime
                       730.862k i/100ms
Calculating -------------------------------------
Concurrent.monotonic_time
                          5.695M (± 4.4%) i/s -     28.532M in
                          5.019798s
Process.clock_gettime
                          7.398M (± 2.5%) i/s -     37.274M in
                          5.041776s

Comparison:
Process.clock_gettime:  7397700.9 i/s
Concurrent.monotonic_time:  5695385.0 i/s - 1.30x  (± 0.00) slower
```

And this cop is compatible with ruby-concurrency/concurrent-ruby#915.
SerhiiMisiura added a commit to SerhiiMisiura/Rubocop-Performance that referenced this pull request Oct 5, 2023
@SerhiiMisiura
Add new Performance/ConcurrentMonotonicTime cop
793bd98 
Follow up to rails/rails#43502.

This PR adds new `Performance/ConcurrentMonotonicTime` cop.

This cop identifies places where `Concurrent.monotonic_time`
can be replaced by `Process.clock_gettime(Process::CLOCK_MONOTONIC)`.

```ruby
# bad
Concurrent.monotonic_time

# good
Process.clock_gettime(Process::CLOCK_MONOTONIC)
```

Below is a benchmark.

```ruby
% cat monotonic_time.rb
#!/usr/local/bin/ruby

p `ruby -v`

require 'concurrent'
require 'benchmark/ips'

Benchmark.ips do |x|
  x.report('Concurrent.monotonic_time') { Concurrent.monotonic_time }
  x.report('Process.clock_gettime')     {
  Process.clock_gettime(Process::CLOCK_MONOTONIC) }

  x.compare!
end
```

```console
% ruby monotonic_time.rb
"ruby 3.0.2p107 (2021-07-07 revision 0db68f0233) [x86_64-darwin19]\n"
Warming up --------------------------------------
Concurrent.monotonic_time
                       607.064k i/100ms
Process.clock_gettime
                       730.862k i/100ms
Calculating -------------------------------------
Concurrent.monotonic_time
                          5.695M (± 4.4%) i/s -     28.532M in
                          5.019798s
Process.clock_gettime
                          7.398M (± 2.5%) i/s -     37.274M in
                          5.041776s

Comparison:
Process.clock_gettime:  7397700.9 i/s
Concurrent.monotonic_time:  5695385.0 i/s - 1.30x  (± 0.00) slower
```

And this cop is compatible with ruby-concurrency/concurrent-ruby#915.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chrisseaton chrisseaton chrisseaton approved these changes

Assignees

@chrisseaton chrisseaton

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@casperisfine @chrisseaton @byroot