-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Switch float printing from grisu to ryu algorithm #32799
Conversation
Unsigned integers are printed differently by |
Yes, I should clarify that I was referring to |
@quinnj something in the transition is reaching out from Base and grabbing DoubleFloats. |
Ok, I've pushed some updates here, notably adding some enhancements to So to give an overall status update on everything going on here:
The remaining TODO item is switching over the usages of Grisu in |
This looks really nice.
|
937e8ae
to
ca2b3fd
Compare
Alright, I think this is good to go; the 32-bit and windows 64-bit CI failures look unrelated. As a quick update/recap:
julia> @btime Ryu.writeshortest(2.2250738585072014e-308)
61.852 ns (2 allocations: 496 bytes)
"2.2250738585072014e-308"
julia> @btime string(2.2250738585072014e-308)
299.455 ns (7 allocations: 352 bytes)
"2.2250738585072014e-308"
julia> @btime Ryu.writeshortest(1.0)
46.744 ns (2 allocations: 480 bytes)
"1.0"
julia> @btime string(1.0)
162.301 ns (4 allocations: 192 bytes)
"1.0"
julia> @btime Ryu.writeshortest(512.0e+3)
50.171 ns (2 allocations: 480 bytes)
"512000.0"
julia> @btime string(512.0e+3)
189.952 ns (4 allocations: 192 bytes)
"512000.0" Note that I didn't add a NEWS entry because I'm not sure where I'd put it; we seem to have a 1.3 section, but I think 1.3 is frozen? So this would be 1.4? Not sure. |
1.3 is not yet frozen, you've got a couple of days. Then again, I'm not sure if we want to merge this right before 1.3 feature freeze or very early in the 1.4 release cycle. |
Ah, good to know. I mean, speaking somewhat selfishly, I'd rather merge sooner than wait for the next release cycle, just because I've kind of got the momentum going here and can respond quickly to things that crop up over the release candidates. But if people feel like it's too risky, then I guess we can wait. I'm not currently planning on doing any more here or w/ Printf now that the tests are all passing (there's the one weird generate_precompile.jl thing that @KristofferC says should be fixed soon), but I am going to incorporate the code, probably into Parsers.jl that can use the code on older versions. |
Yeah, I'd say let's go for it and get lots of testing in the 1.3 release. |
base/ryu/Ryu.jl
Outdated
decchar::UInt8=UInt8('.')) where {T <: Base.IEEEFloat} | ||
buf = Vector{UInt8}(undef, neededdigits(T)) | ||
pos = writeshortest(buf, 1, x) | ||
return unsafe_string(pointer(buf), pos-1) |
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.
forcing an (unsafe) copy here seem unnecessary. above do _string_n(neededdigits(T))
to make the Vector, then use String(resize!(buf, pos - 1))
here
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.
What if we just add an @assert
that pos <= length(buf)
? It seems like that would actually ensure more safety (in the case pos
becomes something unsafe-ly large)
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.
Yes, but that would still be slow. I'm suggesting you don't make an extra copy of the data here by sticking with the safe API.
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.
Oh.......got it. Yeah, we can do that.
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.
Well, we actually want StringVector(n)
instead of _string_n
, right? The underlying functions all expect a Vector{UInt8}
.
i = len - 1 | ||
while i >= 0 | ||
j = p10bits - e2 | ||
#=@inbounds=# mula, mulb, mulc = POW10_SPLIT[POW10_OFFSET[idx + 1] + i + 1] |
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.
#=@inbounds=# mula, mulb, mulc = POW10_SPLIT[POW10_OFFSET[idx + 1] + i + 1] | |
mula, mulb, mulc = POW10_SPLIT[POW10_OFFSET[idx + 1] + i + 1] |
end | ||
if e >= 100 | ||
c = e % 10 | ||
unsafe_copyto!(buf, pos, DIGIT_TABLE, 2 * div(e, 10) + 1, 2) |
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.
unsafe_copyto!(buf, pos, DIGIT_TABLE, 2 * div(e, 10) + 1, 2) | |
copyto!(buf, pos, DIGIT_TABLE, 2 * div(e, 10) + 1, 2) |
base/ryu/shortest.jl
Outdated
exp_form = true | ||
pt = nexp + olength | ||
prec = precision | ||
if -4 < pt <= (precision == -1 ? 6 : precision) #&& !(pt >= olength && abs(mod(x + 0.05, 10^(pt - olength)) - 0.05) > 0.05) |
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.
if -4 < pt <= (precision == -1 ? 6 : precision) #&& !(pt >= olength && abs(mod(x + 0.05, 10^(pt - olength)) - 0.05) > 0.05) | |
if -4 < pt <= (precision == -1 ? 6 : precision) |
base/ryu/utils.jl
Outdated
const MANTISSA_MASK = 0x000fffffffffffff | ||
const EXP_MASK = 0x00000000000007ff | ||
|
||
memcpy(d, doff, s, soff, n) = ccall(:memcpy, Cvoid, (Ptr{UInt8}, Ptr{UInt8}, Int), d + doff - 1, s + soff - 1, n) |
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.
wrong method signature for memcpy and memmove—also I think this is already unsafe_copyto!
base/ryu/Ryu.jl
Outdated
end | ||
buf = Vector{UInt8}(undef, neededdigits(T)) | ||
pos = writeshortest(buf, 1, x) | ||
GC.@preserve buf unsafe_write(io, pointer(buf), pos - 1) |
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.
GC.@preserve buf unsafe_write(io, pointer(buf), pos - 1) | |
write(io, resize!(bu, pos - 1)) |
although it also seems like write(io, writeshortest(x))
would give the same result?
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.
write(io, writeshortest(x))
would make an extra copy of the buffer though (when creating the string)
@@ -0,0 +1,362 @@ | |||
@inline function writeshortest(buf::Vector{UInt8}, pos, x::T, |
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.
as the only function which uses unsafe
operations, should we add some bounds-checking for sanity?
I'm not really qualified to comment on the algorithm, so I just made a few comments on other stuff.
you could see how we handled BigFloat, which had the same problem, but I know you're already looking into 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.
seems to have always failed on linux32 CI?
On the 32-bit CI: both windows & linux 32-bit have seemed to be really flaky. The windows CI seems to often have some kind of error building julia dependencies, and the linux 32-bit times out during the "generate precompile statements" step. I could look further into the latter, but at first glance it didn't seem like the changes here would have caused any issues. |
I think you're making that up: I don't see any other time the linux32 bot run into an error like that in at least the past week, except every time it ran this PR. There were a couple configuration errors seen earlier this week so it would fail outside of the build, but those have gotten fixed. |
Sorry, I didn't mean they've been flaky in general, just on this PR; I inspected the 32-bit results a few times and the errors seems different enough from run to run that it didn't seem likely that anything here was a problem. I've since identified indeed a bug on 32-bit and pushed a fix; along with updates to address @vtjnash's review comments. |
I'll merge this in 12 hours or so unless there are other comments/concerns. |
not applicable: passes on 32-bit now
I really don't think I'm ok with this (sorry for being late to the party). For
|
|
||
julia> reinterpret(Float32, UInt32[1 2 3 4 5]) | ||
1×5 reinterpret(Float32, ::Array{UInt32,2}): | ||
1.4013e-45 2.8026e-45 4.2039e-45 5.60519e-45 7.00649e-45 |
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.
What happened here? These are not the same floating point value:
julia> 1.4013e-45 == 1.0e-45
false
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.
Ignore me, they are in Float32:
julia> 1.4013f-45 == 1.0f-45
true
I agree we should restore the type info in |
So in my mind, this is a key difference between |
|
@Keno, I understand the example you share of the difference between Frankly, I think julia> Float16(1)
Float16(1.0)
julia> Float32(1)
1.0f0
julia> Float64(1)
1.0 is inconsistent and ugly. While we're making changes to float printing code internals, I think it's a good time to try and address this and come up with something better. Maybe we introduce |
The immediate problem is this:
because show == repr. We could restore the type info for |
Even before this PR we have:
|
Yeah, the BigFloat printing should probably be fixed to print as |
What if we were to break numerical juxtaposition for just macros? We could then allow something like
to be parsed as
(and similar for |
I am a really big fan if the I would actually prefer to extend it, i.e. print signed integers like |
microsoft/STL#498 includes a fairly extensive test suite of Ryu functionality that would be good to incorporate. |
Switch float printing from grisu to ryu algorithm
Because it's faster and simpler.
TODO:
Signed
); personally, I'm not a huge fan of the current1.0
,1.0f0
,Float16(1.0)
inconsistency, so I'd rather just treat them like signed integers and print them all the same. If people think that's too heretical, I can look into keeping the same "typed" logicWill fix #14698 and #22137