Skip to content

Conversation

@THinnerichs
Copy link
Member

@THinnerichs THinnerichs commented Mar 14, 2025

  • StateInt and StateSparseSet are now parametric and dynamically decide for the smallest type using HerbCore.smallest_Int_type.
  • The default type for everything integer is not the concrete type Int (which defaults to Int64 on 64-bit machines) but now the abstract type Integer (for which UInt8 <: Integer and Int <: Integer holds)

Requires Herb-AI/HerbCore.jl#41

Why this is still a draft:
We have to decide what types we want to allow here—the more types we iterate over in smallest_Int_type, the slower the enumeration potentially. I will test this as soon as possible.

As StateInts are initialized by StateSparseSets exclusively, StateSparseSet should do type narrowing, and StateInt should inherit the type from it.

@THinnerichs THinnerichs self-assigned this Mar 14, 2025
@THinnerichs THinnerichs requested a review from ReubenJ March 14, 2025 08:56
@THinnerichs
Copy link
Member Author

  • parametrized StateInt and StateSparseSet
  • defaulted that type to UInt8

@THinnerichs THinnerichs marked this pull request as ready for review March 14, 2025 09:45
@THinnerichs
Copy link
Member Author

Hm, this seems to be more sophisticated:

This is the old HerbSearch version: (Int instead of Integer + Int instead of UInt8)
image

This is the new version: (see notes above)
image

This is when removing parameterization of StateInt and StateSparseSet.
image

@sebdumancic
Copy link
Member

This is a big hit at the performance. Is it possible that the issue is not only in finding the type but that this also introduces type instability (the type can only be deduced during runtime?)

@ReubenJ
Copy link
Member

ReubenJ commented Mar 17, 2025

Just checked the same with BenchmarkTools and AirspeedVelocity to get some more stable numbers. I can post the script to run this tomorrow, so that we can check further modifications.

Runtime

current parametric current/parametric
depth=1 4.21 ± 0.75 μs 7.46 ± 1.4 μs 0.564
depth=2 8.12 ± 0.67 μs 16.3 ± 1.4 μs 0.497
depth=3 26.5 ± 1.3 μs 0.0613 ± 0.0015 ms 0.432
depth=4 0.366 ± 0.015 ms 0.964 ± 0.027 ms 0.38
depth=5 0.0944 ± 0.0019 s 0.272 ± 0.003 s 0.347
time_to_load 0.278 ± 0.0085 s 0.288 ± 0.012 s 0.967

Memory

current parametric current/parametric
depth=1 0.092 k allocs: 12.6 kB 0.116 k allocs: 13.4 kB 0.564
depth=2 0.168 k allocs: 15.8 kB 0.241 k allocs: 18 kB 0.497
depth=3 0.5 k allocs: 29.6 kB 0.845 k allocs: 0.0388 MB 0.432
depth=4 8.29 k allocs: 0.29 MB 14.6 k allocs: 0.457 MB 0.38
depth=5 2.69 M allocs: 0.0762 GB 4.71 M allocs: 0.124 GB 0.347
time_to_load 0.209 k allocs: 13.8 kB 0.209 k allocs: 13.8 kB 0.967

As a sidenote, we should definitely start adding some benchmarks into our packages with the airspeed package. We can have it comment the benchmark results on PRs, which is super cool. It can also benchmark prior versions of our packages to see how our performance has changed with new releases.

@ReubenJ
Copy link
Member

ReubenJ commented Mar 17, 2025

but that this also introduces type instability (the type can only be deduced during runtime?)

Likely something along these lines. I think this video would be a good starting place to check this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants