Skip to content

Remove ruby inline and c function call #24

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.

Sign up for GitHub

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

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

Remove ruby inline and c function call #24

wants to merge 2 commits into 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
@tubedude
Removed inline #24
0c89d72
sk- added a commit to sk-/xirr that referenced this pull request Apr 30, 2024
@sk-
fix: remove require of RubyInline
2c8ef2c 
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.
@sk- sk- mentioned this pull request Apr 30, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@milus @tubedude