Skip to content

Remove eval from ArtifactInfo#to_hash - #437

Open
tas50 wants to merge 1 commit into
chef:mainfrom
tas50:perf/artifact-info-no-eval
Open

Remove eval from ArtifactInfo#to_hash#437
tas50 wants to merge 1 commit into
chef:mainfrom
tas50:perf/artifact-info-no-eval

Conversation

@tas50

@tas50 tas50 commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

Summary

ArtifactInfo#to_hash called eval(attribute) once per attribute:

def to_hash
  Hash[ATTRIBUTES.map { |attribute| [attribute.to_sym, eval(attribute)] }]
end

So reading 13 instance variables compiled 13 fresh Ruby ASTs on every call — 33 µs and 67 allocations to do the work of 13 ivar reads. #initialize had a smaller version of the same problem, building "@#{attribute}" and attribute.to_sym per attribute per object.

This precomputes the attribute name and instance variable name as Symbols once at load time and looks them up directly.

Measurements

Ruby 4.0.6:

before after speedup allocations
#to_hash 33194 ns 1814 ns 18.3x 67 → 2
#clone_with 37218 ns 3251 ns 11.4x 84 → 6
.new 2945 ns 1211 ns 2.4x 14 → 1

The gain compounds through Backend::Base#windows_artifact_fixup!, which calls #clone_with for every Windows artifact plus every supported desktop version:

before after speedup
clone a 20-artifact set 744 µs 64 µs 11.7x

Also

Removing the eval drops a dynamic code path from a method that only ever needs to read instance variables. It is not exploitable today since ATTRIBUTES is a frozen literal, but it is a sharp edge worth not having in a method that is otherwise trivial.

Verification

  • 576 unit examples and the functional spec pass; chefstyle clean.
  • Explicit round-trip assertions: to_hash equals the input hash, clone_with overrides only the given keys, the generated readers agree with to_hash, and absent keys still read back nil.
  • All 176 golden SHA256 digests match main.

@tas50
tas50 requested review from a team and jaymzh as code owners August 28, 2026 03:56
`#to_hash` called `eval(attribute)` once per attribute, so reading 13 instance
variables compiled 13 fresh Ruby ASTs on every call. `#initialize` had a
smaller version of the same problem, building `"@#{attribute}"` and
`attribute.to_sym` per attribute per object.

Precompute the attribute name and instance variable name as Symbols once at
load time and look them up directly.

Measured on Ruby 4.0.6:

  #to_hash        33194 ns ->  1814 ns   18.3x   67 -> 2 allocations
  #clone_with     37218 ns ->  3251 ns   11.4x   84 -> 6 allocations
  .new             2945 ns ->  1211 ns    2.4x   14 -> 1 allocation

The gain compounds through `Backend::Base#windows_artifact_fixup!`, which
calls `#clone_with` for every Windows artifact plus every supported desktop
version. Cloning a 20 artifact set: 744 us -> 64 us, 11.7x.

Removing the eval also drops an unnecessary dynamic code path from a method
that only ever needs to read instance variables.

Output is unchanged: 576 unit examples pass and all 176 golden SHA256 digests
match main.

Signed-off-by: Tim Smith <tsmith84@proton.me>
@tas50
tas50 force-pushed the perf/artifact-info-no-eval branch from b85f21c to ba9b950 Compare August 28, 2026 03:58
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.

1 participant