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

improve (restore) performance of fieldname by moving out error paths in separate functions #38921

Merged
merged 1 commit into from
Dec 18, 2020

Conversation

KristofferC
Copy link
Member

@KristofferC KristofferC commented Dec 17, 2020

This makes it inline again (maybe new inference change to not infer error paths made it not inline anymore) and also improves performance relative 1.5.

Benchmark:

julia> using BenchmarkTools

julia> struct A
              a::Int
              b::Float64
              c::String
              end

julia> function ff(::Type{T}, i) where {T}
              return fieldname(T, i)
              end;

julia> @btime ff(A, 2)

Before (master): 245.346 ns
Before (1.5.2): 4.747 ns
PR: 2.727 ns

Fixes #38920

cc @quinnj

@KristofferC KristofferC added the performance Must go faster label Dec 17, 2020
@KristofferC KristofferC added the backport 1.6 Change should be backported to release-1.6 label Dec 17, 2020
@KristofferC KristofferC changed the title improve performance of fieldname by moving out error paths in separate functions improve (restore) performance of fieldname by moving out error paths in separate functions Dec 17, 2020
@timholy
Copy link
Member

timholy commented Dec 17, 2020

On my slightly-outdated build (avoiding the JuliaInterpreter breakage...), Base.print_statement_costs(stdout, fieldname, (DataType, Int)) suggests the reason it didn't inline is because _fieldnames is not inferrable and hence length(names) cannot have its dispatch resolved at compile time. This doesn't fix that, which leaves me puzzled about why this solves the inlining problem.

But certainly getting the string interpolation out into separate calls is a good idea.

@KristofferC
Copy link
Member Author

KristofferC commented Dec 17, 2020

Base.print_statement_costs(stdout, fieldname, (DataType, Int))

Doesn't it specialize on the type passed in? So what you get from that are the costs for the abstract DataType but not the actual type (A in this case).

@Keno
Copy link
Member

Keno commented Dec 18, 2020

Can we get a nanosoldier test for this?

@Keno Keno merged commit 9921e8c into master Dec 18, 2020
@Keno Keno deleted the kc/inline_fieldname branch December 18, 2020 03:49
@KristofferC KristofferC added the potential benchmark Could make a good benchmark in BaseBenchmarks label Dec 18, 2020
KristofferC added a commit that referenced this pull request Dec 18, 2020
@KristofferC KristofferC mentioned this pull request Dec 18, 2020
53 tasks
@KristofferC KristofferC removed the backport 1.6 Change should be backported to release-1.6 label Dec 19, 2020
staticfloat pushed a commit that referenced this pull request Jan 15, 2021
ElOceanografo pushed a commit to ElOceanografo/julia that referenced this pull request May 4, 2021
staticfloat pushed a commit that referenced this pull request Dec 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Must go faster potential benchmark Could make a good benchmark in BaseBenchmarks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Performance regression 1.5 -> 1.6 calling fieldname(T, i)
4 participants