-
-
Notifications
You must be signed in to change notification settings - Fork 5.7k
Open
Description
@KristofferC pointed out to me that our NamedTuple setindex implementation is a bit unfortunate, since the index really needs to be Const(...) if it's going to end up inferred. This does not happen by default:
julia> nt = @NamedTuple{region::UnitRange{Int64}, label::Symbol, value}((1:1, :foo, "bar"));
julia> foo(ann) = Base.setindex(ann, 1:3, :region)
julia> @code_typed optimize=true foo(nt)
CodeInfo(
1 ─ %1 = invoke (Base.setindex::typeof(Base.setindex))(ann::@NamedTuple{region::UnitRange{Int64}, label::Symbol, value}, 1:3::UnitRange{Int64}, :region::Symbol)::NamedTuple
└── return %1
) => NamedTuple # bad return typeThis can be improved by marking the relevant method @inline:
julia> @eval Base @inline function setindex(nt::NamedTuple, v, idx::Symbol)
merge(nt, (; idx => v))
end
julia> @code_typed optimize=true foo(nt)
CodeInfo(
1 ─ %1 = dynamic Base.merge(ann, (region = 1:3,))::@NamedTuple{region::UnitRange{Int64}, label::Symbol, value}
└── return %1
) => @NamedTuple{region::UnitRange{Int64}, label::Symbol, value}This result isn't perfect, but it's substantially better. The merge(...) call became eligible for static dispatch only after inlining, which the optimizer wasn't prepared to notice. However, the return type from inference is now much-improved.
What's unusual is that using @inline at the call site instead gives perfect results somehow:
julia> nt = @NamedTuple{region::UnitRange{Int64}, label::Symbol, value}((1:1, :foo, "bar"));
julia> foo(ann) = @inline Base.setindex(ann, 1:3, :region)
julia> @code_typed optimize=true foo(nt)
CodeInfo(
1 ─ %1 = builtin Base.getfield(ann, :label)::Symbol
│ %2 = builtin Base.getfield(ann, :value)::Any
│ %3 = %new(@NamedTuple{region::UnitRange{Int64}, label::Symbol, value}, 1:3, %1, %2)::@NamedTuple{region::UnitRange{Int64}, label::Symbol, value}
└── return %3
) => @NamedTuple{region::UnitRange{Int64}, label::Symbol, value}Metadata
Metadata
Assignees
Labels
No labels