mpfr: speed improvements #27331
mpfr: speed improvements #27331
Conversation
|
Does this help with serializing and deserializing BigFloats? |
| @@ -55,18 +56,29 @@ mutable struct BigFloat <: AbstractFloat | |||
| sign::Cint | |||
| exp::Clong | |||
| d::Ptr{Limb} | |||
| # _d::Buffer{Limb} # Julia gc handle for memory @ d | |||
| _d::String # Julia gc handle for memory @ d (optimized) | |||
There was a problem hiding this comment.
Mutating immutable memory is dangerous. If you must have this add a FixedSizeBuffer type.
There was a problem hiding this comment.
It's not that dangerous, it's only bad if this pointer gets leaked anywhere else, but it's purely internal. I mean, it's not like there's a deepcopy method definition here that assumes they can't be mutated or anything like that. No, that would be crazy talk, and definitely not a bug in this very PR. :P
There was a problem hiding this comment.
Actually, is it a problem though? since BigFloat itself is not supposed to be mutated either, they share the same danger if someone violated that expectation
There was a problem hiding this comment.
Immutable memory is still being overwritten (since you construct it and then overwrite it with the result of an operation). I agree that it's much less likely to be harmful, since the compiler shouldn't be able to know the objectid of a newly-allocated bigfloat that gets subsequently modified (as long as the BigFloat itself isn't mutated more than once), but it does sound fairly risky.
There was a problem hiding this comment.
Isn't that just the same concern as any other usage of String?
This would have caused us to perform a number of unncessary allocations and method specializations
Unlike GMP / BigInt, the MPFR library exposes an interface for external allocation: http://www.mpfr.org/mpfr-current/mpfr.html#Custom-Interface Using that allows us to use our fast memory-pool gc and avoid adding finalizers and use the slow malloc/free functions. (and in some cases, some of it might even end up on the stack!) Also switch to allocating lgamma_sign on the stack, for better performance (and thread-safety) Also fixes Serialization to preserve BigFloat precision.
Forgot to add that to the initial commit – now it does :) |
vtjnash
dismissed
Keno’s stale review
Keno agreed this should be no worse than the current existence of _string_n in other code.
|
@nanosoldier |
|
Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @ararslan |
|
Nice :) |
|
So, about 25% faster. (The allocation difference is just previous measurement error) |
An understatement! That's fantastic! 🎉 |
|
It was lucky that it had memory regressions or we wouldn't have seen the speed improvements because the threshold is put to 50% (e.g. needs to become twice as fast for it to be considered insignificant). Feels kinda high... |
|
FYI this PR is probably the cause of a SEGFAULT in |
Unlike GMP / BigInt, the MPFR / BigFloat library exposes an interface for external allocation:
http://www.mpfr.org/mpfr-current/mpfr.html#Custom-Interface
Using that allows us to use our fast memory-pool gc, and avoid adding finalizers and use the slow malloc/free functions, and allows us to better track memory pressure.
(
@profilenow indicates that this is now about equal parts mpfr time and allocation-related time)