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

Force specialization on the type argument of _similar_for #30331

Merged
merged 1 commit into from
Dec 12, 2018

Conversation

martinholters
Copy link
Member

Looking into the ["misc", "iterators", "zip(1:1, 1:1, 1:1, 1:1)"] regression reported in #30218 (comment), it looks like #28284 pushed the HasShape method of _similar_for beyond the inlining threshold for that case, causing the regression. Adding a @_meta_inline would directly combat that, but forcing specialization on the type argument is less invasive and still restores performance:

julia> for N in (1,1000), M in (1,2,3,4)
           @btime collect(zip($(Iterators.repeated(1:N, M)...,)...))
       end
# master
  30.213 ns (1 allocation: 96 bytes)
  32.706 ns (1 allocation: 96 bytes)
  38.512 ns (1 allocation: 112 bytes)
  130.325 ns (5 allocations: 272 bytes)
  1.069 μs (1 allocation: 7.94 KiB)
  1.753 μs (1 allocation: 15.75 KiB)
  2.367 μs (2 allocations: 23.52 KiB)
  3.187 μs (6 allocations: 31.48 KiB)
# this PR
  30.425 ns (1 allocation: 96 bytes)
  32.565 ns (1 allocation: 96 bytes)
  37.667 ns (1 allocation: 112 bytes)
  42.188 ns (1 allocation: 112 bytes)
  1.048 μs (1 allocation: 7.94 KiB)
  1.781 μs (1 allocation: 15.75 KiB)
  2.359 μs (2 allocations: 23.52 KiB)
  2.837 μs (2 allocations: 31.33 KiB)

I hope @nanosoldier runbenchmarks(ALL, vs = ":master") agrees. In that case, I guess we want this on 1.1 as well.

I vaguely remember having noticed this when working on #29238 but decided it was an orthogonal change to be done in a separate PR and then forgot about it...

@nanosoldier
Copy link
Collaborator

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @ararslan

@martinholters
Copy link
Member Author

OSX Travis ended with:

1788.795694  cd /Users/travis/build/JuliaLang/julia/base && if ! DYLD_FALLBACK_LIBRARY_PATH="/usr/local/lib:/lib:/usr/lib:/usr/local/opt/openblas-julia/lib:/usr/local/opt/suite-sparse-julia/lib" /Users/travis/build/JuliaLang/julia/usr/bin/julia-debug -O0 -C "generic;native" --output-o /Users/travis/build/JuliaLang/julia/usr/lib/julia/sys-debug-o.a.tmp  --startup-file=no --warn-overwrite=yes --sysimage /Users/travis/build/JuliaLang/julia/usr/lib/julia/sys.ji /Users/travis/build/JuliaLang/julia/contrib/generate_precompile.jl 1; then echo '*** This error is usually fixed by running `make clean`. If the error persists, try `make cleanall`. ***'; false; fi 
No output has been received in the last 30m0s, this potentially indicates a stalled build or something wrong with the build itself.
Check the details on how to adjust your build configuration on: https://docs.travis-ci.com/user/common-build-problems/#Build-times-out-because-no-output-was-received
The build has been terminated

Restarted.

Benchmarks look good, but were run before I also changed the AbstractSet / AbstractDict methods, so better safe than sorry: @nanosoldier runbenchmarks(ALL, vs = ":master")

@nanosoldier
Copy link
Collaborator

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @ararslan

@martinholters
Copy link
Member Author

Barring objections (or anyone beating me to it) I'll merge tomorrow.

@martinholters martinholters merged commit 891e2ab into master Dec 12, 2018
@martinholters martinholters deleted the mh/specialize_similar_for branch December 12, 2018 09:50
@martinholters
Copy link
Member Author

What's our policy for backporting performance enhancements to 1.0?

@StefanKarpinski
Copy link
Sponsor Member

If they're low risk then absolutely do it.

KristofferC pushed a commit that referenced this pull request Dec 12, 2018
KristofferC pushed a commit that referenced this pull request Dec 30, 2018
@KristofferC KristofferC mentioned this pull request Dec 30, 2018
53 tasks
@StefanKarpinski StefanKarpinski added status:triage This should be discussed on a triage call backport 1.0 and removed status:triage This should be discussed on a triage call labels Jan 31, 2019
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

Successfully merging this pull request may close these issues.

5 participants