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

Fix a couple of inferrability issues #18474

Merged
merged 2 commits into from
Sep 13, 2016
Merged

Fix a couple of inferrability issues #18474

merged 2 commits into from
Sep 13, 2016

Conversation

timholy
Copy link
Member

@timholy timholy commented Sep 13, 2016

Discovered while kicking the tires on Unitful. I'm not sure whether there's a concern about making tol a positional argument for rationalize, but the solution I adopted seems reasonable to me. Even when you rely on the keyword argument for tol, this still makes the function faster:

Julia 0.5:

julia> @benchmark rationalize(3.2)
BenchmarkTools.Trial: 
  samples:          10000
  evals/sample:     195
  time tolerance:   5.00%
  memory tolerance: 1.00%
  memory estimate:  224.00 bytes
  allocs estimate:  5
  minimum time:     494.00 ns (0.00% GC)
  median time:      499.00 ns (0.00% GC)
  mean time:        556.76 ns (7.63% GC)
  maximum time:     31.35 μs (97.69% GC)

This branch:

julia> @benchmark rationalize(3.2)
BenchmarkTools.Trial: 
  samples:          10000
  evals/sample:     525
  time tolerance:   5.00%
  memory tolerance: 1.00%
  memory estimate:  192.00 bytes
  allocs estimate:  3
  minimum time:     213.00 ns (0.00% GC)
  median time:      215.00 ns (0.00% GC)
  mean time:        233.26 ns (3.46% GC)
  maximum time:     2.98 μs (87.50% GC)

and

julia> @benchmark rationalize(Int, 3.2, $(eps(3.2)))
BenchmarkTools.Trial: 
  samples:          10000
  evals/sample:     861
  time tolerance:   5.00%
  memory tolerance: 1.00%
  memory estimate:  0.00 bytes
  allocs estimate:  0
  minimum time:     140.00 ns (0.00% GC)
  median time:      141.00 ns (0.00% GC)
  mean time:        146.25 ns (0.00% GC)
  maximum time:     319.00 ns (0.00% GC)

Even when `tol` isn't passed directly as a positional argument, due to the function barrier this also makes the core routine considerably faster.
@tkelman
Copy link
Contributor

tkelman commented Sep 16, 2016

waiting until 0.5.1

@tkelman tkelman added this to the 0.5.x milestone Sep 16, 2016
tkelman pushed a commit that referenced this pull request Feb 22, 2017
(cherry picked from commit 021895a)
ref #18474

Fix inferrability of rationalize

Even when `tol` isn't passed directly as a positional argument, due to the function barrier this also makes the core routine considerably faster.

(cherry picked from commit 2eb8259)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants