Project

General

Profile

Actions

Feature #16255

closed

Make `monitor.rb` built-in

Added by ko1 (Koichi Sasada) almost 5 years ago. Updated almost 5 years ago.

Status:
Closed
Target version:
-
[ruby-core:95349]

Description

Abstract

monitor.rb introduces MonitorMixin module and Monitor class.
Now, rubygems.rb requires monitor.rb so that most of Ruby applications (*1) requires monitor.rb

[*1] running MRI with --disable-gems can eliminate monitor.rb, but only a few people use this option.

Also recent changes (6ec1720aa38772ba4b7515790443cb578340518b and so on) to make it atomic introduces several overhead.
Making them in C will avoid such overhead.

Naruse-san made monitor-ext gem to make it faster by making C-extension.
This proposal is to make it built-in class.

Approach

Implement Monitor class as re-entrant mutex object in C and implement MonitorMixin with it.
Now, Monitor class is implemented with MonitorMixin, but it is easy to implement Monitor class directly and faster in C.

MonitorMixin and MonitorMixin::ConditionalVariable are remained in lib/monitor.rb, because it is easy to write :p

Implementation

https://github.com/ruby/ruby/compare/master...ko1:thread_monitor

Evaluation

require_relative 'benchmark/lib/load'
Benchmark.driver(repeat_count: 1){|x|
  x.executable name: '2.5.2', command: %w'/home/ko1/ruby/install/ruby_2_5/bin/ruby'
  x.executable name: '2.6.6', command: %w'/home/ko1/ruby/install/ruby_2_6/bin/ruby'
  x.executable name: 'trunk', command: %w'/home/ko1/ruby/install/trunk/bin/ruby'

  x.prelude %{
    mon = Monitor.new
  }

  x.report %{
    mon.synchronize{
      mon.synchronize{
      }
    }
  }
}

#=>
# trunk:   4345545.8 i/s
# 2.5.2:   1657762.1 i/s - 2.62x  slower
# 2.6.6:    499165.7 i/s - 8.71x  slower

Discussion

Queue, Mutex are the alias of Thread::Queue, Thread::Mutex.

p Queue #=> Thread::Queue

I defined Thread::Monitor and make alias by Monitor = Thread::Monitor. Is it okay?

I removed mon_ prefix methods from Monitor class. I believe they are only for MonitorMixin.

... but I found two examples which call mon_ prefix methods for Monitor class:

/home/gem-codesearch/gem-codesearch/latest-gem/gel-0.3.0/lib/gel/db.rb:      @monitor.mon_owned?
/home/gem-codesearch/gem-codesearch/latest-gem/keepyourhead-0.2.2/lib/Keepyourhead/Cache.rb:            @monitor.mon_synchronize {

Should we obsolete them for Monitor class?

Updated by ko1 (Koichi Sasada) almost 5 years ago

Matz wants to make it as an extension library, so I moved it as extension library.

Actions #2

Updated by ko1 (Koichi Sasada) almost 5 years ago

  • Status changed from Open to Closed

Applied in changeset git|caac5f777ae288b5982708b8690e712e1cae0cf6.


make monitor.so for performance. (#2576)

Recent monitor.rb has performance problem because of interrupt
handlers. 'Monitor#synchronize' is frequently used primitive
so the performance of this method is important.

This patch rewrite 'monitor.rb' with 'monitor.so' (C-extension)
and make it faster. See [Feature #16255] for details.

Monitor class objects are normal object which include MonitorMixin.
This patch introduce a Monitor class which is implemented on C
and MonitorMixin uses Monitor object as re-entrant (recursive)
Mutex. This technique improve performance because we don't need
to care atomicity and we don't need accesses to instance variables
any more on Monitor class.

Updated by y-yagi (Yuji Yaginuma) almost 5 years ago

This change breaks the library that extends Monitor class.
For example, Rails has such class and it breaks in Ruby 2.7.
https://github.com/rails/rails/blob/461aae05f194f775e6367be9add4dda37746bd5c/activesupport/lib/active_support/concurrency/load_interlock_aware_monitor.rb#L7-L15

Isn't it guaranteed to make such extending to Monitor?
(I'm not sure if Monitor class is a public API or not)

Updated by Eregon (Benoit Daloze) almost 5 years ago

Could you share the error and backtrace in both of these cases?

Updated by y-yagi (Yuji Yaginuma) almost 5 years ago

The latest google-cloud-pubsub breaks too because new_cond comes before initialization:

Rails encountered the same issue(related to https://github.com/rails/rails/pull/37822).
The error and backtrace of in this case are as follows.
https://buildkite.com/rails/rails/builds/65193#02e136eb-edea-4367-aee0-77e0a82f8531/990-1200

In the first case, the method did not call as expected, so it does not show error and backtrace that related to the issue.

Updated by ko1 (Koichi Sasada) almost 5 years ago

I fixed new_cond before mon_initialize problem c6e3db0c66312af1e932c21006437419efa9ac75

https://github.com/rails/rails/blob/461aae05f194f775e6367be9add4dda37746bd5c/activesupport/lib/active_support/concurrency/load_interlock_aware_monitor.rb#L7-L15

This code depends "how to implement #synchronize" so that I'm not sure MRI should support it for compatibility (I don't think so, if most of people doesn't use this technique).

Updated by y-yagi (Yuji Yaginuma) almost 5 years ago

Thanks for your response! I understood the support policy.

Actions

Also available in: Atom PDF

Like0
Like0Like0Like0Like0Like0Like0Like0Like0