Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace getproperty parameter data access with typed getindex to improve inference #9

Merged
merged 9 commits into from
Apr 1, 2021

Conversation

halleysfifthinc
Copy link
Owner

The getproperty syntax to access the data in a parameter, e.g.
f.groups[:POINT].RATE caused type instability (due to the parameter
data possibly being any number of types, String, Vector{Float32}, Int16,
etc). This type instability also affected accessing Group fields using
the dot syntax. (getfield would not have been affected.)

The root cause of the type instability is that the type
of the params Dictionary in each Group must be not fully concrete, since groups will have any
number of parameters with different types. However, some types can be
assumed based on the group/name (e.g. POINT:USED should always be an
integer). My solution is to add a typed getindex that type asserts
the return type: f.groups[:POINT][Int, :USED]. This solves the type
instability, while maintaining the ability to access parameters even when
you don't know or care to assert the type: f.groups[:POINT][:USED].

This (and a few other inference related changes) is responsible for dramatically improving the "first run" time and reducing allocations as well.

Benchmarks:

#master
julia> @time readc3d("data/sample01/Eb015pr.c3d")
  1.866162 seconds (5.28 M allocations: 303.120 MiB, 3.61% gc time, 14.19% compilation time)

julia> @benchmark readc3d("data/sample01/Eb015pr.c3d")
BenchmarkTools.Trial:
  memory estimate:  2.85 MiB
  allocs estimate:  24863
  --------------
  minimum time:     2.247 ms (0.00% GC)
  median time:      2.309 ms (0.00% GC)
  mean time:        2.460 ms (4.77% GC)
  maximum time:     4.668 ms (46.08% GC)
  --------------
  samples:          2032
  evals/sample:     1
#PR
julia> @time readc3d("data/sample01/Eb015pr.c3d")
  1.019505 seconds (2.80 M allocations: 159.463 MiB, 4.10% gc time, 23.21% compilation time)

julia> @benchmark readc3d("data/sample01/Eb015pr.c3d")
BenchmarkTools.Trial:
  memory estimate:  1.84 MiB
  allocs estimate:  5295
  --------------
  minimum time:     821.959 μs (0.00% GC)
  median time:      861.177 μs (0.00% GC)
  mean time:        922.916 μs (5.83% GC)
  maximum time:     2.498 ms (59.99% GC)
  --------------
  samples:          5416
  evals/sample:     1

The `getproperty` syntax to access the data in a parameter, e.g.
`f.groups[:POINT].RATE` caused type instability (due to the parameter
data possibly being any number of types, String, Vector{Float32}, Int16,
etc). This type instability also affected accessing `Group` fields using
the dot syntax. (`getfield` would not have been affected.)

The root cause of the type instability is the required "unstable" type
of the `params` Dictionary in each `Group`, since groups will have any
number of parameters with different types. However, some types can be
assumed based on the group/name (e.g. POINT:USED should always be an
integer). My solution is to add a typed `getindex` that type asserts
the return type: `f.groups[:POINT][Int, :USED]`. This solves the type
instablity, while maintaining the ability to access parameters even when
you don't know or care to assert the type: `f.groups[:POINT][:USED]`.
@halleysfifthinc halleysfifthinc merged commit e154c1e into master Apr 1, 2021
@halleysfifthinc halleysfifthinc deleted the improve-inference branch April 1, 2021 19:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant