-
Notifications
You must be signed in to change notification settings - Fork 113
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
Fix performance problems with interpolation #37
Conversation
Whoa! I am, as always, immensely impressed by your ability to sqeeze even more out of Julia. This is really beginning to shape up to something like what we (you) envisioned when you started talking about the metaprogramming approach in the issue on Grid.jl. As I mentioned in the other comment I unfortunately don't have much time to help you out at the moment, but I trust you to write Good Stuff, so feel free to merge at will while I'm "away" :) |
@@ -0,0 +1,31 @@ | |||
# Base's Rational type is too slow because of its penchant for calling gcd and div. | |||
# So we roll our own. |
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.
Without having perused the code carefully, this seems like something we shouldn't have to do; if for no other reason, then because there is probably also other packages and code bases that could benefit from these speed improvements.
In the long term, maybe we should split these out and incorporate them either in Base or in their own package? (I imagine these do a subset of the stuff that Base.Rational
do - maybe we could re-write Rational
s to be a two-step rocket, exposing these too? Loong-term goal...)
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.
Presumably we can't make Rational
itself behave this way, because if you don't call gcd
and div
, then overflow seems likely. But you're right that perhaps we could use a layered approach.
CCing @StefanKarpinski, @simonbyrne, and @JeffBezanson. Executive summary: Interpolations is the successor to Grid, with an aim to improve the API and performance. At the top of this issue I linked to a performance problem that stemmed from using Rational
, where I got a 50x speed boost by rolling my own stripped-down version (called Ratio
). We're attracted to use something like Rational
for coefficients like 1//2
, rather than floating-point numbers like 0.5
, because it works out better in terms of promotion behavior for different Number
types. I don't immediately see a good way of designing Rational
to get both behaviors, but wondering if you folks have any thoughts about this.
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.
It would be worth opening an issue on the main julia repo to track this issue – calling gcd and lcm all the time is pretty awful for performance. There's also the chronic overflow issue that Rational has, which seems like something that would need to be address to use the for any kind of real computation.
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.
I too would like to see a faster Rational type, though overflow should have been fixed since JuliaLang/julia#8672.
This gets rid of all the allocations, and boosts performance several-fold
Fix performance problems with interpolation
This is built on top of #36; only the last commit is new.
OK, the good news is that the discouraging behavior of #36 (comment) has largely been fixed:
The core problem was that Base's
Rational
type is no speed demon, because it callsgcd
anddiv
repeatedly. So I added a simpler type.The only remaining puzzle is the memory allocation with
itp
. It goes away if I change this line toone(typeof(ret))
. However,Base.return_types
assures me thatgetindex
is type-stable, and@code_warntype
did not turn up any oddities, so I am very puzzled.