Build Product DSL ivar names once instead of per access - #439
Open
tas50 wants to merge 1 commit into
Open
Conversation
Each of the 10 generated DSL property methods interpolated a String and
interned a Symbol on every read and every write:
instance_variable_get("@#{prop}".to_sym)
`prop` is fixed when the method is defined, so the Symbol can be built once in
the enclosing block and captured by the closure.
Measured on Ruby 4.0.6:
lookup + omnibus_project 893 ns -> 499 ns 1.79x 5.04 -> 3.04 allocations
lookup + install_path 771 ns -> 555 ns 1.39x 6.02 -> 4.02 allocations
products_available_... 17193 ns -> 11846 ns 1.45x
all 10 props x 32 products 266 us -> 220 us 1.21x 1401 -> 983 allocations
Proc-valued properties are unchanged, as expected: they are dominated by the
Mixlib::Versioning.parse call in `version_for`, not by the ivar lookup.
I also tried replacing `version.to_sym == :latest` in ProductMatrix#lookup
with a direct comparison, on the theory that interning a Symbol per lookup was
wasteful. It measured worse -- allocations went 3.04 -> 4.02 and the call got
slower -- because this file has no frozen_string_literal comment, so the
"latest" literal allocates a String on every call while `to_sym` on an already
interned String is nearly free. That change is deliberately not included.
Output is unchanged: 576 unit examples pass and all 176 golden SHA256 digests
match main.
Signed-off-by: Tim Smith <tsmith84@proton.me>
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
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.
Summary
Each of the 10 generated
ProductDSL property methods interpolated a String and interned a Symbol on every read and every write:propis fixed at the moment the method is defined, so the Symbol can be built once in the enclosing block and captured by the closure.Measurements
Ruby 4.0.6:
lookup+omnibus_projectlookup+install_path(default)products_available_on_downloads_siteProc-valued properties are unchanged, as expected — they are dominated by the
Mixlib::Versioning.parsecall inversion_for, not by the ivar lookup.One thing I tried and rejected
I also tried replacing
version.to_sym == :latestinProductMatrix#lookupwith a direct comparison, on the theory that interning a Symbol per lookup was wasteful.It measured worse — allocations went 3.04 → 4.02 and the call got slower. This file has no
frozen_string_literalcomment, so the"latest"literal allocates a String on every call, whileto_symon an already-interned String is nearly free. That change is deliberately not included here.Verification
chefstyleclean.:not_available, a missing key, and thePACKAGE_ROUTER_ENDPOINTenvironment override.main.