From 71fb5a5198f41f3ef29a53c01940cf7cf6b233eb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=A9dric=20Belmant?= Date: Mon, 19 Aug 2024 06:33:43 +0200 Subject: [PATCH] Improve performance of `setproperties`/`getproperties` for structs with unions (#91) * Use more generated functions for setproperties/getproperties * Fix properties_are_fields * Avoid allocations on older Julia versions * Allow allocs for union types on older Julia versions * Adjust test bound * Annotate patch with NamedTuple * Attempt to remove method on ::Type * Reuse old getfields implementation * Remove `setproperties_namedtuple` dispatch * Add another NamedTuple annotation * Fix `is_propertynames_overloaded` for NamedTuple * Remove obsolete comment * Restore `getproperties` implementation * Encourage union splitting during `NamedTuple` creation. * Turn Tuple check into an additional method * Make check more concise * Bump version * Whoops, check was backwards --- Project.toml | 2 +- src/ConstructionBase.jl | 55 +++++++++++++++++------------------------ test/runtests.jl | 15 +++++++++++ 3 files changed, 38 insertions(+), 34 deletions(-) diff --git a/Project.toml b/Project.toml index aac34fb..da7b104 100644 --- a/Project.toml +++ b/Project.toml @@ -1,7 +1,7 @@ name = "ConstructionBase" uuid = "187b0558-2788-49d3-abe0-74a17ed4e7c9" authors = ["Takafumi Arakaki", "Rafael Schouten", "Jan Weidner"] -version = "1.5.6" +version = "1.5.7" [deps] LinearAlgebra = "37e2e46d-f89d-539d-b4ee-838fcccc9c8e" diff --git a/src/ConstructionBase.jl b/src/ConstructionBase.jl index ba31236..c0d1236 100644 --- a/src/ConstructionBase.jl +++ b/src/ConstructionBase.jl @@ -61,6 +61,7 @@ if VERSION >= v"1.7" end else function is_propertynames_overloaded(T::Type)::Bool + T <: NamedTuple && return false which(propertynames, Tuple{T}).sig !== Tuple{typeof(propertynames), Any} end @@ -95,7 +96,9 @@ end Tuple(vals) end # names are symbols: return namedtuple +@inline tuple_or_ntuple(::Type{Symbol}, names, vals::Tuple) = namedtuple(names, vals...) @inline tuple_or_ntuple(::Type{Symbol}, names, vals) = NamedTuple{Tuple(names)}(vals) +@inline namedtuple(names, vals...) = NamedTuple{Tuple(names)}((vals...,)) # this seemingly unnecessary method encourages union splitting. # otherwise: throw an error tuple_or_ntuple(::Type, names, vals) = error("Only Int and Symbol propertynames are supported") @@ -131,37 +134,16 @@ end setproperties(obj , patch::Tuple ) = setproperties_object(obj , patch ) setproperties(obj , patch::NamedTuple ) = setproperties_object(obj , patch ) -setproperties(obj::NamedTuple , patch::Tuple ) = setproperties_namedtuple(obj , patch ) -setproperties(obj::NamedTuple , patch::NamedTuple ) = setproperties_namedtuple(obj , patch ) setproperties(obj::Tuple , patch::Tuple ) = setproperties_tuple(obj , patch ) setproperties(obj::Tuple , patch::NamedTuple ) = setproperties_tuple(obj , patch ) -setproperties_namedtuple(obj, patch::Tuple{}) = obj -@noinline function setproperties_namedtuple(obj, patch::Tuple) - msg = """ - setproperties(obj::NamedTuple, patch::Tuple) only allowed for empty Tuple. Got: - obj = $obj - patch = $patch - """ - throw(ArgumentError(msg)) -end -function setproperties_namedtuple(obj, patch) - res = merge(obj, patch) - check_patch_properties_exist(res, obj, obj, patch) - res +@generated function check_patch_fields_exist(obj, patch) + fnames = fieldnames(obj) + pnames = fieldnames(patch) + pnames ⊆ fnames ? :(nothing) : :(throw(ArgumentError($("Failed to assign fields $pnames to object with fields $fnames.")))) end -function check_patch_properties_exist( - nt_new::NamedTuple{fields}, nt_old::NamedTuple{fields}, obj, patch) where {fields} - nothing -end -@noinline function check_patch_properties_exist(nt_new, nt_old, obj, patch) - O = typeof(obj) - msg = """ - Failed to assign properties $(propertynames(patch)) to object with properties $(propertynames(obj)). - """ - throw(ArgumentError(msg)) -end -function setproperties_namedtuple(obj::NamedTuple{fields}, patch::NamedTuple{fields}) where {fields} + +function setproperties(obj::NamedTuple{fields}, patch::NamedTuple{fields}) where {fields} patch end @@ -210,13 +192,20 @@ setproperties_object(obj, patch::Tuple{}) = obj end setproperties_object(obj, patch::NamedTuple{()}) = obj -function setproperties_object(obj, patch) +@generated function setfields_object(obj, patch::NamedTuple) + args = Expr[] + pnames = fieldnames(patch) + for fname in fieldnames(obj) + source = fname in pnames ? :patch : :obj + push!(args, :(getproperty($source, $(QuoteNode(fname))))) + end + :(constructorof(typeof(obj))($(args...))) +end + +function setproperties_object(obj, patch::NamedTuple) check_properties_are_fields(obj) - nt = getproperties(obj) - nt_new = merge(nt, patch) - check_patch_properties_exist(nt_new, nt, obj, patch) - args = Tuple(nt_new) # old julia inference prefers if we wrap in Tuple - constructorof(typeof(obj))(args...) + check_patch_fields_exist(obj, patch) + setfields_object(obj, patch) end include("nonstandard.jl") diff --git a/test/runtests.jl b/test/runtests.jl index f4e68ff..6e5fff9 100644 --- a/test/runtests.jl +++ b/test/runtests.jl @@ -476,6 +476,21 @@ end end end +struct S2 + a::Union{Nothing, Int} + b::Union{UInt32, Int32} +end + +@testset "no allocs S2" begin + obj = S2(3, UInt32(5)) + @test 0 == hot_loop_allocs(constructorof, typeof(obj)) + if VERSION < v"1.6" + @test 32 ≥ hot_loop_allocs(setproperties, obj, (; a = nothing, b = Int32(6))) + else + @test 0 == hot_loop_allocs(setproperties, obj, (; a = nothing, b = Int32(6))) + end +end + @testset "inference" begin @testset "Tuple n=$n" for n in [0,1,2,3,4,5,10,20,30,40] t = funny_numbers(Tuple,n)