Skip to content
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

regression in float to int performance on master #18954

Closed
JeffBezanson opened this issue Oct 15, 2016 · 5 comments
Closed

regression in float to int performance on master #18954

JeffBezanson opened this issue Oct 15, 2016 · 5 comments
Labels
performance Must go faster

Comments

@JeffBezanson
Copy link
Sponsor Member

function g(x)
               a = similar(x,Int)
               for i=1:length(x)
                 a[i] = x[i]
               end
              a
              end

x=Float64.(rand(1:1000000,2000,2000));

On 0.5.0:

julia> @time for i=1:20; g(x); end
  0.284237 seconds (40 allocations: 610.353 MB, 26.00% gc time)

on master:

julia> @time for i=1:20; g(x); end
  0.921421 seconds (40 allocations: 610.353 MB, 3.41% gc time)

The difference in generated code is that convert is no longer inlined, and it uses branches instead of eager &.

@JeffBezanson JeffBezanson added the performance Must go faster label Oct 15, 2016
@simonbyrne
Copy link
Contributor

Could be #14763 related, will look into it.

@vchuravy
Copy link
Sponsor Member

We should add a set of benchmarks for this.

@tkelman tkelman added the kind:potential benchmark Could make a good benchmark in BaseBenchmarks label Oct 17, 2016
@stevengj
Copy link
Member

@vchuravy, just pushed a PR to BaseBenchmarks.

@stevengj stevengj removed the kind:potential benchmark Could make a good benchmark in BaseBenchmarks label Oct 19, 2016
@KristofferC
Copy link
Sponsor Member

KristofferC commented Jan 23, 2017

I'm getting (using the fancy new benchmark macros from JuliaCI/BenchmarkTools.jl#37):

0.6

julia> @btime g(x)
  11.714 ms (2 allocations: 30.52 MiB)

0.5

julia> @btime g(x)
  11.308 ms (2 allocations: 30.52 MiB)

So seems fixed?

@tkelman
Copy link
Contributor

tkelman commented Mar 3, 2017

Do we know what fixed this? Am considering backporting the original change, but it causes a performance regression in conversion and whatever fixed it was not marked for backporting.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Must go faster
Projects
None yet
Development

No branches or pull requests

6 participants