-
Notifications
You must be signed in to change notification settings - Fork 26
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 version.rb and load it from stringio.rb #58
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about just defining StringIO::VERSION
both in ext/stringio/stringio.c
and ext/java/org/jruby/ext/stringio/StringIO.java
instead of introducing lib/stringio/version.rb
?
We can update version information in them automatically by rakelib/version.rake
.
I don't want to use lib/stringio.rb
as much as possible to reduce require
time.
See also: ruby/bigdecimal#148 and ruby/io-console#4
Co-authored-by: Sutou Kouhei <kou@cozmixng.org>
I'm not sure what caused the slow down in those gems you linked, but these days adding the stub stringio.rb and version.rb files really should not make much of a difference. Here's a test of stock 3.1 stringio versus my modified stringio:
Activating stringio as a gem shows no impact either:
Can you show me how the extra .rb files slow down loading stringio? |
As long as this is acceptable and happens consistently, it would be fine with me. It will be a step anyone doing a release has to remember: bump the version, then run rake to update sources. Do you have an example gem that does this today? |
... and commit the results |
You need to have many installed gems. In ruby/io-console#4 case, there are 656 gems:
We always bump the version after a new release. This is a normal release step. And it's already done by
Some repositories in
|
I am not convinced. I have 150 gems installed on this 3.1 and there's basically no difference between my way and the old way:
As I mentioned, I believe improvements in RubyGems and Ruby itself have greatly reduced the cost of loading files from gems, and we're talking about a couple extra files out of thousands required by a typical app. I also believe those stub files were doing a LOT more than this, trying several platforms and failing with LoadError until it finds the right one, which is extremely expensive on any impl. Plus I cannot show it slowing down at all on 3.1.
If that is a standard thing for other core gems then I'm fine with it. At the moment we have been forced to exclude all StringIO specs because of the version checks. I just want this fixed as soon as possible. |
Keep in mind the issues you linked to had 4-5x as many gems with a performance hit of 0.1s or more; my example shows that any slowdown with 150 gems (if I could even show it) would be in the 1-2 millisecond range (8-10ms if it scales with number of gems). I don't understand the exact cause in those issues, but it doesn't seem to be a problem here. |
Ah, sorry. We need one more condition for this problem: begin
require "always-failed"
rescue LoadError
require "stringio.so"
end The We don't have the condition in this case. Sorry. |
Anyway, I still think that the non |
Another approach: #59 |
@kou Your PR looks good to me! |
Close this in favor of #59. |
Fixes #57
This probably will need some discussion. There are no shared .rb files between the C extension gem and the JRuby extension gem. There's no easy way to inject a value into the javac compilation, so it seemed like the best answer is to move the version to a common
stringio/version.rb
file and start shipping thestringio.rb
that loads correctly for JRuby or non-JRuby.I am open to suggestions on a better way to solve this without having the version string in more than one place.