Use common methods for Base functions instead of generating #103

ararslan · 2023-09-29T18:20:04Z

Currently, methods for ==, isequal, hash, and NamedTuple are generated by the @version macro for every row type that gets defined. The bodies of these methods always follow the same patterns, none of which require any information that's only available in the context where the definitions are generated. In fact, specific methods don't need to be generated at all; AbstractRecords can all use the same generic methods for these functions. This provides the following additional benefits:

When many schema versions are defined, this significantly reduces the excessive number of redundant methods which make method autocompletion effectively useless.
Defining the generic methods in terms of regular functions rather than generating expressions in the macro means that it's easier to reason about the methods' behavior and ensure overall consistency. For example, moving from generating a chain of == and && over a record's fields to directly using all means that missing is treated consistently (see == semantics don't match NamedTuple's in presence of missing field #101).

Note that the generic method for hash as defined here loops over the fields in reverse order. This is to match the previous foldr behavior, ensuring that hashes don't change with this implementation.

Fixes #101.

Currently, methods for `==`, `isequal`, `hash`, and `NamedTuple` are generated by the `@version` macro for every row type that gets defined. The bodies of these methods always follow the same patterns, none of which require any information that's only available in the context where the definitions are generated. In fact, specific methods don't need to be generated at all; `AbstractRecord`s can all use the same generic methods for these functions. This provides the following additional benefits: - When many schema versions are defined, this significantly reduces the excessive number of redundant methods which make method autocompletion effectively useless. - Defining the generic methods in terms of regular functions rather than generating expressions in the macro means that it's easier to reason about the methods' behavior and ensure overall consistency. For example, moving from generating a chain of `==` and `&&` over a record's fields to directly using `all` means that `missing` is treated consistently (see issue 101). Note that the generic method for `hash` as defined here loops over the fields in reverse order. This is to match the previous `foldr` behavior, ensuring that hashes don't change with this implementation.

ericphanson · 2023-09-29T18:42:53Z

Since this closes #101, can you add the test from that post? (The current one doesn't cover the case in which there is missing present but == should return false since there is a non-missing field that differs)

src/schemas.jl

@Schema

We want to compare by field, ignoring any type parameters, but not allowing comparison of differing row types. That kind of type query can't be specified via dispatch, but we can add an additional check on the equality of the row types. This is done in a, uh, creative way. ```julia julia> @Schema "test.param" Param julia> @Version ParamV1 begin x::(<:Integer) end julia> x = ParamV1(; i=one(Int32)); julia> typeof(x) ParamV1{Int32} julia> Base.typename(ans) typename(ParamV1) julia> ans.wrapper ParamV1 julia> Base.unwrap_unionall(ans) ParamV1{_I<:Integer} ``` By applying this transformation to the types of two records, we can compare without considering the type parameter. And since we know that the types of the records are the same, we can use `nfields(x)` with the peace of mind that `nfields(y)` will be the same.

src/schemas.jl

jrevels · 2023-09-30T19:29:58Z

can we check that performance is the same as the old methods in benchmark contexts where record types aren't fully inferred

I don't want to go to hell just yet

ararslan · 2023-09-30T21:56:19Z

Not sure if this is what you had in mind but here's what I get. Looks fine to me.

Setup:

using Legolas, BenchmarkTools
using Legolas: @schema, @version

@schema "test.param" Param
@version ParamV1 begin
    i::(<:Integer)
end
@version ParamV2 begin
    i::(<:Integer)
end

bad1() = ParamV1(; i=(rand() < 0.5 ? Int32 : Int64)(1))
bad2() = (rand() < 0.5 ? ParamV1 : ParamV2)(; i=(rand() < 0.5 ? Int32 : Int64))

The return type of bad1 infers as Union{ParamV1{Int32},ParamV1{Int64}} and the return type of bad2 can't be inferred.

main:

julia> @benchmark bad1() == bad1()
BenchmarkTools.Trial: 10000 samples with 10 evaluations.
 Range (min … max):  870.300 ns … 37.980 μs  ┊ GC (min … max): 0.00% … 0.00%
 Time  (median):       1.292 μs              ┊ GC (median):    0.00%
 Time  (mean ± σ):     1.913 μs ±  2.597 μs  ┊ GC (mean ± σ):  0.00% ± 0.00%

  ██▆▆▅▃▂▂▂▂▂▁                                                 ▂
  ███████████████▇▆▅▅▁▄▄▄▄▄▄▄▄▅▅▄▅▄▁▅▁▃▃▁▃▄▁▃▄▁▁▄▁▁▄▃▃▁▃▅▅▃▅▅▅ █
  870 ns        Histogram: log(frequency) by time      19.5 μs <

 Memory estimate: 64 bytes, allocs estimate: 4.

julia> @benchmark bad2() == bad2()
BenchmarkTools.Trial: 10000 samples with 10 evaluations.
 Range (min … max):  1.441 μs …  14.840 μs  ┊ GC (min … max): 0.00% … 0.00%
 Time  (median):     1.660 μs               ┊ GC (median):    0.00%
 Time  (mean ± σ):   1.668 μs ± 224.386 ns  ┊ GC (mean ± σ):  0.00% ± 0.00%

                         ▁▂▂▅▅▆▇▇▇█▇█▇█▆▄▂▃▁▁
  ▁▁▁▁▁▁▁▁▁▂▂▂▂▂▃▃▄▄▅▅▆▆██████████████████████▆▆▅▄▄▃▃▂▂▂▂▂▁▁▁ ▄
  1.44 μs         Histogram: frequency by time        1.83 μs <

 Memory estimate: 96 bytes, allocs estimate: 6.

this branch:

julia> @benchmark bad1() == bad1()
BenchmarkTools.Trial: 10000 samples with 10 evaluations.
 Range (min … max):  1.269 μs …  11.213 μs  ┊ GC (min … max): 0.00% … 0.00%
 Time  (median):     1.493 μs               ┊ GC (median):    0.00%
 Time  (mean ± σ):   1.510 μs ± 210.917 ns  ┊ GC (mean ± σ):  0.00% ± 0.00%

          ▂▄▆█▇▆▅▂
  ▂▂▂▃▃▅▆▇████████▇▆▄▃▃▂▂▂▂▂▂▂▂▂▂▂▂▂▂▂▂▂▂▁▂▁▂▁▁▁▁▁▁▂▂▂▂▂▂▁▂▂▂ ▃
  1.27 μs         Histogram: frequency by time        2.37 μs <

 Memory estimate: 64 bytes, allocs estimate: 4.

julia> @benchmark bad2() == bad2()
BenchmarkTools.Trial: 10000 samples with 10 evaluations.
 Range (min … max):  963.900 ns … 36.246 μs  ┊ GC (min … max): 0.00% … 0.00%
 Time  (median):       1.435 μs              ┊ GC (median):    0.00%
 Time  (mean ± σ):     1.937 μs ±  2.029 μs  ┊ GC (mean ± σ):  0.00% ± 0.00%

  ▅█▇▅▅▅▄▃▂▁ ▁▁▁ ▁                                             ▂
  ██████████████████▆█▆▆▆▆▅▄▅▁▁▃▄▄▄▅▄▅▆▅▆▅▆▅▅▃▄▄▄▃▄▅▄▄▅▅▄▄▄▄▃▄ █
  964 ns        Histogram: log(frequency) by time      13.8 μs <

 Memory estimate: 96 bytes, allocs estimate: 6.

ararslan · 2023-09-30T22:05:20Z

More directly targeting the equality check, there's unsurprisingly no difference at all.

main:

julia> @benchmark x == y setup=(x = bad1(); y = bad1())
BenchmarkTools.Trial: 10000 samples with 1000 evaluations.
 Range (min … max):  6.113 ns … 366.950 ns  ┊ GC (min … max): 0.00% … 0.00%
 Time  (median):     6.207 ns               ┊ GC (median):    0.00%
 Time  (mean ± σ):   8.007 ns ±   8.376 ns  ┊ GC (mean ± σ):  0.00% ± 0.00%

  █  ▆           ▁▅▁   ▃                                      ▁
  █▅▇██▆▆▃▆▅▄▇▇▆▆███▆▅▄██▅▄▄▅▆▅▆▆▆▆▆▆████▇▇█▇▇▆▇▆▆▅▆▅▄▄▄▅▆▇▇▆ █
  6.11 ns      Histogram: log(frequency) by time      17.5 ns <

 Memory estimate: 0 bytes, allocs estimate: 0.

julia> @benchmark x == y setup=(x = bad2(); y = bad2())
BenchmarkTools.Trial: 10000 samples with 995 evaluations.
 Range (min … max):  26.546 ns … 97.779 ns  ┊ GC (min … max): 0.00% … 0.00%
 Time  (median):     34.875 ns              ┊ GC (median):    0.00%
 Time  (mean ± σ):   35.495 ns ±  4.587 ns  ┊ GC (mean ± σ):  0.00% ± 0.00%

               █▃       ▁▆▃
  ▂▃▃▄▃▂▁▁▂▂▂▃▆██▆▅▄▃▃▂▄████▇▅▆▅▆▅▅▅▄▄▄▅▅▆▅▄▄▃▃▃▃▃▃▂▂▂▂▂▂▂▂▂▁ ▃
  26.5 ns         Histogram: frequency by time        46.1 ns <

 Memory estimate: 0 bytes, allocs estimate: 0.

this branch:

julia> @benchmark x == y setup=(x = bad1(); y = bad1())
BenchmarkTools.Trial: 10000 samples with 1000 evaluations.
 Range (min … max):  6.113 ns … 192.035 ns  ┊ GC (min … max): 0.00% … 0.00%
 Time  (median):     6.820 ns               ┊ GC (median):    0.00%
 Time  (mean ± σ):   7.461 ns ±   4.978 ns  ┊ GC (mean ± σ):  0.00% ± 0.00%

  █  █▆▁▁     ▁▁▂▂▂▃▂▂▁ ▁     ▁▁▁                             ▂
  █████████████████████████▇▇██████▇▇▇▆▆▆▆▆▄▆▁▄▆▅▆▄▄▇▄▄▄▄▃▅▅▅ █
  6.11 ns      Histogram: log(frequency) by time      16.8 ns <

 Memory estimate: 0 bytes, allocs estimate: 0.

julia> @benchmark x == y setup=(x = bad2(); y = bad2())
BenchmarkTools.Trial: 10000 samples with 995 evaluations.
 Range (min … max):  26.282 ns … 201.913 ns  ┊ GC (min … max): 0.00% … 0.00%
 Time  (median):     34.388 ns               ┊ GC (median):    0.00%
 Time  (mean ± σ):   35.156 ns ±   4.858 ns  ┊ GC (mean ± σ):  0.00% ± 0.00%

                █▅      ▁▅█▂    ▁
  ▂▃▅▃▃▁▁▁▂▄▅▅▄▆██▅▄▅▃▃▅████▇▅▄▅█▇▅▅▆▅▄▅▆▇▆▅▄▄▃▄▃▄▄▃▃▂▂▂▂▂▂▂▂▁ ▃
  26.3 ns         Histogram: frequency by time         45.6 ns <

 Memory estimate: 0 bytes, allocs estimate: 0.

test/runtests.jl

omus · 2023-10-02T18:50:05Z

test/runtests.jl

+        @test !isequal(UnionMissingV1(; a=missing, b=1), UnionMissingV1(; a=missing, b=2))
+        @test ParamV1(; i=one(Int32)) == ParamV1(; i=one(Int64))
+        @test isequal(ParamV1(; i=one(Int32)), ParamV1(; i=one(Int64)))
+    end


Should add some tests comparing ChildV1 to a ParentV1 and a ParentV1 to a ChildV1. In particuar I am thinking about _compare_fields(eq, ::AbstractRecord, ::AbstractRecord)

Is what I just added what you had in mind?

Co-authored-by: Curtis Vogt <[email protected]>

jrevels · 2023-10-03T13:02:22Z

I sanity checked NamedTuple conversion performance as it can end up being in the critical inner loops in some use cases; result is that it seems fine on julia 1.9.

using Legolas, BenchmarkTools
using Legolas: @schema, @version

@schema "test.foo" Foo

@version FooV1 begin
    a::(<:Any)
    b::(<:Any)
    c::(<:Any)
    d::(<:Any)
end

@version FooV2 begin
    x::(<:Any)
    y::(<:Any)
    z::(<:Any)
end

# attempt to thwart compiler optimizations
function test(v, n)
    result = 0
    for _ in 1:n
        result += sum(sizeof, values(NamedTuple(rand(v))))
    end
    return result
end

gen_foos() = Any[FooV1(a=1,b=2,c=3,d=4),FooV2(x="x",y="y",z="z")]

@benchmark test(foos, 100) setup=(foos=gen_foos()) evals=1

this branch:

julia> @benchmark test(foos, 100) setup=(foos=gen_foos()) evals=1
BenchmarkTools.Trial: 10000 samples with 1 evaluation.
 Range (min … max):  15.541 μs …  1.652 ms  ┊ GC (min … max): 0.00% … 97.53%
 Time  (median):     16.167 μs              ┊ GC (median):    0.00%
 Time  (mean ± σ):   16.839 μs ± 28.094 μs  ┊ GC (mean ± σ):  2.84% ±  1.69%

        ▄ █▁▆▂
  ▁▁▁▂▃▆█▇████▄▅▂▃▃▂▃▂▃▂▁▂▁▁▁▁▂▁▂▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁ ▂
  15.5 μs         Histogram: frequency by time        19.5 μs <

 Memory estimate: 15.11 KiB, allocs estimate: 441.

main:

julia> @benchmark test(foos, 100) setup=(foos=gen_foos()) evals=1
BenchmarkTools.Trial: 10000 samples with 1 evaluation.
 Range (min … max):  15.750 μs …  1.811 ms  ┊ GC (min … max): 0.00% … 96.41%
 Time  (median):     16.625 μs              ┊ GC (median):    0.00%
 Time  (mean ± σ):   17.324 μs ± 30.596 μs  ┊ GC (mean ± σ):  2.99% ±  1.68%

          ▄ ██▅
  ▂▂▂▂▂▃▅██████▆█▅▄▄▃▄▄▃▂▂▂▂▂▂▃▃▃▂▂▂▂▂▂▂▂▂▂▂▂▂▂▂▂▁▁▂▂▂▂▂▂▂▂▂▂ ▃
  15.8 μs         Histogram: frequency by time        20.1 μs <

 Memory estimate: 14.98 KiB, allocs estimate: 443.

ericphanson · 2023-10-03T14:05:23Z

FWIW if you are looking for perf drops here, I would look at > 10 fields. E.g. in https://discourse.julialang.org/t/slowness-of-fieldnames-and-propertynames/55364/2 (very similar case) I found inference failed for 11 fields but not for 9. That was on an older version of julia though.

ararslan · 2023-10-03T18:48:01Z

Tried using row types with 15 and 25 parameters and still get no meaningful difference.

setup

using Legolas, BenchmarkTools, Base.Iterators
using Legolas: @schema, @version

macro make_version(name, n)
    block = Expr(:block)
    ex = Expr(:macrocall, Symbol("@version"), __source__, name, block)
    for i in 1:n
        x = Symbol(:x, i)
        push!(block.args, Expr(:(::), Symbol(:x, i), :(<:Any)))
    end
    return ex
end

@schema "test.foo" Foo
@make_version FooV1 15
@make_version FooV2 25

function make_row(T, values)
    nt = NamedTuple()
    for (field, value) in zip(fieldnames(T), values)
        nt = merge(nt, (; field => value))
    end
    return T(; nt...)
end

gen_foos() = Any[make_row(FooV1, countfrom(1)), make_row(FooV2, string.('a':'z'))]

function test(v, n)
    result = 0
    for _ in 1:n
        result += sum(sizeof, values(NamedTuple(rand(v))))
    end
    return result
end

@benchmark test(foos, 100) setup=(foos = gen_foos()) evals=1

this branch

BenchmarkTools.Trial: 10000 samples with 1 evaluation.
 Range (min … max):  17.958 μs … 887.416 μs  ┊ GC (min … max): 0.00% … 95.49%
 Time  (median):     19.166 μs               ┊ GC (median):    0.00%
 Time  (mean ± σ):   20.348 μs ±  27.253 μs  ┊ GC (mean ± σ):  4.52% ±  3.29%

     ▁▃▅▅▇███▇▇▅▄▃▂▁                                           ▂
  ▂▆██████████████████▇▇▆▆▅▅▄▆▆▇▆▇▇▇████▇▇▆▇▇▇▇▇▅▆▆▄▆▆▅▃▆▅▅▅▅▄ █
  18 μs         Histogram: log(frequency) by time      25.1 μs <

 Memory estimate: 61.17 KiB, allocs estimate: 484.

main

BenchmarkTools.Trial: 10000 samples with 1 evaluation.
 Range (min … max):  19.125 μs … 811.000 μs  ┊ GC (min … max): 0.00% … 94.50%
 Time  (median):     20.292 μs               ┊ GC (median):    0.00%
 Time  (mean ± σ):   21.633 μs ±  29.285 μs  ┊ GC (mean ± σ):  4.93% ±  3.55%

         ▁▁▇█▆▂▁
  ▁▁▁▁▂▃▅███████▅▃▂▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁ ▂
  19.1 μs         Histogram: frequency by time         25.8 μs <

 Memory estimate: 60.22 KiB, allocs estimate: 485.

ararslan · 2023-10-03T18:55:06Z

Will wait to trigger registration since I'd like to get #102 merged too

ararslan requested review from jrevels and omus as code owners September 29, 2023 18:20

Don't test hash value directly

639ec14

Add (nearly) exact test case from issue 101

ee86e09

ararslan mentioned this pull request Sep 29, 2023

Allow a schema version to declare no fields #102

Merged

ararslan commented Sep 29, 2023

View reviewed changes

src/schemas.jl Outdated Show resolved Hide resolved

ericphanson reviewed Sep 30, 2023

View reviewed changes

src/schemas.jl Outdated Show resolved Hide resolved

ararslan added 2 commits September 30, 2023 13:54

Use Legolas functions instead of Base internals

88288f5

I don't want to go to hell just yet

Simplify further

c7792de

omus reviewed Oct 2, 2023

View reviewed changes

ararslan and others added 2 commits October 2, 2023 13:10

Make tests better

2cf0556

Co-authored-by: Curtis Vogt <[email protected]>

Better formatted tests + add parent/child equality tests

1d054bd

jrevels approved these changes Oct 3, 2023

View reviewed changes

ararslan merged commit 927b2ac into main Oct 3, 2023

ararslan deleted the aa/fix-101 branch October 3, 2023 18:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use common methods for Base functions instead of generating #103

Use common methods for Base functions instead of generating #103

ararslan commented Sep 29, 2023

ericphanson commented Sep 29, 2023

jrevels commented Sep 30, 2023

ararslan commented Sep 30, 2023

ararslan commented Sep 30, 2023

omus Oct 2, 2023

ararslan Oct 2, 2023

jrevels commented Oct 3, 2023

ericphanson commented Oct 3, 2023

ararslan commented Oct 3, 2023

ararslan commented Oct 3, 2023

Use common methods for Base functions instead of generating #103

Use common methods for Base functions instead of generating #103

Conversation

ararslan commented Sep 29, 2023

ericphanson commented Sep 29, 2023

jrevels commented Sep 30, 2023

ararslan commented Sep 30, 2023

ararslan commented Sep 30, 2023

omus Oct 2, 2023

Choose a reason for hiding this comment

ararslan Oct 2, 2023

Choose a reason for hiding this comment

jrevels commented Oct 3, 2023

ericphanson commented Oct 3, 2023

ararslan commented Oct 3, 2023

ararslan commented Oct 3, 2023