-
Notifications
You must be signed in to change notification settings - Fork 21
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
Prevent allocations for FD{Int128} by freezing max_exp10(Int128) #41
Conversation
Before this commit, reinterpret(FD{Int128,f}, x) allocates because: 1. max_exp10(Int128) widens the Int128 to a BigInt, which causes allocations. 2. The result of max_exp10(Int128) isn't getting const-folded away, so the above widen operation occurs every time it's called at runtime. This commit simply "freezes" the result for Int128s via a top-level `@eval` statement.
Actually, another solution that only just occurred to me as I was writing the above explanation, is that we can just mark julia> @code_typed reinterpret(FixedPointDecimals.FD{Int128,2}, 200)
>> CodeInfo(
94 1 ─ goto #3 if not false │
2 ─ nothing::Nothing │
3 ─ nothing::Nothing │
95 │ %4 = (Base.sext_int)(Int128, i)::Int128 │╻ rem
│ %5 = %new(FixedDecimal{Int128,2}, %4)::FixedDecimal{Int128,2} │
└── return %5 │
) => FixedDecimal{Int128,2}
julia> @code_typed reinterpret(FixedPointDecimals.FD{Int64,2}, 200)
>> CodeInfo(
94 1 ─ goto #3 if not false │
2 ─ nothing::Nothing │
3 ─ nothing::Nothing │
95 │ %4 = %new(FixedDecimal{Int64,2}, i)::FixedDecimal{Int64,2} │
└── return %4 │
) => FixedDecimal{Int64,2} Here's a commit where I did that solution, and got this result: 341c931 Maybe that's a better solution? But I know that I've heard from people in that past that we should be wary of using @JeffBezanson @vtjnash: This is another example of what we talked about last week. Do you have a preference for either of these two solutions in this case? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This LGTM. The @pure
solution is interesting but I'm not sure myself on when it is safe and correct first.
Codecov Report
@@ Coverage Diff @@
## master #41 +/- ##
==========================================
+ Coverage 98.82% 98.83% +0.01%
==========================================
Files 1 1
Lines 170 172 +2
==========================================
+ Hits 168 170 +2
Misses 2 2
Continue to review full report at Codecov.
|
Yeah makes sense. I'm not really sure either. That said, I do see that there are lots of But yeah, I think this PR is good to merge as-is if you're alright with it! :) Thanks! |
This looks good to me, so I'll merge it. I've invited you to collaborate on this repo; let me know if you did not get the invite link. |
:) Thanks!
Ah, sorry!! I really appreciate that. I've just accepted it now. I'm sorry I missed it -- thanks for the reminder! |
This is a follow-up to https://github.com/JuliaMath/FixedPointDecimals.jl/pull. It seems that removing
applicable
wasn't sufficient for the case whereT=Int128
.Before this PR,
reinterpret(FD{Int128,f}, x)
allocates because:max_exp10(Int128)
widens theInt128
to aBigInt
, which causes allocations.max_exp10(Int128)
isn't getting const-folded away, so the above widen operation occurs every time it's called at runtime.I'm not entirely sure why
max_exp10(Int128)
isn't const-folding, but here's my guess:Because
max_exp10(::Type{Int128})
ends up more complicated (because it allocates, etc), I think it ends up not being inlined, where for other int types it is inlined. And so then, because it's not inlined, LLVM isn't able to determine that its input is a static constant, so it doesn't know it can eliminate the function entirely.Here we can see it not being inlined:
And so then LLVM is able to const fold everything away, even though julia wasn't able to:
(I'm not including the
@code_native
for Int128, because it's very long.)