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

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

Closed
wants to merge 1 commit into from

Conversation

domsleee
Copy link
Contributor

Attempted fix for performance issue mentioned in #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 #866 (comment) as the default windows permissions behaviour.

P.S thanks for this great tool 🎉


TODO

  • Use cargo fmt
  • Add necessary tests (Not applicable imo)
  • Add changelog entry
  • Update default config/theme in README (if applicable)
  • Update man page at lsd/doc/lsd.md (if applicable)

@muniu-bot
Copy link

muniu-bot bot commented Sep 15, 2023

[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:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@domsleee domsleee force-pushed the bugfix/slow-acl-windows branch from c5641ad to 83105d2 Compare September 15, 2023 10:33
@zwpaper
Copy link
Member

zwpaper commented Sep 15, 2023

hi @domsleee, thanks for the contribution, I am working on #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.

@zwpaper zwpaper added this to the v1.1.0 milestone Sep 15, 2023
@domsleee
Copy link
Contributor Author

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: docker/for-win#2131 (comment)

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
Copy link
Contributor Author

Since #882 is merged, I will close this one and open another PR for this comment: #882 (comment)

@domsleee domsleee closed this Sep 25, 2023
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.

2 participants