Improve performance of setproperties/getproperties for structs with unions

[serenity4]

Addresses #55.

These changes make use of generated functions in the case where properties are fields, both for getproperties and setproperties.

In terms of performance, see

julia> using ConstructionBase

julia> using BenchmarkTools

julia> struct A
           a::String
       end

julia> x = A("hello");

julia> @btime getproperties($x)
  1.259 ns (0 allocations: 0 bytes)
(a = "hello",)

julia> @btime setproperties($x, (; a = $("world")))
  1.433 ns (0 allocations: 0 bytes)
A("world")

julia> struct B
           a::Union{Nothing, String}
       end

julia> x = B("hello");

julia> @btime getproperties($x)
  53.251 ns (2 allocations: 32 bytes)
(a = "hello",)

julia> @btime setproperties($x, (; a = nothing))
  0.759 ns (0 allocations: 0 bytes)
B(nothing)

julia> @btime B(nothing)
  0.719 ns (0 allocations: 0 bytes)
B(nothing)

getproperties still allocates, this is due to using a NamedTuple that contains a union for one of its fields. I guess one could force the Union to be formed on the outside, e.g. Union{NamedTuple{....}, NamedTuple{...}}, but I think the current state is fine and has the benefit of simplicity. setproperties is as fast as one would expect, at least.

The "no allocs S2" testset captures the performance improvements of this PR, failing on current master.

[codecov-commenter]

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 0% with 34 lines in your changes missing coverage. Please review.

Project coverage is 1.76%. Comparing base (8e3e773) to head (9e83da3).
Report is 9 commits behind head on master.

Files	Patch %	Lines
src/ConstructionBase.jl	0.00%	34 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #91       +/-   ##
==========================================
- Coverage   25.49%   1.76%   -23.73%     
==========================================
  Files           5       5               
  Lines         153     170       +17     
==========================================
- Hits           39       3       -36     
- Misses        114     167       +53     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[serenity4]

All tests pass, the two downstream failures on nightly don't seem to be related to this PR.

[aplavin]

Nice that it's possible to be both fast and correct :)

[serenity4]

The Setfield failure is due to some timing comparison. The test passes locally for me on the same Julia version, with much lower timings (7ns and 1ns), so I'm guessing it might just be some runner performance inconsistency.

[rafaqz]

Does getproperties on a different Union have that cost in a larger context where the NamedTuple is never returned from a function?

It probably won't need that allocation in practice.

[serenity4]

Does getproperties on a different Union have that cost in a larger context where the NamedTuple is never returned from a function?

It probably won't need that allocation in practice.

Indeed, the allocation is removed:

julia> struct C
         a::Union{Nothing, String}
         b::UInt32
       end

julia> f(x) = begin; props = getproperties(x); props.b end
f (generic function with 2 methods)

julia> x = C("hello", 6)
C("hello", 0x00000006)

julia> @btime f($x)
  1.613 ns (0 allocations: 0 bytes)
0x00000006

[serenity4]

Any objections to merging this? AFAIK this should no longer break anything and only result in performance improvements, particularly so for setproperties.

If the small changes to getproperties aren't welcome, I'm fine with removing them, the intended optimization target was mainly setproperties.

[aplavin]

getfield instead? as the function is named setfields_object here

[serenity4]

The previous implementation used getproperty, I thought best to stay in line with it. Notably, the edge cases where we can't distinguish overloaded properties from fields is handled the same way as before.

[aplavin]

Yeah, the field/property handling in the package should be improved even further in the future, but I agree that for now the minimal change is best

[aplavin]

I guess there are no objections here, @jw3126 @rafaqz?
Then @serenity4 could you bump the version in Project? Then we can merge it.

[serenity4]

Should be all good now, thanks!

[aplavin]

Thanks, should be registered soon!