Reimplement show #60

jmkuhn · 2018-02-14T18:33:43Z

Reimplement show without using @sprintf. The motivation for doing this is to show values without rounding.

julia> maxintfloat(Dec128) - 1
9.999999999999999999999999999999999e33

A side effect of the reimplementation is some recently reported display issues also go away

julia> realmin(d64"3.4")
1.0e-398

julia> realmin(Dec64)
1.0e-398

julia> parse(Dec128, "56123")
56123.0

julia> parse(Dec128, "561234")
561234.0

Please keep issue #59 open since I still need to address this in @sprintf

cc @ScottPJones

jmkuhn · 2018-02-14T18:43:44Z

Windows build failures unrelated to this PR.

stevengj · 2018-02-14T19:13:01Z

src/DecFP.jl

+                        unsafe_write(io, pointer(_buffer, 2), lastnonzeroindex - 1)
+                        write(io, '0'^(normalized_exponent - lastnonzeroindex + 2), ".0")
+                    elseif normalized_exponent == lastnonzeroindex - 2
+                        unsafe_write(io, pointer(_buffer, 2), lastnonzeroindex - 1)


You should wrap all of the code that does unsafe_write(..., pointer(_buffer, n)) in GC.@preserve _buffer .... in Julia 0.7. (In Julia 0.6 it is Base.@gc_preserve; unfortunately this is not in Compat yet).

GC.@preserve was added with JuliaLang/julia#25616. Base.@gc_preserve was added with JuliaLang/julia#23610 in 0.7.0-DEV.1795. Is there an equivalent for 0.6?

I think it can be a no-op in 0.6.

The need to be more cautious about pointer apparently occurred because the gc became more aggressive in 0.7 (JuliaLang/julia#23562), as I understand it.

Good point - I think I've got to look at a lot of my code in Strs.jl for just that issue, because of the more aggressive GC in master.

stevengj · 2018-02-14T19:15:37Z

src/DecFP.jl

+                    if normalized_exponent == -1
+                        write(io, "0.")
+                    else
+                        write(io, "0.", '0'^(-normalized_exponent - 1))


Note that this allocates a temporary string. Might be better to define a writezeros(io, n) = for i=1:n; write(io, UInt8('0')); end function

That might be a bit slow, even some small tables would help a lot, say strings of 0s, 1:9, then strings of 0's, 10:10:90,
then 100:100:1000. The exponent for Dec128 can be -6143..6144, so at most you'd need to write out the 1000 "0" string 6 times, the length 100 "0" once, a 40 "0" once, and a 4 "0" string, but all with no allocation (with a table size of > 6105, but it could be used by other numeric output packages)

stevengj · 2018-02-14T19:17:48Z

src/DecFP.jl

+                r = normalized_exponent
+                while b > 0
+                    q, r = divrem(r, b)
+                    write(io, Char('0' + q))


Why is the call to Char needed? '0' + q should already give a Char, no? Maybe more efficient to do UInt8('0')+(q%UInt8)

Yes, Steven is correct. Converting what is an ASCII character to Char, and then have to have that checked to see how many bytes it might be, and deal with the new representation of Char, would be inefficient. I'd just put write(io, q%UInt8 + 0x30)

write(io, 0x30 + q%UInt8) would also work just fine.

(UInt8('0') will get inlined as 0x30 and is more readable to people who don't have ASCII memorized 😉.)

As long as it still gets in-lined as 0x30, I was afraid with the changes to Char, it might not on master.
Note that if you have UInt8('0' + q) instead of UInt8('0') + UInt8(q), it generates positively horrible code since #24999.

On v0.6.2:

julia> f(q) = ('0' + q)%UInt8 f (generic function with 1 method) julia> @code_native f(0x9) .section __TEXT,__text,regular,pure_instructions Filename: REPL[4] pushq %rbp movq %rsp, %rbp Source line: 4 addb $48, %dil Source line: 1 movl %edi, %eax popq %rbp retq

On v0.7 (built today) (much too long to paste here!):
https://gist.github.com/ScottPJones/8531146104d22a3a46b6777e90968b42

ScottPJones · 2018-02-15T02:18:05Z

src/DecFP.jl

@@ -105,6 +97,83 @@ for w in (32,64,128)

        $BID(x::AbstractString) = parse($BID, x)

+        function Base.show(io::IO, x::$BID)
+            if isnan(x)


You might consider this stylistic, but this code is so long. What do you think of shortening it up a bit, i.e.:

isnan(x) && return write(io, "NaN") isinf(x) && return write(io, signbit(x) ? "-Inf" : "Inf") x == 0 && return write(io, signbit(x) ? "-0.0" : "0.0") ccall(($(bidsym(w,"to_string")), libbid), Cvoid, (Ptr{UInt8}, $BID), _buffer, x) _buffer[1] == '-'%UInt8 && write(io, '-'%UInt8)

5 lines instead of 24, and IMO easier to read.

I can appreciate the shorter code but your use of return has me considering something that I had overlooked before. Even in my original PR, show sometimes returns nothing and sometimes returns an Int with no consistent meaning due to a lack of return at the end of the function. Should I add an explicit return while keeping my other returns as is to ensure show always returns nothing?

So, instead of && return write(io, ..., you could have && (write(io, ...); return) instead, to make sure it's always returning nothing

ScottPJones · 2018-02-15T02:35:16Z

src/DecFP.jl

+                    if normalized_exponent == -1
+                        write(io, "0.")
+                    else
+                        write(io, "0.", '0'^(-normalized_exponent - 1))


That might be a bit slow, even some small tables would help a lot, say strings of 0s, 1:9, then strings of 0's, 10:10:90,
then 100:100:1000. The exponent for Dec128 can be -6143..6144, so at most you'd need to write out the 1000 "0" string 6 times, the length 100 "0" once, a 40 "0" once, and a 4 "0" string, but all with no allocation (with a table size of > 6105, but it could be used by other numeric output packages)

ScottPJones · 2018-02-15T02:37:55Z

src/DecFP.jl

+                    normalized_exponent = -normalized_exponent
+                end
+                b_lb = div(normalized_exponent, 10)
+                b = 1


Shouldn't this use a table, or in some way do fewer multiplications?

ScottPJones · 2018-02-15T02:41:31Z

src/DecFP.jl

+                    b *= 10
+                end
+                r = normalized_exponent
+                while b > 0


This whole loop concerns me, it seems to be doing quite a lot of divisions for each digit output.
Can't you start outputting from the end, and then divide by 10, and get the remainder, every time through the loop.

ScottPJones · 2018-02-15T02:44:13Z

src/DecFP.jl

+                # %f
+                if normalized_exponent >= 0
+                    if normalized_exponent > lastnonzeroindex - 2
+                        unsafe_write(io, pointer(_buffer, 2), lastnonzeroindex - 1)


Why is this always starting at 2? If it's positive, does the library "to_string" function output a +?

ScottPJones · 2018-02-15T02:45:03Z

src/DecFP.jl

+            normalized_exponent = nox(ccall(($(bidsym(w,"ilogb")), libbid), Cint, ($BID,), x))
+            lastdigitindex = Compat.findfirst(equalto(UInt8('E')), _buffer) - 1
+            lastnonzeroindex = Compat.findlast(!equalto(UInt8('0')), view(_buffer, 1:lastdigitindex))
+            if -5 < normalized_exponent < 6


Why not just set a variable here for the pointer to _buffer, i.e. pnt = pointer(buffer)?
pointer(_buffer, 2) would become pnt + 1.

ScottPJones · 2018-02-15T02:46:22Z

src/DecFP.jl

+                        unsafe_write(io, pointer(_buffer, 2), lastnonzeroindex - 1)
+                        write(io, '0'^(normalized_exponent - lastnonzeroindex + 2), ".0")
+                    elseif normalized_exponent == lastnonzeroindex - 2
+                        unsafe_write(io, pointer(_buffer, 2), lastnonzeroindex - 1)


Good point - I think I've got to look at a lot of my code in Strs.jl for just that issue, because of the more aggressive GC in master.

ScottPJones · 2018-02-15T02:49:53Z

src/DecFP.jl

+                r = normalized_exponent
+                while b > 0
+                    q, r = divrem(r, b)
+                    write(io, Char('0' + q))


write(io, 0x30 + q%UInt8) would also work just fine.

ScottPJones · 2018-02-15T03:19:25Z

Don't mind the detailed review - this is very good that you are doing this! 👍💯

stevengj · 2018-02-16T16:10:45Z

Unrelated CI errors.

Reimplement show

59f64e9

stevengj reviewed Feb 14, 2018

View reviewed changes

ScottPJones reviewed Feb 15, 2018

View reviewed changes

Review updates

d12a43c

stevengj merged commit 135efec into JuliaMath:master Feb 16, 2018

jmkuhn deleted the show branch February 16, 2018 16:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reimplement show #60

Reimplement show #60

jmkuhn commented Feb 14, 2018

jmkuhn commented Feb 14, 2018

stevengj Feb 14, 2018

jmkuhn Feb 14, 2018

stevengj Feb 14, 2018 •

edited

Loading

ScottPJones Feb 15, 2018

stevengj Feb 14, 2018 •

edited

Loading

ScottPJones Feb 15, 2018

stevengj Feb 14, 2018

ScottPJones Feb 15, 2018

ScottPJones Feb 15, 2018

stevengj Feb 15, 2018

ScottPJones Feb 15, 2018

ScottPJones Feb 15, 2018

jmkuhn Feb 15, 2018

ScottPJones Feb 15, 2018

ScottPJones Feb 15, 2018

ScottPJones Feb 15, 2018

ScottPJones Feb 15, 2018

ScottPJones Feb 15, 2018

ScottPJones Feb 15, 2018

ScottPJones Feb 15, 2018

ScottPJones Feb 15, 2018

ScottPJones commented Feb 15, 2018

stevengj commented Feb 16, 2018

Reimplement show #60

Reimplement show #60

Conversation

jmkuhn commented Feb 14, 2018

jmkuhn commented Feb 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevengj Feb 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevengj Feb 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ScottPJones commented Feb 15, 2018

stevengj commented Feb 16, 2018

stevengj Feb 14, 2018 •

edited

Loading

stevengj Feb 14, 2018 •

edited

Loading