VNV type stability improvements and tests #1102
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Benchmark Report for Commit 305b730Computer InformationBenchmark Results |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1102 +/- ##
==========================================
+ Coverage 81.17% 81.23% +0.05%
==========================================
Files 40 40
Lines 3793 3805 +12
==========================================
+ Hits 3079 3091 +12
Misses 714 714 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
DynamicPPL.jl documentation for PR #1102 is available at: |
|
Current benchmarks: Still not happy with that overhead, will try to understand it better and fix. |
|
Thanks, Markus. To clarify, is the below accurate?
|
|
Yes, that's correct. |
|
Some of the overheads turned out to be in Loop univariate has gotten a lot better, from a 12% slow down to 3%. I might still look into e.g. Dynamic, see if I can squeeze that down, but if there's nothing obvious to be found I would call this good enough and start a PR replacing Metadata with VNV. |
penelopeysm
reviewed
Nov 3, 2025
Comment on lines
+1300
to
+1303
| # Linking can often change the sizes of variables, causing inactive elements. We don't | ||
| # want to keep them around, since typically linking is done once and then the VarInfo | ||
| # is evaluated multiple times. Hence we contiguify here. | ||
| metadata = contiguify!(metadata) |
There was a problem hiding this comment.
If I'm reading the code correctly, I think contiguify! should return nothing and this should not assign to metadata.
There was a problem hiding this comment.
Why should contiguify! return nothing? It doesn't have to return anything since it does its work in-place, but I think it can, see #653 (comment).
There was a problem hiding this comment.
Meh. For sanity's sake I'd prefer single-bang to return nothing, but ok.
| # Linking can often change the sizes of variables, causing inactive elements. We don't | ||
| # want to keep them around, since typically linking is done once and then the VarInfo | ||
| # is evaluated multiple times. Hence we contiguify here. | ||
| metadata = contiguify!(metadata) |
Comment on lines
+590
to
+596
| @test vnv == DynamicPPL.tighten_types!!(deepcopy(vnv)) | ||
| # TODO(mhauru) We would like to check something more stringent here, namely that | ||
| # the operation is compiled to a direct no-op, with no instructions at all. I | ||
| # don't know how to do that though, so for now we just check that it doesn't | ||
| # allocate. | ||
| @allocations(DynamicPPL.tighten_types!!(vnv)) == 0 | ||
| return nothing |
There was a problem hiding this comment.
Could you do something with this?
julia> using DynamicPPL; vnv = DynamicPPL.VarNamedVector();
julia> ct = code_typed(DynamicPPL.tighten_types!!, (typeof(vnv),))
1-element Vector{Any}:
CodeInfo(
1 ─ nothing::Nothing
└── return vnv
) => DynamicPPL.VarNamedVector{Union{}, Union{}, Union{}, Vector{Union{}}, Vector{Union{}}, Vector{Union{}}}
julia> only(ct).first.code
2-element Vector{Any}:
nothing
:(return _2)There was a problem hiding this comment.
I wondered, but this seems brittle to me. Why is there a nothing in that Vector? Will there always be nothing, or will that change with updates to Julia?
There was a problem hiding this comment.
loosen_types!! does this:
julia> ct = code_typed(DynamicPPL.loosen_types!!, (typeof(vnv), Type{Union{}}, Type{Union{}}, Type{Union{}}))
1-element Vector{Any}:
CodeInfo(
1 ─ nothing::Nothing
│ nothing::Nothing
│ nothing::Nothing
└── return vnv
) => DynamicPPL.VarNamedVector{Union{}, Union{}, Union{}, Vector{Union{}}, Vector{Union{}}, Vector{Union{}}}There was a problem hiding this comment.
Yeah, that's fair. On 1.10 there's no nothing. 🤷♀️
julia> only(ct).first.code
1-element Vector{Any}:
:(return _2)I guess it's like pick your poison, either you aren't really testing what you want to test, or you have to use some language internals. Happy with either choice (let's hope @allocations doesn't get broken again).
There was a problem hiding this comment.
Oh, interesting about loosen_types. So maybe to be general, we could test that the last element is a return and everything else is a nothing. But that makes me displeased too.
test/varnamedvector.jl
Outdated
| vnv = DynamicPPL.VarNamedVector() | ||
| vnv = setindex!!(vnv, 1.0, vn) | ||
| vnv = setindex!!(vnv, 2, @varname(b)) | ||
| @test ~DynamicPPL.is_tightly_typed(vnv) |
There was a problem hiding this comment.
Any reason to prefer ~ over !?
There was a problem hiding this comment.
Nope, don't know why I did that. Changed to ! (also in other files, for consistency).
penelopeysm
approved these changes
Nov 3, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
More aggressively tighten element types after each
setindex_internal!!. This fixes performance for some models, most notably the Loop univariate 10k that showed substantial disadvantage compared to Metadata in still in #1098.Also, add tests for
tighten_andloosen_types!!. These become more important since we are calling those two functions all the time now, and they must be compile-time no-ops for this to not cause a large overhead. Tests check this the best they can (I've checked more thoroughly manually withcode_typed).Also fix a type instability in
loosen_types!!caught by the new tests.Benchmarking now, depending on results will either mark this ready or add more performance improvements to this if needed.