-
-
Notifications
You must be signed in to change notification settings - Fork 925
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
defined? poor performance #3808
Comments
Interesting...shouldn't be hard to find the issue. It is helpful to see the real-world perf issues this is causing, as well. |
In this particular case, we are hitting exception logic. If I require action_dispatch then defined? returns constant and it is much less slow. In fact, it is faster than const_defined?:
And I can see when it is not defined we raise a NameError then catch it and make sure it is not some more important exception and then returns nil. So JRuby's exception handling is as slow as molasses as the cost of backtrace generation is really expensive for us. The solution to this is to redesign defined? to not use exceptions at all internally. This is possible for pure constant case but we need to preserve all semantics of defined? (e.g. Foo::Bar::car of Foo::Bar::Car where Car is a method).
|
Quick note on this. Semantics of Ruby defined? for constants is that if the segment of a colon2 starts with a capital letter it will not try and see if it is a method call unlike if the segment is lower case: Foo::Bar::Gar If any of these three segments is a method and not a constant defined would return nil. As opposed to: Foo::bar::Gar where bar will be looked for as a method and executed as part of defined? resolution. The good news is that these slightly unintuitive semantics means we can fast-path all capital chain constants in a defined call in Java or at least without having to catch a potential exception. Due to some recent discussions of granularity of instructions we will probably make a new instr for this: constant_value operand1, ...operandn Where each operand is just a segment of the colon2/3 chain. A failed lookup will return %unassigned. |
Nice. I inadvertently fixed this in 9.1.2.0 by fixing some other MRI tagged issue. We no longer will trigger the exception:
The before/after are nearly identical results. |
@etehtsea could you verify this is fixed in 9.1.2.0 please? |
@etehtsea sorry I fixed this after 9.1.2.0 was put out. It is fixed on master and will be in 9.1.3.0 after all... |
I've got different results:
with
|
@etehtsea perhaps I am no longer reproducing this because I am not using correct version of gem. Which version of activesupport is this against? I had happened to change some behavior in defined and realized I might have killed the exception thrown and the reran the bench and it seemed cleared up. I also cleaned up my installed gems. I happen to have activesupport 4.2.5 installed atm. |
I just tried making sure this constant cannot exist by adding nonsense on the end of the constant name to guarantee it does not exist and it got even faster. I wonder if this case involves autoload or something in a different version? We are still performing autoload when a constant is on the right-side of a colon2. In any case, I want to see this locally again so I think your versions of gem(s) will do that. |
@enebo I made docker image for you. Simply save script posted below to the
and you should get the same results as I got.
Dockerfile: FROM nimmis/java-centos:openjdk-8-jre
ARG JRUBY_VERSION
ADD jruby-bin-${JRUBY_VERSION}.tar.gz /opt/
ENV PATH /opt/jruby-9.1.3.0-SNAPSHOT/bin:$PATH
RUN ln -s /opt/jruby-9.1.3.0-SNAPSHOT/bin/jruby /opt/jruby-9.1.3.0-SNAPSHOT/bin/ruby && \
gem install bundler -v 1.11.2 |
Forgot to add updated require 'bundler/inline'
gemfile(true) do
source 'https://rubygems.org'
gem 'benchmark-ips'
#gem 'activesupport'
end
require 'benchmark/ips'
# Uncomment for the second bench
# require 'active_support/dependencies'
Benchmark.ips do |x|
x.config(warmup: 30, time: 20)
x.report("const_defined?") do
Object.const_defined?(:ActionDispatch) &&
::ActionDispatch.const_defined?(:Http) &&
::ActionDispatch::Http.const_defined?(:ParameterFilter)
end
x.report("defined?") { defined?(ActionDispatch::Http::ParameterFilter) }
x.compare!
end |
@enebo ping |
@etehtsea I can for some reason reproduce this again. With that said all my original notes in this issue still apply (my thinking it worked before was from another commit which eliminated exceptions for a subset of defined? calls on constants). I did make some improvements in another commit which solved perf for what I guess was a slightly different case. The main work for today will be: a. Change internal call for pure constant colon2 chains A::B to not raise (except see c) b means we need an exception handler to be emitted like we do today (which is basically what c is) and so since a and b are not distinguishable when we generate instrs we need to still wrap with exception handling. in the common case of a this will never actually throw. (PS - I mostly wrote these notes for myself to plan an angle of attack) |
@enebo is it worth to reopen this issue just for the sake of tracking its status? P.S. thanks! |
Warming up --------------------------------------
const_defined? 197.581k i/100ms
defined? 259.411k i/100ms
Calculating -------------------------------------
const_defined? 6.938M (± 8.2%) i/s - 137.714M in 19.995864s
defined? 18.248M (±10.5%) i/s - 360.062M in 19.986502s
Comparison:
defined?: 18248492.6 i/s
const_defined?: 6938116.9 i/s - 2.63x slower This is awesome, thanks! |
Started in newrelic/newrelic-ruby-agent#222
JRuby 9.0.5.0
JRuby 1.7.23
MRI 2.3.0
With
ActiveSupport::Dependencies
required:JRuby 9.0.5.0
JRuby 1.7.23
MRI 2.3.0
P.S.
UPD:
The text was updated successfully, but these errors were encountered: