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

perf(loader): use a quicker version of vim.fs.normalize #28746

Merged
merged 2 commits into from
May 15, 2024

Conversation

lewis6991
Copy link
Member

@lewis6991 lewis6991 commented May 14, 2024

Problem:

vim.fs.normalize() normalizes too much vim.loader and is slow.

Solution:

Make it faster by doing less. This reduces the times spent in
vim.fs.normalize in vim.loader from ~13ms -> 1-2ms.

Numbers from a relative benchmark:

  • Skipping vim.validate(): 285ms -> 230ms
  • Skipping path_resolve_dot(): 285ms -> 60ms
  • Skipping double_slash: 60ms -> 35ms

@@ -488,6 +488,8 @@ end
--- (default: `true`)
--- @field expand_env? boolean
---
--- @field package _fast? boolean
Copy link
Member

Choose a reason for hiding this comment

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

Do you plan to remove this in 0.11?

I wonder if vim.validate() can be made faster.

Copy link
Member Author

Choose a reason for hiding this comment

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

TBD, most of the gain here is by bypassing path_resolve_dot(), bypassing vim.validate is just a bonus.

The alternative is to maintain a small version of this in loader.lua.

Copy link
Member

@justinmk justinmk May 14, 2024

Choose a reason for hiding this comment

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

This is fine for now but maybe we can make path_resolve_dot faster in 0.11. And use assert() instead of vim.validate.

@wookayin wookayin added performance issues reporting performance problems lua stdlib labels May 14, 2024
@lewis6991 lewis6991 force-pushed the perfnorm branch 2 times, most recently from ab740fd to 2a5e867 Compare May 15, 2024 09:39
@lewis6991
Copy link
Member Author

Refined this some more and added some benchmarking data. Also added some optimisations to path_resolve_dot().

Problem:

vim.fs.normalize() normalizes too much vim.loader and is slow.

Solution:

Make it faster by doing less. This reduces the times spent in
vim.fs.normalize in vim.loader from ~13ms -> 1-2ms.

Numbers from a relative benchmark:
- Skipping `vim.validate()`: 285ms -> 230ms
- Skipping `path_resolve_dot()`: 285ms -> 60ms
- Skipping `double_slash`: 60ms -> 35ms
@lewis6991 lewis6991 force-pushed the perfnorm branch 2 times, most recently from bc7b060 to f77b3f5 Compare May 15, 2024 10:08
@lewis6991 lewis6991 merged commit 14a5813 into neovim:master May 15, 2024
29 checks passed
local double_slash = vim.startswith(path, '//') and not vim.startswith(path, '///')
local double_slash = false
if not opts._fast then
double_slash = vim.startswith(path, '//') and not vim.startswith(path, '///')
Copy link
Member

@justinmk justinmk May 15, 2024

Choose a reason for hiding this comment

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

is vim.startswith() slow? I see it uses vim.validate, maybe path:sub() could be inlined here?

and maybe we should use assert instead of vim.validate in startswith ...

Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't think it would be, but skipping this line achieved a 50% increase (60ms -> 35ms) after the other opts applied.

The important thing to note is that vim.loader really does not need most of the logic in this function. Arguably it could work well enough without any normalization, it is just used to increase cache hits for different input paths that point to the same place.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lua stdlib performance issues reporting performance problems
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants