Skip to content
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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mame
Copy link

@mame mame commented Feb 20, 2023

Previously, 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

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
```
@mame
Copy link
Author

mame commented Feb 20, 2023

Using greadlink was introduced at b7e19b4 for Solaris 10 that has no readlink command. But as far as I know, if recent Solaris 10 has /usr/ccs/bin/greadlink, it usually has /usr/ccs/gnu/readlink as well. Probably it won't be much of a problem.

greadlink also used to be required on older macOS whose readlink did not support the -f option. But now, thanks to the manual resolution implemented in #1482, even old readlink can work great now. Interestingly, manual resolution is slightly faster than using greadlink bundled in macports coreutils package, if my experiment is correct.

@@ -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
Copy link
Member

@mislav mislav Feb 20, 2023

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.

Copy link
Author

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.

Copy link
Member

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. 🙇

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.

None yet

2 participants