Skip to content

Don't load tmpdir and fileutils at require time - #282

Merged
tpowell-progress merged 2 commits into
chef:mainfrom
tas50:perf/lazy-require-fileutils
Aug 28, 2026
Merged

Don't load tmpdir and fileutils at require time#282
tpowell-progress merged 2 commits into
chef:mainfrom
tas50:perf/lazy-require-fileutils

Conversation

@tas50

@tas50 tas50 commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

Description

lib/mixlib/shellout.rb has required tmpdir since forever, but nothing in the gem ever calls Dir.mktmpdir or Dir.tmpdir:

$ git grep -n 'mktmpdir\|Dir\.tmpdir' lib/
(no hits)

It's not free — tmpdir transitively pulls in fileutils and etc, which is where essentially all of the require-time allocation comes from.

fileutils itself is used, but in exactly one place: set_cgroup, which only runs when the :cgroup option is set and cgroup v2 is mounted. This moves that require to fork_subprocess, gated on cgroup being set.

Worth calling out why it goes in fork_subprocess and not in set_cgroup itself: set_cgroup runs inside the forked child. require takes a global lock, and if the parent forked while another thread held that lock, the child would block on it forever. Loading in the parent keeps it lazy without the fork-safety hazard.

This matters beyond this gem — it's paid by every consumer (chef-infra, ohai, test-kitchen, inspec) on every process start.

Benchmark

# bench_require.rb — cold process, measures just the require
lib = ARGV[0]
$LOAD_PATH.unshift(lib)
o1 = ObjectSpace.count_objects[:TOTAL]
f1 = $LOADED_FEATURES.size
s  = Process.clock_gettime(Process::CLOCK_MONOTONIC)
require "mixlib/shellout"
e  = Process.clock_gettime(Process::CLOCK_MONOTONIC)
printf("%.3f ms  +%d objects  +%d files\n",
       (e - s) * 1000,
       ObjectSpace.count_objects[:TOTAL] - o1,
       $LOADED_FEATURES.size - f1)

30 cold processes each, ruby 4.0.6, arm64-darwin:

median min max heap objects files loaded
before 12.96 ms 12.63 ms 14.36 ms +2148 +7
after 5.39 ms 4.91 ms 6.58 ms +0 +5

58% faster, and it no longer allocates at require time.

Verification

  • bundle exec rspec — 147 examples, 0 failures (unchanged from main)
  • bundle exec cookstyle --chefstyle -c .rubocop.yml — no offenses
  • Confirmed neither library leaks in after a require, and that a normal run_command works without them:
    $ ruby -Ilib -e 'require "mixlib/shellout"
        raise if $LOADED_FEATURES.any? { |f| f.end_with?("tmpdir.rb", "fileutils.rb") }
        c = Mixlib::ShellOut.new("/bin/echo", "hello"); c.run_command; c.error!'
    
  • Confirmed the :cgroup path still loads FileUtils, and that it loads in the parent before the fork.

spec_helper.rb already requires tmpdir explicitly, so the specs never relied on the library leaking it.

@tas50
tas50 requested review from a team and jaymzh as code owners August 28, 2026 03:51
tas50 added 2 commits August 27, 2026 20:56
`require "tmpdir"` was never used -- nothing in the gem calls Dir.mktmpdir
or Dir.tmpdir -- but it transitively pulls in fileutils and etc on every
require of mixlib-shellout.

`fileutils` is used in exactly one place, set_cgroup, which only runs when
the :cgroup option is set and cgroup v2 is mounted. Load it in the parent
inside fork_subprocess instead. It deliberately goes there rather than in
set_cgroup itself: set_cgroup runs post-fork, and taking the require lock
after forking a threaded parent can deadlock.

Both of these are paid by every consumer of the gem -- chef-infra, ohai,
test-kitchen, inspec -- on every process start.

    $ ruby -e 'require "mixlib/shellout"'   # 30 runs, ruby 4.0.6

    before   median 12.96 ms   +2148 heap objects, +7 files
    after    median  5.39 ms      +0 heap objects, +5 files

58% faster, and it no longer allocates.

Signed-off-by: Tim Smith <tsmith84@proton.me>
The spellcheck job only scans files a PR touches, so any change to
lib/mixlib/shellout/unix.rb or lib/mixlib/shellout.rb fails on
identifiers that have been in those files for years: cgroupv, ducktype,
endgrent, getgrent, LOGNAME, pgid, secondarygroups, seconderies, sgids
and WNOHANG.

"proccess" was flagged too, but that one is an actual typo in a comment
rather than a word worth teaching the dictionary, so fix it instead.

Signed-off-by: Tim Smith <tsmith84@proton.me>
@tpowell-progress
tpowell-progress merged commit b6bc2ae into chef:main Aug 28, 2026
34 checks passed
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.

2 participants