New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Prefer readlink to greadlink #1484
base: master
Are you sure you want to change the base?
Conversation
rbenv tries to find `greadlink` and then `readlink`. However, there is usually no `greadlink` in Linux. Looking for `greadlink` could be a significant overhead on some environments where `PATH` contains many directories. Ubuntu on WSL2 is just such an environment. Looking for `readlink` before `greadlink` speeds up ruby startup. Before this change: ``` $ time ruby -v ruby 3.3.0dev (2023-02-20T01:33:06Z master 7d5794bad5) [x86_64-linux] real 0m0.532s user 0m0.022s sys 0m0.087s ``` After this change: ``` $ time ruby -v ruby 3.3.0dev (2023-02-20T01:33:06Z master 7d5794bad5) [x86_64-linux] real 0m0.419s user 0m0.042s sys 0m0.055s ```
Using
|
@@ -41,7 +41,7 @@ export RBENV_DIR | |||
|
|||
canonicalize() { | |||
local readlink resolved_path | |||
if readlink="$(type -P greadlink)" || readlink="$(type -P readlink)"; then | |||
if readlink="$(type -P readlink)" || readlink="$(type -P greadlink)"; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We mostly check greadlink
because of older macOS versions (without readlink -f
) where Homebrew or Macports might have placed a GNU readlink into PATH. Checking first readlink
then greadlink
nulifies that use-case because readlink
is always available on macOS (but may or may not support -f
).
You're right that our fallback to manual resolution mostly alleviates the need for greadlink
, but I felt that if greadlink
is present, let's just use it. I didn't think there would be significant overhead with PATH lookups for anyone.
In fact, applying your change does not net any tangible improvements on my machine.
# with this PR
$ hyperfine --shell=none --warmup 3 'bin/rbenv --version'
Benchmark 1: bin/rbenv --version
Time (mean ± σ): 6.7 ms ± 0.8 ms [User: 1.3 ms, System: 3.6 ms]
Range (min … max): 5.6 ms … 11.8 ms 296 runs
# without this PR - around 0.5 ms slower
$ hyperfine --shell=none --warmup 3 'bin/rbenv --version'
Benchmark 1: bin/rbenv --version
Time (mean ± σ): 7.2 ms ± 0.7 ms [User: 1.3 ms, System: 3.9 ms]
Range (min … max): 6.1 ms … 11.7 ms 280 runs
I would say that this change benefits only people with very slow PATH resolution, like on your machine, and that the proper fix would be to change your PATH settings (or your resolver) to be faster.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your comment!
the proper fix would be to change your PATH settings (or your resolver) to be faster.
As for #1483, it is possible. I can put the Linux directory before the Windows directory in the PATH.
However, it is difficult to work around this issue. WSL2 inserts many NTFS directories in the PATH by default, and they are actually needed to be able to transparently launch Windows executables from WSL2 environment.
One possible hack is to put /usr/local/bin/greadlink
as a symbolic link to /usr/bin/readlink
. TBH, I don't want to do that on all my Windows machines.
Your experiment shows an improvement of 0.5 ms on macOS. I admit that it is not so big, but I am seeing the same improvement on my macOS. I believe this change has little maintenance concern. I would appreciate your positive consideration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not opposed to the change. However, please wait a few weeks for me to reach a decision, as I am just leaving to vacation. 🙇
Previously, rbenv tries to find
greadlink
and thenreadlink
. However, there is usually nogreadlink
in Linux. Looking forgreadlink
could be a significant overhead on some environments wherePATH
contains many directories.Ubuntu on WSL2 is just such an environment. Looking for
readlink
beforegreadlink
speeds up ruby startup.Before this change:
After this change: