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

Add ext_monitor gem to improve Monitor class's operations #2670

Merged
merged 2 commits into from
Jan 9, 2020

Conversation

ganmacs
Copy link
Member

@ganmacs ganmacs commented Oct 28, 2019

Which issue(s) this PR fixes:
Fixes #2645

What this PR does / why we need it:

From ruby 2.6.5, Monitor class became slower due to fixing a deadlock bug (https://bugs.ruby-lang.org/issues/15992).
https://github.com/nurse/ext_monitor/ mitigates performance for ruby 2.6.5 or later and improves it for prior to 2.6.4 by 2x(#2645 (comment)).

Docs Changes:

no need

Release Note:

same as the title

@ganmacs
Copy link
Member Author

ganmacs commented Dec 12, 2019

I'll merge this after #2380

@ganmacs ganmacs self-assigned this Jan 6, 2020
@repeatedly
Copy link
Member

repeatedly commented Jan 7, 2020

By introducing ext_monitor in ruby 2.7, how about changing to if ext_monitor is installed, fluentd uses it like oj? It reduces the dependencies and fluentd works even if ext_monitor has a problem.

@ganmacs
Copy link
Member Author

ganmacs commented Jan 8, 2020

Sorry, I had a mistake about a condition of installing.
Ruby 2.7 is bundled ext_monitor like mitigation code by default so 2.7 is fast enough without ext_monitor.

It reduces the dependencies and fluentd works even if ext_monitor has a problem.

Does it mean like below? and removing gem.add_runtime_dependency("ext_monitor", [">= 0.1.1", "< 0.2"]) from gemspec?

if Gem::Version.create(RUBY_VERSION) >= Gem::Version.create('2.7.0')
  require 'monitor'
else
  begin
    require 'ext_monitor'
  rescue LoadError => _
    require 'monitor'
  end
end

@repeatedly
Copy link
Member

Does it mean like below?

Yes.

@ganmacs ganmacs force-pushed the add-ext_monintor branch 2 times, most recently from 06f6a91 to 4ae5416 Compare January 8, 2020 02:40
@ganmacs
Copy link
Member Author

ganmacs commented Jan 8, 2020

fixed.
4ae5416

This log always outputs to STDOUT without ext_monitor. it might be noisy. what do you think of that? @repeatedly

@repeatedly
Copy link
Member

Maybe, such message is not needed.
td-agent and docker images will install ext_monitor.
So most users don't have Monitor issue.

Add to instruction to gem install article is enough.

add move to dev dependency

Signed-off-by: Yuta Iwama <[email protected]>
@ganmacs
Copy link
Member Author

ganmacs commented Jan 8, 2020

Maybe, such message is not needed.

👍

fixed 0a772f2

@ganmacs ganmacs merged commit 8610cd7 into fluent:master Jan 9, 2020
@ganmacs ganmacs deleted the add-ext_monintor branch January 9, 2020 02:52
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.

Faster implementation of monitor than standard library
3 participants