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

Speedup windows by only fetching the necessary blocks #475

Closed
wants to merge 2 commits into from

Conversation

AsafMah
Copy link

@AsafMah AsafMah commented Jan 20, 2021

Now from_path is aware of blocks, so it will not fetch permissions and owner and group data if they are not needed.

That creates massive speedup on Domain machines.

Based on the discussion in: #297


  • [ X] Use cargo fmt
  • Add necessary tests
  • Add changelog entry

…d owner and group data if they are not needed.

That creates massive speedup on Domain machines.
@codecov-io
Copy link

Codecov Report

Merging #475 (b422660) into master (1396c59) will decrease coverage by 0.37%.
The diff coverage is 71.73%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #475      +/-   ##
==========================================
- Coverage   84.58%   84.20%   -0.38%     
==========================================
  Files          35       35              
  Lines        3353     3431      +78     
==========================================
+ Hits         2836     2889      +53     
- Misses        517      542      +25     
Impacted Files Coverage Δ
src/core.rs 0.00% <0.00%> (ø)
src/meta/permissions.rs 35.41% <ø> (ø)
src/meta/mod.rs 18.80% <36.36%> (+1.68%) ⬆️
src/flags/blocks.rs 93.36% <100.00%> (-0.45%) ⬇️
src/icon.rs 97.61% <100.00%> (ø)
src/meta/filetype.rs 81.72% <100.00%> (ø)
src/meta/name.rs 93.77% <100.00%> (ø)
src/sort.rs 97.97% <100.00%> (ø)
src/flags/symlink_arrow.rs 88.00% <0.00%> (-12.00%) ⬇️
src/main.rs 0.00% <0.00%> (-10.00%) ⬇️
... and 10 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1396c59...b422660. Read the comment docs.

@meain
Copy link
Member

meain commented Jan 21, 2021

Hey, I had given implementing this a shot after out discussions in #297 . There is one issue in that we use this information to get color/icon in for the name block. Due to this skipping this just because we do not have a permissions block is not really a good idea.

With that out of the way, I was not sure what the best way forward would be. One solution that comes to mind is to have something like windows-mode like thing in the config but that for some reason does not feel right. Or, switch to showing only owner permissions in case of windows by default with an override like unix-mode in config. But I am not sure if that is the best way to go about it.

We could however handle the cases in which we error out when trying to fetch "world" permissions buy dropping a ? in the permissions field(???rw-rwx) but displaying everything else as is.

@AsafMah
Copy link
Author

AsafMah commented Jan 21, 2021

Even just fetching the owner permissions only in a domain in windows takes a looong time.
We need an option to disable the fetching, even at the cost of some of the colors.

@meain
Copy link
Member

meain commented Jan 21, 2021

I don't think we should be skipping fetching owner as well. Once you start removing things like color/permissions completely, I think it is better to use a tool that would be better suited for the job than trying to make lsd handle it.

Ignoring group/other(world) kinda makes sense to me as those don't translate over to Windows from what I understand, but we should at least keep owner as it more or less translates over from user in unix system.

@vvuk
Copy link

vvuk commented Feb 10, 2021

For most Windows users, they are single-user machines. Ownership etc. rarely comes up, other than sometimes needing to click a "Whatever" button on a prompt. I don't think this information is significantly meaningful for the majority of Windows users -- even "do I have permissions to do X on this file" is not that useful, because 99% of the time they can just get the permission they need trivially. So I don't think it makes sense to push a massive perf hit on lsd for something that most windows users won't care about (and if they really do, they're likely going to be using a tool that better understands or can manipulate the permissions).

One thing that's kind of interesting is that dir /q can pull this info quite quickly. cmd /c dir /q takes about 50ms for my test dir from another PR (unpatched lsd takes about 600ms; patched to hack out permission/ownership, 40ms). That pays the same process creation cost (cmd.exe) as lsd would. Might be useful to systrace & profile cmd.exe and see what it's doing, as dir is an internal call there.

@meain
Copy link
Member

meain commented Mar 24, 2021

Closing this. #484 seems to be better for now.

@meain meain closed this Mar 24, 2021
@AsafMah
Copy link
Author

AsafMah commented Jan 18, 2022

Can you maybe re-open? The other pr doesn’t seem to be moving, plus this one is more dynamic as it doesn’t require a special compilation

@meain
Copy link
Member

meain commented Jan 24, 2022

#475 (comment) is my issue with this implementation. It sort of starts removing features even for users without this issue at that point.

@AsafMah
Copy link
Author

AsafMah commented Jan 24, 2022

@meain I think there has been a misunderstanding as to what this PR does.

It doesn't remove any features or change any of the outputs for users.

Right now, lsd always fetches permissions, even if the block isn't selected by the user.
So it does the work of fetching, and then throws it away.

All this PR does, is that for permissions for windows, it checks first if the block is requested, and if it doesn't it doesn't fetch it.

Users who use the blocks will still have them with no change, an users who don't use them will not have the penalty.

@meain
Copy link
Member

meain commented Feb 2, 2022

@AsafMah , I was not able to go through the code again, but if I am remembering correctly doing https://github.com/Peltoche/lsd/pull/475/files#diff-656cbac8b68ed3a36ace232e395f1bb1246301342cda210688d60babec27cda1R33-R39 will disable fetching permissions if they don't have that block and that will result in the file colors being different as we will not have information about things like if the file is an executable.

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.

4 participants