search

lsd-rs / lsd

The next gen ls command
Apache License 2.0
13.16k stars 429 forks source link

Add windows-acl-permissions feature to fix perf issue on windows #902

Closed domsleee closed 11 months ago

domsleee commented 12 months ago

Attempted fix for performance issue mentioned in https://github.com/lsd-rs/lsd/issues/200, for machines on a domain.

Benchmark

For a folder with 50 items.

Before change: lsd took ~40 seconds

After change: 

❯ hyperfine "lsd" --runs 50
Benchmark 1: lsd
  Time (mean ± σ):      39.4 ms ±   8.3 ms    [User: 0.6 ms, System: 0.6 ms]
  Range (min … max):    33.6 ms …  91.8 ms    50 runs

Other notes

I think it makes more sense for this to be disabled by default on windows due to performance, but I could be swayed. My suggestion would be to use https://github.com/lsd-rs/lsd/issues/866#issuecomment-1721012397 as the default windows permissions behaviour.

P.S thanks for this great tool 🎉

TODO

muniu-bot[bot] commented 12 months ago

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: domsleee

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files: - **[OWNERS](https://github.com/lsd-rs/lsd/blob/master/OWNERS)** Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
zwpaper commented 12 months ago

hi @domsleee, thanks for the contribution, I am working on https://github.com/lsd-rs/lsd/pull/882 to add a runtime flag instead of a build time feature for this.

can you have a look at that, and we can discuss which one would be good for users.

domsleee commented 12 months ago

Hi @zwpaper, a runtime flag is probably the better approach. I see in your PR that you are handling this issue as well, nice:

lsd: .: The specified domain either does not exist or could not be contacted. (os error 1355).

I have been able to reproduce this os error 1355 by disconnecting from the internet from my work machine.

I found a similar issue here: https://github.com/docker/for-win/issues/2131#issuecomment-402941794

I used process monitor and found, somewhat hilariously, that GetEffectiveRightsFromAclW is calling FSCTL_PIPE_TRANSCEIVE to fetch a file from a remote server!

Each read takes about a second, which explains why it was taking 45s for my 50 files 😄

domsleee commented 11 months ago

Since https://github.com/lsd-rs/lsd/pull/882 is merged, I will close this one and open another PR for this comment: https://github.com/lsd-rs/lsd/pull/882#issuecomment-1722126112