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

Use widen(Int128)=BitIntgers.Int256 within FixedDecimals #7

Open
wants to merge 5 commits into
base: rai-manual_mul_invcoeff
Choose a base branch
from

Conversation

ghost
Copy link

@ghost ghost commented Dec 19, 2018

Re-opening #6 for the new branch, rai-manual_mul_invcoeff, which is being used for the PR to JuliaMath/FixedPointDecimals.jl.


Remove allocations for FD{Int128} by switching to widen to Int256 instead of BigInt.

Adds a dependency on the BitIntegers package, but the performance gains seem worth it to me.

@NHDaly
Copy link
Member

NHDaly commented Dec 19, 2018

Hmm, i can't figure out how to add you as reviewers, @TotalVerb or @omus. I opened it against the same branch as in JuliaMath#45 (rai-manual_mul_invcoeff), because i wanted the diffbase to be easier to read, but i didn't mean for it to stay within the RelationalAI-oss fork... Github, man. SO anyway, if you wouldn't mind leaving your Review here as if it's a JuliaMath PR, that'd be much appreciated! Thanks!

@NHDaly
Copy link
Member

NHDaly commented Dec 19, 2018

And here's the multiplication timings:

  time (ns) allocs
FD{ Int32,2} 1.95 0
FD{ Int64,2} 8.43 0
FD{Int128,2} 72.20 0

You can see that the performance for Int128 is drastically improved, because it never has to widen into a BigInt! :)

Copy link

@TotalVerb TotalVerb left a comment

Choose a reason for hiding this comment

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

I have a few comments on the base branch also, but I'll leave that on the JuliaMath PR.

_widen(::Type{Int128}) = BitIntegers.Int256
_widen(::Type{UInt128}) = BitIntegers.UInt256

_widemul(a,b) = _widen(a)*_widen(b)

Choose a reason for hiding this comment

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

Base's widemul actually has these two special cases:

widemul(x::Signed,y::Unsigned) = widen(x) * signed(widen(y))
widemul(x::Unsigned,y::Signed) = signed(widen(x)) * widen(y)

I believe we should replicate these here.

Copy link
Member

Choose a reason for hiding this comment

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

huh weird, i wonder why those are needed...? But yeah okay sounds good to me!

Copy link
Member

Choose a reason for hiding this comment

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

update: it turns out this is because apparently widemul(::Signed, ::Unsigned) returns a Signed, whereas promote would return Unsigned.

I've added your suggestion into the new take on this PR, here: JuliaMath#93


# BitIntegers is missing iseven/isodd
# Prevent expensive calculation for Int256. Needed since `_round_to_even` calls `iseven`.
Base.isodd(a::BitIntegers.Int256) = Base.isodd(a % Int) # only depends on the final bit! :)

Choose a reason for hiding this comment

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

These are monkey patching functionality that belongs in a different package (neither BitIntegers nor Base.isodd are "owned" by this package). Sometimes, this can lead to method overwriting warnings when the downstream packages are updated (although I suspect it won't in this case, but I'd prefer to avoid it overall). Would it be reasonable to simply wait for BitIntegers to merge the appropriate PRs?

Copy link
Member

Choose a reason for hiding this comment

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

:) Yeah i think that's reasonable! I agree.
I'm linking those two PRs here for posterity:
rfourquet/BitIntegers.jl#3
rfourquet/BitIntegers.jl#2

The unsigned(::Type{T}) one, rfourquet/BitIntegers.jl#2, might take a while because I think it's waiting for some progress on JuliaLang/julia#30444.

@NHDaly
Copy link
Member

NHDaly commented Dec 20, 2018

Thanks for taking a look here, @TotalVerb! I'm happy to wait for progress on JuliaMath#45. :)

@ghost ghost force-pushed the rai-manual_mul_invcoeff-Int256 branch from 3672ac6 to 65fdac2 Compare February 12, 2019 16:17
@ghost ghost force-pushed the rai-manual_mul_invcoeff-Int256 branch from 65fdac2 to f2830d3 Compare February 12, 2019 16:18
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