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

Remove ruby inline and c function call #24

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

milus
Copy link

@milus milus commented Dec 22, 2021

Use pure ruby instead of Inline feature. Since Ruby math functions are already implemented in C it does not impact on performance.
In C extention BigDecmial is casted to double. Ruby's Float internally is also implemented as double, so casting BigDecimal to Float in ruby has the same effect without compromising on performance

Benchmark:

require 'date'
require 'benchmark'
require 'xirr'

def test_xirr
  cf = Xirr::Cashflow.new
  cf << Xirr::Transaction.new(-1000,  date: Date.parse('2014-01-01'))
  cf << Xirr::Transaction.new(-2000,  date: Date.parse('2014-03-01'))
  cf << Xirr::Transaction.new( 4500, date: Date.parse('2015-12-01'))
  cf.xirr
end

n = ENV.fetch('N', 1_000).to_i

module Xirr
  module Base
    def xnpv(rate)
      cf.inject(0) do |sum, t|
        sum + xnpv_impl(rate, t.amount, periods_from_start(t.date))
      end
    end
  end
end

Benchmark.bm do |x|
  x.report('Inline implementation [double]') do
    module Xirr
      module Base
        def xnpv_impl(*args)
          xnpv_c(*args)
        end
      end
    end

    n.times { test_xirr }
  end

  x.report('Ruby implementation [Float]') do
    module Xirr
      module Base
        def xnpv_impl(rate, amount, period)
          amount / (1+rate.to_f) ** period
        end
      end
    end

    n.times { test_xirr }
  end

  x.report('Ruby implementation [BigDecimal]') do
    module Xirr
      module Base
        def xnpv_impl(rate, amount, period)
          amount / (1+rate) ** period
        end
      end
    end

    n.times { test_xirr }
  end
end

Benchmark result:

# $ N=10000 ruby -Ilib benchmark.rb
#        user     system      total        real
# Inline implementation [double]  1.187576   0.001224   1.188800 (  1.189479)
# Ruby implementation [Float]  1.171843   0.001200   1.173043 (  1.173044)
# Ruby implementation [BigDecimal] 23.502473   0.019631  23.522104 ( 23.524580)

@milus milus changed the title Remove ruby inline and c function cal Remove ruby inline and c function call Dec 27, 2021
@tubedude
Copy link
Owner

Should we remove BigDecimal entirely as well?

tubedude added a commit that referenced this pull request Feb 25, 2024
sk- added a commit to sk-/xirr that referenced this pull request Apr 30, 2024
For some reason the commit added (tubedude@0c89d72) to remove RubyInline is not the same as PR tubedude#24, and a require was left in the code.

That require fails, as the dependency is no longer specified. And that makes the test to fail:

```
$ bundle exec rake test_units
/Users/coder/.rbenv/versions/3.3.1/lib/ruby/3.3.0/bundled_gems.rb:74:in `require': cannot load such file -- inline (LoadError)
	from /Users/coder/.rbenv/versions/3.3.1/lib/ruby/3.3.0/bundled_gems.rb:74:in `block (2 levels) in replace_require'
	from /Users/coder/code/xirr/lib/xirr/base.rb:7:in `<module:Base>'
	from /Users/coder/code/xirr/lib/xirr/base.rb:5:in `<module:Xirr>'
	from /Users/coder/code/xirr/lib/xirr/base.rb:3:in `<top (required)>'
	from /Users/coder/.rbenv/versions/3.3.1/lib/ruby/3.3.0/bundled_gems.rb:74:in `require'
	from /Users/coder/.rbenv/versions/3.3.1/lib/ruby/3.3.0/bundled_gems.rb:74:in `block (2 levels) in replace_require'
	from /Users/coder/code/xirr/test/test_helper.rb:12:in `<top (required)>'
	from /Users/coder/code/xirr/test/test_cashflow.rb:1:in `require_relative'
	from /Users/coder/code/xirr/test/test_cashflow.rb:1:in `<top (required)>'
	from /Users/coder/.rbenv/versions/3.3.1/lib/ruby/3.3.0/bundled_gems.rb:74:in `require'
	from /Users/coder/.rbenv/versions/3.3.1/lib/ruby/3.3.0/bundled_gems.rb:74:in `block (2 levels) in replace_require'
	from /Users/coder/.rbenv/versions/3.3.1/lib/ruby/gems/3.3.0/gems/rake-13.2.1/lib/rake/rake_test_loader.rb:21:in `block in <main>'
	from /Users/coder/.rbenv/versions/3.3.1/lib/ruby/gems/3.3.0/gems/rake-13.2.1/lib/rake/rake_test_loader.rb:6:in `select'
	from /Users/coder/.rbenv/versions/3.3.1/lib/ruby/gems/3.3.0/gems/rake-13.2.1/lib/rake/rake_test_loader.rb:6:in `<main>'
rake aborted!
Command failed with status (1)
/Users/coder/.rbenv/versions/3.3.1/bin/bundle:25:in `load'
/Users/coder/.rbenv/versions/3.3.1/bin/bundle:25:in `<main>'
Tasks: TOP => test_units
(See full trace by running task with --trace)
```

After the change, the tests are working again.

PS: in a later PR I will be configuring Github Actions for this repo.
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