Skip to content

version/parser: speed up StemParser#23007

Merged
MikeMcQuaid merged 1 commit into
mainfrom
stem-parser-perf
Jul 8, 2026
Merged

version/parser: speed up StemParser#23007
MikeMcQuaid merged 1 commit into
mainfrom
stem-parser-perf

Conversation

@Bo98

@Bo98 Bo98 commented Jul 8, 2026

Copy link
Copy Markdown
Member

An I/O operation in a version parser is pretty unusual and, unsurprisingly, does show up in flame graphs when doing a lot of version parsing (though the I/O operation itself isn't too expensive).

The check was added in 355bfc1 but seems obsolete to me after extname was adjusted in 8b5fa68. It seems to have additionally also be handled with a5a1b29.

There was a unit test added in a06bd4e that did test the behaviour I am removing but it seems to be very focussed on what StemParser previously did internally rather than what it was intended to do to the broader version parsing system. Indeed the version number actually returned from Version.parse on that test sample remains the same even with the special StemParser logic removed.

Removing the directory check should also avoid the scenario where version parsing behaved differently if you happen to have a directory in your PWD that matches the input to the parser.

For this change I did the following:

export HOMEBREW_NO_INSTALL_FROM_API=1

brew info --json=v2 --formulae | jq '.formulae[] | "\(.name) \(.versions.stable) \(.versions.head)"'
brew info --json=v2 --casks | jq '.casks[] | "\(.token) \(.version)"'

and did not find any changes between the outputs with and without this change.


  • Have you followed our Contributing guidelines?
  • Have you checked for other open Pull Requests for the same change?
  • Have you explained what your changes do? Performance claims (e.g. "this is faster") must include Hyperfine benchmarks.
  • Have you explained why you'd like these changes included, not just what they do?
  • For bug fixes, have you given step-by-step brew commands to reproduce the bug?
  • Have you written new tests (excluding integration tests)? Here's an example.
  • Have you successfully run brew lgtm (style, typechecking and tests) locally?

  • AI was used to generate or assist with generating this PR.

Copilot AI review requested due to automatic review settings July 8, 2026 04:48

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR removes an unnecessary filesystem I/O check from Version::StemParser to speed up version parsing and avoid parsing behavior depending on whether a matching directory happens to exist on disk.

Changes:

  • Remove spec.directory? short-circuit from Version::StemParser.process_spec, eliminating a filesystem stat during parsing.
  • Remove the unit test that asserted the old “directories are returned as basename” behavior, aligning tests with the new deterministic parsing behavior.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
Library/Homebrew/version/parser.rb Removes the directory existence check in StemParser to avoid I/O and keep parsing purely string-based.
Library/Homebrew/test/version/parser_spec.rb Drops the directory-focused spec that validated the removed behavior.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@MikeMcQuaid MikeMcQuaid left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@MikeMcQuaid MikeMcQuaid added this pull request to the merge queue Jul 8, 2026
Merged via the queue into main with commit 2b14e07 Jul 8, 2026
49 checks passed
@MikeMcQuaid MikeMcQuaid deleted the stem-parser-perf branch July 8, 2026 07:27
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.

4 participants