Significant performance regression for types with unions in 1.3.0

[ancapdev]

using BenchmarkTools
using ConstructionBase

struct A
    x::Int
    y::Int
end

struct B
    x::Int
    y::Union{Nothing, Int}
end

function update(x)
    setproperties(x, (; x = x.x + 1))
end

function update2(x::T) where T
    T(x.x + 1, x.y)
end

println("Type without union")
@btime update($(Ref(A(1, 2)))[])
@btime update2($(Ref(A(1, 2)))[])

println("Type with union")
@btime update($(Ref(B(1, 2)))[])
@btime update2($(Ref(B(1, 2)))[])

Results 1.3.0

Type without union
  1.182 ns (0 allocations: 0 bytes)
  1.182 ns (0 allocations: 0 bytes)
Type with union
  381.271 ns (6 allocations: 176 bytes)
  12.597 ns (0 allocations: 0 bytes)

Results 1.2.1

Type without union
  1.182 ns (0 allocations: 0 bytes)
  1.182 ns (0 allocations: 0 bytes)
Type with union
  12.597 ns (0 allocations: 0 bytes)
  12.597 ns (0 allocations: 0 bytes)

This appears to be due to the change to remove the use of generated functions for setproperties. Whereas before it would generate the equivalent of a hand rolled function (update2), now it relies on going through named tuples for its merging logic. The issue being, getproperties is type unstable in the presence of union fields. In this example:

x = B(1, 2)
@code_warntype ConstructionBase.setproperties_object(x, (; x = x.x + 1))

Outputs

MethodInstance for ConstructionBase.setproperties_object(::Main.Foo.B, ::NamedTuple{(:x,), Tuple{Int64}})
  from setproperties_object(obj, patch) in ConstructionBase at /home/christian/.julia/dev/ConstructionBase/src/ConstructionBase.jl:145
Arguments
  #self#::Core.Const(ConstructionBase.setproperties_object)
  obj::Main.Foo.B
  patch::NamedTuple{(:x,), Tuple{Int64}}
Locals
  nt_new::NamedTuple{(:x, :y)}
  nt::NamedTuple{(:x, :y), _A} where _A<:Tuple{Int64, Union{Nothing, Int64}}
Body::Main.Foo.B
1 ─      nothing
│        (nt = ConstructionBase.getproperties(obj))
│        (nt_new = ConstructionBase.merge(nt, patch))
│        ConstructionBase.validate_setproperties_result(nt_new, nt, obj, patch)
│   %5 = ConstructionBase.typeof(obj)::Core.Const(Main.Foo.B)
│   %6 = ConstructionBase.constructorof(%5)::Core.Const(Main.Foo.B)
│   %7 = ConstructionBase.Tuple(nt_new)::Tuple
│   %8 = Core._apply_iterate(Base.iterate, %6, %7)::Main.Foo.B
└──      return %8

[jw3126]

Interesting, maybe we have to go back to generated functions then.

[ancapdev]

Interesting, maybe we have to go back to generated functions then.

Out of interest, what was the motivation to go away from them in the first place? It seems counter intuitive to me to ask the compiler to make heuristic decisions through constant prop and inlining, when for something like setproperties I think you always want to generate specific optimized code for every combination of argument types.

[jw3126]

@generated functions are not as transparent to the compiler as ordinary functions. Which can for instance hurt inference.
See also:

• Using @generated less? #21
• https://discourse.julialang.org/t/why-not-use-generated/29531

That being said I know very little about the tradeoff here. I know no example for which setproperties got better by the current implementation and your example is the only regression I know about the current implementation.

[ancapdev]

I wasn't aware of the inference issues, that's interesting.

Ideally I think the functionality of this package would be supported in the core compiler, what we're having to do here is making the compiler jump through hoops and apply a lot of extra work to recover what should be semantically clear and simple instructions.

One workaround I could think of is for getproperties to capture union fields in a struct that encapsulate the union type, such that their physical layout (e.g., tagged union) is preserved and the type remains stable.

[jw3126]

One workaround I could think of is for getproperties to capture union fields in a struct that encapsulate the union type, such that their physical layout (e.g., tagged union) is preserved and the type remains stable.

It is an interesting idea, but I have no idea how to implement it in a compiler friendly way and it would be a gotcha for users of this package.

@rafaqz @tkf are you okay with restoring the old @generated setproperties definition?

[jw3126]

@aplavin might have some thoughts on this

[serenity4]

Any updates on this? Is there an agreement on going forward with reverting to using @generated functions?

I personally agree with the sentiment of the thread that @generated functions are a good use in this context, and I don't see any other way to address the performance issue (even less so in a reliable way without them).

EDIT: literally posted seconds after the mention above.

[aplavin]

I personally don't have anything against generated functions here.

Originally, this switch from generated to regular functions was introduced in a bugfix #60. I guess, whenever fields == properties, one can use @generated on all julia versions. And there may be clever way to do it with arbitrary properties as well, idk.

It would be great to add a test ensuring that union types remain performant in the future. Without a test, it's very easy to miss such a regression in the future – just as it happened in #60.

[rafaqz]

I'm 100% for using generated functions.

It's a guarantee that Julia versions won't break performance.

[aplavin]

Should be fixed by #91