Read and compile each script fragment only once - #440
Open
tas50 wants to merge 2 commits into
Open
Conversation
`erb` costs ~7ms to require and pulls in strscan, cgi/escape and the rest of the erb tree (11 files). It and `ostruct` are only touched inside Generator::Base.get_script when a template actually needs rendering, so load them there rather than at require time. Also fixes the guard on the erb require: the constant is `ERB`, not `Erb`, so `unless defined?(Erb)` never matched and the guard never did anything. Measured on Ruby 4.0.6 (min of 15 fresh subprocesses): before 56.85 ms 240 loaded features after 49.11 ms 229 loaded features Generated output is unchanged: 176 SHA256 digests covering install.sh, install.ps1, the platform detection scripts, every product matrix entry and the ScriptGenerator variants are byte-identical to main. Signed-off-by: Tim Smith <tsmith84@proton.me>
`Generator::Base.get_script` re-read every script fragment from disk on every
call, and re-compiled every erb template from source on every call.
`install_sh` makes 8 such calls, so generating one script did 8 File.reads and
3 ERB compiles. Profiling a single template on Ruby 4.0.6:
File.read 32 us 3 allocations
ERB.new (compile) 94 us 150 allocations
ERB#result (render) 81 us
Reading and compiling together were ~58% of the total. The fragments ship
inside the gem and do not change while the process is running, so they are now
read -- and compiled -- once per path and reused.
install_sh 946 us -> 274 us 3.45x 1041 -> 574 allocations
install_ps1 872 us -> 288 us 3.03x 928 -> 541 allocations
Generator#install_command
818 us -> 242 us 3.38x
detect_platform_sh 36 us -> 1.9 us 19.4x 15 -> 11 allocations
This matters most for anything generating install scripts repeatedly, such as
a service answering install.sh requests, where every request currently pays
the full compile.
Two details worth calling out:
- Plain fragments are cached as Strings but returned with `dup`, because the
old code handed back a fresh String from `File.read` each time and a caller
is free to mutate it. There is a test for this.
- The template branch tests `script.is_a?(String)` rather than `is_a?(ERB)`.
With erb lazily required, referencing the ERB constant when the first script
requested is a plain fragment would raise NameError.
Verified that renders with different contexts do not bleed into each other,
that repeat renders with the same context are stable, and that sh and ps1 do
not share cache entries. 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
Generator::Base.get_scriptre-read every script fragment from disk on every call, and re-compiled every erb template from source on every call.install_shmakes 8 such calls, so generating one script did 8File.reads and 3ERBcompiles.Profiling a single template on Ruby 4.0.6:
File.readERB.new(compile)ERB#result(render)Reading and compiling together were ~58% of the total. The fragments ship inside the gem and do not change while the process is running, so they are now read — and compiled — once per path and reused.
Measurements
install_shinstall_ps1Generator#install_commanddetect_platform_shThis matters most for anything generating install scripts repeatedly — a service answering
install.shrequests pays the full compile on every request today.Two details worth calling out
dup. The old code handed back a fresh String fromFile.readeach time, so a caller is free to mutate the result. Returning the shared cached String would silently break that. There is an explicit test for it.script.is_a?(String), notis_a?(ERB). With erb lazily required, referencing theERBconstant when the first script requested is a plain fragment would raiseNameError. My first draft had exactly that bug.Verification
chefstyleclean.user_agent_stringis recomputed per render, mutating a returned plain script does not poison the cache, and the sh and ps1 generators do not share cache entries.main— which matters here because CI feeds the generated scripts to ShellCheck and PSScriptAnalyzer.