Skip to content

Work around $PATH issues under WSL#10506

Merged
mqudsi merged 1 commit into
fish-shell:masterfrom
mqudsi:wsl_paths
May 20, 2024
Merged

Work around $PATH issues under WSL#10506
mqudsi merged 1 commit into
fish-shell:masterfrom
mqudsi:wsl_paths

Conversation

@mqudsi
Copy link
Copy Markdown
Contributor

@mqudsi mqudsi commented May 18, 2024

A common complaint has been the massive amount of directories Windows appends to $PATH slowing down fish when it attempts to find a non-existent binary (which it does a lot more often than someone not in the know might think). The typical workaround suggested is to trim unneeded entries from $PATH, but this a) has considerable friction, b) breaks resolution of Windows binaries (you can no longer use clip.exe, cmd.exe, etc).

This patch introduces a two-PATH workaround. If the cmd we are executing does not contain a period (i.e. has no extension) it by definition cannot be a Windows executable. In this case, we skip searching for it in any of the auto-mounted, auto-PATH-appended directories like /mnt/c/Windows/ or /mnt/c/Program Files, but we do include those directories if what we're searching for could be a Windows executable. (For now, instead of hard-coding a list of known Windows executable extensions like .bat, .cmd, .exe, etc, we just depend on the presence of an extension at all).

e.g. this is what starting up fish prints with logging enabled (that has been removed):

bypassing 100 dirs for lookup of kill
bypassing 100 dirs for lookup of zoxide
bypassing 100 dirs for lookup of zoxide
bypassing 100 dirs for lookup of fd
not bypassing dirs for lookup of open.exe
not bypassing dirs for lookup of git.exe

This has resulted in a massive speedup of common fish functions, especially anywhere we internally use or perform the equivalent of if command -q foo and there's a decent chance foo doesn't exist.

Note that the is_windows_subsystem_for_linux() check will need to be patched to extend this workaround to WSLv2, but I'll do that separately.

Note also that I've taken approach that tries to exempt Windows-mounted /mnt/x/... paths manually added to $fish_user_paths from being excluded, just in case someone downloaded Linux binaries to /mnt/c/Users/jsmith/Downloads/mingw-something-something-toolchain-x.y.z/bin/ directory and wants to use them from Linux without a relative or absolute path. Relative and absolute paths continue to resolve directly, so the surface area of this change is actually a lot smaller than it would seem.

Benchmarks to follow.

Closes #10503

A common complaint has been the massive amount of directories Windows appends to
$PATH slowing down fish when it attempts to find a non-existent binary (which it
does a lot more often than someone not in the know might think). The typical
workaround suggested is to trim unneeded entries from $PATH, but this a) has
considerable friction, b) breaks resolution of Windows binaries (you can no
longer use `clip.exe`, `cmd.exe`, etc).

This patch introduces a two-PATH workaround. If the cmd we are executing does
not contain a period (i.e. has no extension) it by definition cannot be a
Windows executable. In this case, we skip searching for it in any of the
auto-mounted, auto-PATH-appended directories like `/mnt/c/Windows/` or
`/mnt/c/Program Files`, but we *do* include those directories if what we're
searching for could be a Windows executable. (For now, instead of hard-coding a
list of known Windows executable extensions like .bat, .cmd, .exe, etc, we just
depend on the presence of an extension at all).

e.g. this is what starting up fish prints with logging enabled (that has been
removed):

    bypassing 100 dirs for lookup of kill
    bypassing 100 dirs for lookup of zoxide
    bypassing 100 dirs for lookup of zoxide
    bypassing 100 dirs for lookup of fd
    not bypassing dirs for lookup of open.exe
    not bypassing dirs for lookup of git.exe

This has resulted in a massive speedup of common fish functions, especially
anywhere we internally use or perform the equivalent of `if command -q foo`.

Note that the `is_windows_subsystem_for_linux()` check will need to be patched to
extend this workaround to WSLv2, but I'll do that separately.

Benchmarks to follow.
@mqudsi mqudsi added release notes Something that is or should be mentioned in the release notes windows subsystem for linux performance Purely performance-related enhancement without any changes in black box output labels May 18, 2024
@mqudsi
Copy link
Copy Markdown
Contributor Author

mqudsi commented May 18, 2024

--- bench-post  2024-05-18 15:07:05.260920400 -0500
+++ bench-post2 2024-05-18 15:35:07.510279200 -0500
@@ -1,70 +1,70 @@
 aliases.fish
 Benchmark #1: release/fish --no-config benchmarks/benchmarks/aliases.fish
-  Time (mean ± σ):     413.1 ms ±   2.7 ms    [User: 256.1 ms, System: 200.2 ms]
-  Range (min … max):   409.6 ms … 419.3 ms    10 runs
+  Time (mean ± σ):     413.5 ms ±   2.1 ms    [User: 226.5 ms, System: 224.1 ms]
+  Range (min … max):   410.9 ms … 417.9 ms    10 runs

 external_cmds.fish
 Benchmark #1: release/fish --no-config 'benchmarks/benchmarks/external_cmds.fish'
-  Time (mean ± σ):      2.741 s ±  0.015 s    [User: 135.8 ms, System: 2563.2 ms]
-  Range (min … max):    2.719 s …  2.762 s    10 runs
+  Time (mean ± σ):      2.485 s ±  0.021 s    [User: 109.2 ms, System: 2304.1 ms]
+  Range (min … max):    2.455 s …  2.518 s    10 runs

 glob.fish
 Benchmark #1: release/fish --no-config benchmarks/benchmarks/glob.fish
-  Time (mean ± σ):      1.208 s ±  0.006 s    [User: 228.0 ms, System: 979.8 ms]
-  Range (min … max):    1.199 s …  1.219 s    10 runs
+  Time (mean ± σ):      1.233 s ±  0.037 s    [User: 212.4 ms, System: 1019.5 ms]
+  Range (min … max):    1.204 s …  1.330 s    10 runs

 load_completions.fish
 Benchmark #1: release/fish --no-config 'benchmarks/benchmarks/load_completions.fish'
-  Time (mean ± σ):     15.825 s ±  0.131 s    [User: 1.331 s, System: 14.513 s]
-  Range (min … max):   15.676 s … 16.100 s    10 runs
+  Time (mean ± σ):      3.691 s ±  0.006 s    [User: 1.191 s, System: 2.494 s]
+  Range (min … max):    3.682 s …  3.704 s    10 runs

 math.fish
 Benchmark #1: release/fish --no-config benchmarks/benchmarks/math.fish
-  Time (mean ± σ):      1.045 s ±  0.014 s    [User: 754.6 ms, System: 291.0 ms]
-  Range (min … max):    1.012 s …  1.061 s    10 runs
+  Time (mean ± σ):      1.041 s ±  0.018 s    [User: 726.5 ms, System: 313.4 ms]
+  Range (min … max):    1.006 s …  1.063 s    10 runs

 no_execute.fish
 Benchmark #1: release/fish --no-config 'benchmarks/benchmarks/no_execute.fish'
-  Time (mean ± σ):      1.022 s ±  0.019 s    [User: 314.0 ms, System: 671.1 ms]
-  Range (min … max):    0.999 s …  1.057 s    10 runs
+  Time (mean ± σ):      1.003 s ±  0.002 s    [User: 299.9 ms, System: 658.5 ms]
+  Range (min … max):    0.999 s …  1.007 s    10 runs

 printf-escapes.fish
 Benchmark #1: release/fish --no-config benchmarks/benchmarks/printf-escapes.fish
-  Time (mean ± σ):      26.2 ms ±   0.4 ms    [User: 9.7 ms, System: 16.3 ms]
-  Range (min … max):    25.7 ms …  27.6 ms    103 runs
+  Time (mean ± σ):      26.2 ms ±   0.3 ms    [User: 8.8 ms, System: 17.1 ms]
+  Range (min … max):    25.6 ms …  27.8 ms    101 runs

 printf.fish
 Benchmark #1: release/fish --no-config benchmarks/benchmarks/printf.fish
-  Time (mean ± σ):     960.5 ms ±  14.1 ms    [User: 676.4 ms, System: 288.0 ms]
-  Range (min … max):   943.3 ms … 992.8 ms    10 runs
+  Time (mean ± σ):     966.0 ms ±  11.2 ms    [User: 657.6 ms, System: 308.7 ms]
+  Range (min … max):   951.5 ms … 988.8 ms    10 runs

 read.fish
 Benchmark #1: release/fish --no-config benchmarks/benchmarks/read.fish
-  Time (mean ± σ):      3.322 s ±  0.071 s    [User: 165.6 ms, System: 3063.2 ms]
-  Range (min … max):    3.232 s …  3.417 s    10 runs
+  Time (mean ± σ):      3.259 s ±  0.012 s    [User: 171.8 ms, System: 2977.3 ms]
+  Range (min … max):    3.247 s …  3.286 s    10 runs

 seq_echo.fish
 Benchmark #1: release/fish --no-config 'benchmarks/benchmarks/seq_echo.fish'
-  Time (mean ± σ):      89.1 ms ±   1.1 ms    [User: 41.7 ms, System: 46.0 ms]
-  Range (min … max):    87.5 ms …  91.9 ms    32 runs
+  Time (mean ± σ):      89.4 ms ±   1.2 ms    [User: 46.8 ms, System: 42.0 ms]
+  Range (min … max):    87.6 ms …  92.7 ms    32 runs

 set_long.fish
 Benchmark #1: release/fish --no-config 'benchmarks/benchmarks/set_long.fish'
-  Time (mean ± σ):     405.8 ms ±   2.9 ms    [User: 349.9 ms, System: 53.6 ms]
-  Range (min … max):   402.9 ms … 410.8 ms    10 runs
+  Time (mean ± σ):     404.4 ms ±   3.0 ms    [User: 359.4 ms, System: 44.1 ms]
+  Range (min … max):   399.9 ms … 409.0 ms    10 runs

 string-repeat.fish
 Benchmark #1: release/fish --no-config benchmarks/benchmarks/string-repeat.fish
-  Time (mean ± σ):     299.0 ms ±  15.0 ms    [User: 254.7 ms, System: 41.1 ms]
-  Range (min … max):   283.4 ms … 328.9 ms    10 runs
+  Time (mean ± σ):     293.7 ms ±   2.7 ms    [User: 253.0 ms, System: 41.3 ms]
+  Range (min … max):   289.3 ms … 297.0 ms    10 runs

 string-wildcard.fish
 Benchmark #1: release/fish --no-config benchmarks/benchmarks/string-wildcard.fish
-  Time (mean ± σ):     703.1 ms ±  16.1 ms    [User: 462.3 ms, System: 241.3 ms]
-  Range (min … max):   677.8 ms … 728.6 ms    10 runs
+  Time (mean ± σ):     697.0 ms ±   7.2 ms    [User: 454.7 ms, System: 245.8 ms]
+  Range (min … max):   685.2 ms … 706.3 ms    10 runs

 string.fish
 Benchmark #1: release/fish --no-config benchmarks/benchmarks/string.fish
-  Time (mean ± σ):     912.1 ms ±  10.5 ms    [User: 701.3 ms, System: 213.0 ms]
-  Range (min … max):   894.9 ms … 924.5 ms    10 runs
+  Time (mean ± σ):     923.6 ms ±  11.5 ms    [User: 706.2 ms, System: 217.7 ms]
+  Range (min … max):   899.1 ms … 941.3 ms    10 runs

Ignoring the tests that don't show any statistical meaningful results due to not relying on path_get_path_core() that much:

  • external_cmds improves by 10%
  • load_completions improves by an incredible 77%

Copy link
Copy Markdown
Member

@ridiculousfish ridiculousfish left a comment

Choose a reason for hiding this comment

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

LGTM, nice targeted fix.

@zanchey zanchey added this to the fish next-3.x milestone May 20, 2024
@mqudsi
Copy link
Copy Markdown
Contributor Author

mqudsi commented May 20, 2024

Thanks. I could have pulled this higher into the call stack where we have a bit more context available regarding the nature of the PATH variable itself but decided to implement it in path_get_path_core() at least for now, where it can be a much simpler change. If there's some use case I'm completely missing here (which is possible even though WSL is my primary driver), we could consider that.

@mqudsi mqudsi merged commit 3374692 into fish-shell:master May 20, 2024
@mqudsi mqudsi deleted the wsl_paths branch May 20, 2024 15:29
mqudsi added a commit that referenced this pull request May 28, 2024
cuichenli added a commit to cuichenli/fish-shell that referenced this pull request May 6, 2025
ridiculousfish pushed a commit that referenced this pull request May 9, 2025
ridiculousfish pushed a commit that referenced this pull request May 9, 2025
@github-actions github-actions Bot locked as resolved and limited conversation to collaborators May 26, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

performance Purely performance-related enhancement without any changes in black box output release notes Something that is or should be mentioned in the release notes windows subsystem for linux

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fish became slow because checking two tools

3 participants