Skip to content

Commit

Permalink
remove some typeasserts that were causing severe performance issues
Browse files Browse the repository at this point in the history
  • Loading branch information
@vtjnash
vtjnash committed Apr 28, 2014
1 parent 69d7931 commit 024bbce
Show file tree
Hide file tree
Showing 2 changed files with 10 additions and 7 deletions.
12 changes: 6 additions & 6 deletions base/printf.jl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -242,11 +242,11 @@ function gen_d(flags::ASCIIString, width::Int, precision::Int, c::Char)
else
fn = :decode_dec
end
push!(blk.args, :((do_out, args) = $fn(out, $x, $flags, $width, $precision, $c)::(Bool,Tuple)))
push!(blk.args, :((do_out, args) = $fn(out, $x, $flags, $width, $precision, $c)))
ifblk = Expr(:if, :do_out, Expr(:block))
push!(blk.args, ifblk)
blk = ifblk.args[2]
push!(blk.args, :((len, pt, neg) = args::(Int32,Int32,Bool)))
push!(blk.args, :((len, pt, neg) = args))
# calculate padding
width -= length(prefix)
space_pad = width > max(1,precision) && '-' in flags ||
Expand Down Expand Up @@ -309,11 +309,11 @@ function gen_f(flags::ASCIIString, width::Int, precision::Int, c::Char)
x, ex, blk = special_handler(flags,width)
# interpret the number
if precision < 0; precision = 6; end
push!(blk.args, :((do_out, args) = fix_dec(out, $x, $flags, $width, $precision, $c)::(Bool,Tuple)))
push!(blk.args, :((do_out, args) = fix_dec(out, $x, $flags, $width, $precision, $c)))
ifblk = Expr(:if, :do_out, Expr(:block))
push!(blk.args, ifblk)
blk = ifblk.args[2]
push!(blk.args, :((len, pt, neg) = args::(Int32,Int32,Bool)))
push!(blk.args, :((len, pt, neg) = args))
# calculate padding
padding = nothing
if precision > 0 || '#' in flags
Expand Down Expand Up @@ -373,11 +373,11 @@ function gen_e(flags::ASCIIString, width::Int, precision::Int, c::Char)
# interpret the number
if precision < 0; precision = 6; end
ndigits = min(precision+1,BUFLEN-1)
push!(blk.args, :((do_out, args) = ini_dec(out,$x,$ndigits, $flags, $width, $precision, $c)::(Bool,Tuple)))
push!(blk.args, :((do_out, args) = ini_dec(out,$x,$ndigits, $flags, $width, $precision, $c)))
ifblk = Expr(:if, :do_out, Expr(:block))
push!(blk.args, ifblk)
blk = ifblk.args[2]
push!(blk.args, :((len, pt, neg) = args::(Int32,Int32,Bool)))
push!(blk.args, :((len, pt, neg) = args))
push!(blk.args, :(exp = pt-1))
expmark = c=='E' ? "E" : "e"
if precision==0 && '#' in flags
Expand Down
5 changes: 4 additions & 1 deletion test/perf/micro/perf.jl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -138,10 +138,13 @@ end

@unix_only begin
function printfd(n)
open("/dev/null","w") do io
io = open("/dev/null","w")
try
for i = 1:n
@printf(io,"%d %d\n",i,i+1)
end
finally
close(io)
end
end

Expand Down

8 comments on commit 024bbce

@vtjnash
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JeffBezanson this commit has the following effects on printfd timing:
~30 (baseline number, from 0.2 / 0.3)
~120 (prior commit)
~40 (prior commit, but with patch to perf.jl)
~20 (this commit, with this patch)
~26 (0.3 with only patch to perf.jl)
~80 (with this patch, but without patch to perf.jl)

I think inference needs to be taught to drop unnecessary typeasserts so that it can do better remove_redundant_temp_vars, SSA reduction, and/or tuple allocation.

@JeffBezanson
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like typeassert should be added to the pure builtins list. Yes, it should also have a custom inliner. Kind of surprised we didn't do that already.
It's debatable whether the change to perf.jl is legit --- we should make both versions fast.

@vtjnash
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this slowdown is because I declare out::IO in the function call, so it can't inline it because it can't verify that io is an IO. adding a typeassert in the perf.jl change also fixes the performance issue

i would make the first version fast by adding a inference transformation that converted into the second version (by inlining the open function, adding an inlining pass to remove single use closures, and rerunning type-inference after inlining a function with a parameter of type Function).

so I it depends on what you want this function to test. i would suggest splitting it into two, if we don't already have tests for the performance of anonymous functions

@vtjnash
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

however, i think this would be fixed by codegen-ing a typeassert at this line, instead of returning NF:

julia/base/inference.jl

Line 2012 in c8c7aae

return NF

@vtjnash
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is typeassert actually pure? it has ramifications for control flow, which make it hard to rearrange safely

@JeffBezanson
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All it might do is throw an error, but other functions in the pure builtins list do too.

@vtjnash
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, i'm working a pull request now

@JeffBezanson
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Eventually we'll also need better codegen of typeasserts with tuple types. I think we punt on those right now.

Please sign in to comment.