Skip to content

Conversation

@dwbutler
Copy link

Hi,

We noticed some performance issues when working with large numbers of integers. We tracked the issue down to the #method_missing implementation defined on the Numeric class in this gem.

This patch replaces the #method_missing implementation by defining the constructors directly on Numeric.

On a side note, many of the specs are failing even on a clean master branch. Perhaps they need to be brought up to date?

Thanks!

@dwbutler
Copy link
Author

dwbutler commented Jul 9, 2013

To be more specific, the performance issue started when we upgraded our app from Rails 2.3 / Ruby 1.8.7 to Rails 3.2 / Ruby 1.9.3. Apparently something in the internals is calling #method_missing on Fixnum a lot more than before.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why was the gemspec removed?

@dwbutler
Copy link
Author

dwbutler commented Jul 9, 2013

Bundler got confused by having two different gemspec files. So I removed .gemspec and left quantity.gemspec. It looked like quantity.gemspec had been modified more recently.

@bhuga
Copy link
Owner

bhuga commented Jul 9, 2013

So this is an old gem, and, it turns out, a busted one:

https://travis-ci.org/bhuga/quantity/builds/8897313

I really have no idea what the past status was--it's been a year--but it's hard for me to pull in changes when they don't pass. You can merge bhuga/stravis into your branch to get the changes that make travis work. If you're willing to fix that, or figure out which ones deserve to be pending and set them as such, I'll merge and cut a release.

@dwbutler
Copy link
Author

Alright, I'll give it a try.

@dwbutler
Copy link
Author

I took a look at the specs that are failing. They fall out of the scope of what I use the gem for, so I'm not clear which failures are bugs in the gem, and which are specs that need to be updated. I think I'll start a new pull request to fix the specs, and keep that discussion over there.

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.

2 participants