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

Performance issue in JRuby #82

Closed
jdmorani opened this issue Feb 24, 2011 · 16 comments
Closed

Performance issue in JRuby #82

jdmorani opened this issue Feb 24, 2011 · 16 comments

Comments

@jdmorani
Copy link

I opened an issue on mattetti's fork but I figured I would open it here as well :

A performance issue has been discovered when running the i18n in Jruby. Full details of the issue is located here : http://jira.codehaus.org/browse/JRUBY-5534

The 2 comments from Charles Oliver Nutter are insightful. Can you please have a look when you get a chance? Is it something that you would be able to get fixed?

Thanks!
JD

@svenfuchs
Copy link
Collaborator

I'll be able to look into this over the weekend.

I'm not sure switching to throw/catch will be a no-brainer though because backends communicating to the frontend through exceptions has been there for years and we'd probably break stuff. Maybe a config switch and some deprecation will do.

@headius
Copy link

headius commented Feb 24, 2011

FWIW, I did some exploration of other Ruby implementations. Ruby 1.8.7 and JRuby 1.5.6 both had reasonably fast backtrace generation, but Ruby 1.9, Macruby, and Rubinius were all from 40-70x slower (with JRuby ranging from 2-3x slower than that), and I'd wager Maglev and IronRuby have similar characteristics. So this isn't just a JRuby thing, it's a modern Ruby implementation thing; backtraces are no longer free. All popular Ruby implementations will suffer greatly from the extreme number of exceptions being raised in i18n (and other libraries).

I'm planning to write up a blog post about all this, to explain why backtraces have gotten more expensive. I'd be happy to answer any questions you have in the interim.

@svenfuchs
Copy link
Collaborator

Charles, thanks for the explanation. I'll happily try to improve this situation for more recent ruby implementations.

FYI I've started removing the use of exceptions from all backend modules we ship and replaced it with throw/catch:

35f3cf3

It currently still throws instances of MissingTranslationData which is a subclass of Exception.

Charles, I assume that the expensive part (collecting the backtrace) happens when an Exception is instanciated, is that correct? Or will it happen when the exception is raised?

Also, currently I18n.t catches this exception from the backend and then raises it to the client. We'll probably want to replace this as well, but as it's been part of the public API this proabably isn't a low hanging fruit.

@svenfuchs
Copy link
Collaborator

So, I believe the reduce-exceptions branch (https://github.com/svenfuchs/i18n/tree/reduce-exceptions) is now in a shape where

  1. I18n doesn't use exceptions internally anymore for missing translations, but uses throw/catch instead.
  2. I18n also doesn't instantiate a MissingTranslationData exception any more unless forced by the user.
  3. I18n.t still allows to pass :raise => true which will turn the caught missing translation metadata into a MissingTranslationData exception for backwards compat
  4. There's a patched version of the ActionView #translate helper (probably the most widely used "user" out there that rescues our exceptions, see https://github.com/svenfuchs/i18n/blob/reduce-exceptions/lib/i18n/rails/translation_helper.rb) that doesn't force exceptions any more. (Not sure we should actually ship this with the gem, Rails 3.0.6 probably won't take that long and will contain that patch as well.)

We'll now need to test this with existing applications. And it would be great to get feedback from you guys if this fixes the performance issues.

@jdmorani
Copy link
Author

Thanks a lot sven for doing this so quickly. I'll test it tomorrow and I report back the performance result.
Thanks again!
JD

@svenfuchs
Copy link
Collaborator

JD, please note that testing this probably currently requires Rails 3 master (or svenfuchs/rails@afe4495 respectively)

@headius
Copy link

headius commented Feb 28, 2011

The expensive part is generating the backtrace, which according to Ruby semantics happens when you call "raise". I was hoping it would be possible, as in Java, to cache a single exception object and reuse it, but then remembered the "raise" behavior.

It may be worthwhile for us to talk to Matz about adding a way to generate exceptions that explicitly do not have backtraces. This is also possible in Java, by overriding a method called "fillInStackTrace" on Throwable subclasses. A similar simple mechanism in Ruby would allow users to reduce the cost of exceptions without requiring a special new method to be available (e.g. "raise_fast" or something). I'll ask around about that.

@svenfuchs
Copy link
Collaborator

Charles, thanks for the feedback.

Good to hear that raise is the expensive part. (Seems obvious now that I think about it, the backtrace must refer to the raise line, not the one where the exception is created.) That reduces the amount of changes for us.

It seems to me that this: https://gist.github.com/848290 should work on all Ruby implementations? I've only quickly tested 1.8.7 and 1.9.2 but will look into others as well.

If that works then we could easily switch to using the exception internally (throwing/catching it) and only raise it to the user in case that's needed for backwards compatibility.

@jdmorani
Copy link
Author

jdmorani commented Mar 1, 2011

I confirm that the reduce-exceptions branch + rails master (from your fork) fixes the problem.
Here is the result :

Rendered config/systems/Financial/views/cases/_address_fields.html.haml (51.0ms)
Rendered config/systems/Financial/views/cases/_consumer_fields.html.haml (141.0ms)
Rendered config/systems/Financial/views/cases/_address_fields.html.haml (46.0ms)
Rendered config/systems/Financial/views/cases/_consumer_fields.html.haml (47.0ms)
Rendered config/systems/Financial/views/cases/_applicants_infos.html.haml (275.0ms)

BEFORE the change it was :

Rendered config/systems/Financial/views/cases/_address_fields.html.haml (467.0ms)
Rendered config/systems/Financial/views/cases/_consumer_fields.html.haml (1045.0ms)
Rendered config/systems/Financial/views/cases/_address_fields.html.haml (439.0ms)
Rendered config/systems/Financial/views/cases/_consumer_fields.html.haml (506.0ms)
Rendered config/systems/Financial/views/cases/_applicants_infos.html.haml (2054.0ms)

Thanks a lot for the quick turnaround! Hope the fix can be part of Rails 3.0.6

@headius
Copy link

headius commented Mar 1, 2011

Sven: Oh yes, that should work fine. I forgot that throw can take an optional second argument to be the return value of the catch. That's a good pattern, and much cleaner than throwing exceptions for the same purpose.

I did also discover another way around backtrace generation: the three-arg form of "raise":

raise SomeException, some_arg, nil

Here some_arg is a single argument passed to SomeException's constructor, and nil is used in place of a generated backtrace. Under JRuby 1.6.0.RC3 and higher, we will short-circuit the internal backtrace generation logic in this case, which reduces the overhead almost to catch/throw. Obviously that would still require logic changes in i18n (to generate a trace in the cases where you still need it, like ex.set_backtrace(caller)), so your work wasn't wasted...but I will include it in an upcoming blog post about exceptions as flow control and the perils and workarounds.

@svenfuchs
Copy link
Collaborator

Jean-Dominique,

so these changes have now been released in i18n-0.6.0beta1 and the current rails master (which will be released as 3.1 soon) has been bumped to use this version.

If you could possibly try this out in your application and see how it goes that would be awesome.

Thanks for all the input, guys!

@jdmorani
Copy link
Author

Great, thanks a lot sven. I'm going to try it out and will let you know the outcome.

@svenfuchs
Copy link
Collaborator

Any results, yet, Jean-Dominique? :)

@svenfuchs
Copy link
Collaborator

So, these changes are now released in 0.6.0 and I'll close this issue.

Again, thanks to everybody for your patience and all the help :D

@jdmorani
Copy link
Author

My apology for taking so long to answer, I know this has been already released but just wanted to let you know that I've re-tested 0.6.0 and everything works fine for me.

Thanks again!
JD

@headius
Copy link

headius commented May 25, 2011

Excellent! Thank you for letting me know!

  • Charlie (mobile)

On May 25, 2011, at 12:16, jdmoranireply@reply.github.com wrote:

My apology for taking so long to answer, I know this has been already released but just wanted to let you know that I've re-tested 0.6.0 and everything works fine for me.

Thanks again!
JD

Reply to this email directly or view it on GitHub:
#82 (comment)

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

No branches or pull requests

3 participants