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

Microoptimize the walk() function #587

Merged
merged 1 commit into from
Jan 8, 2025
Merged

Microoptimize the walk() function #587

merged 1 commit into from
Jan 8, 2025

Conversation

kmod
Copy link
Contributor

@kmod kmod commented Jan 8, 2025

I noticed that it takes pynvim about 4ms to attach to an nvim instance for me, and 3ms of that is due to the single line:
metadata = walk(decode_if_bytes, metadata)

This commit reduces the walk() time down to 1.5ms, which brings the total attach time down to 2.5ms. This is helpful for me because in my use case I end up connecting to all of the currently-running nvim processes and this starts to take a noticeable amount of time. Unfortunately parallelization does not help here due to the nature of the slowness.

walk() is expensive because it does a very large amount of pure-python manipulation, so this commit is just some tweaks to reduce the overheads:

  • *args and **kw make the function call slow, and we can avoid needing them by pre-packing the args into fn via functools.partial
  • The comprehensions can be written to directly construct the objects rather than create a generator which is passed to a constructor
  • The typechecking is microoptimized by calling type() once and unrolling the type_ in [list, tuple] check

I did notice that in my setup the metadata contains no byte objects, so the entire call is a noop. I'm not sure if that is something that could be relied on or detected, which could be an even bigger speedup.

fix #250

I noticed that it takes pynvim about 4ms to attach to an nvim instance for me,
and 3ms of that is due to the single line:
  metadata = walk(decode_if_bytes, metadata)

This commit reduces the walk() time down to 1.5ms, which brings the total
attach time down to 2.5ms. This is helpful for me because in my use case I end
up connecting to all of the currently-running nvim processes and this starts to
take a noticeable amount of time. Unfortunately parallelization does not help
here due to the nature of the slowness.

walk() is expensive because it does a very large amount of pure-python
manipulation, so this commit is just some tweaks to reduce the overheads:

- *args and **kw make the function call slow, and we can avoid needing them by
  pre-packing the args into fn via functools.partial
- The comprehensions can be written to directly construct the objects rather
  than create a generator which is passed to a constructor
- The typechecking is microoptimized by calling type() once and unrolling the
  `type_ in [list, tuple]` check

I did notice that in my setup the metadata contains no byte objects, so the
entire call is a noop. I'm not sure if that is something that could be relied
on or detected, which could be an even bigger speedup.
@kmod
Copy link
Contributor Author

kmod commented Jan 8, 2025

I believe this is the same issue as was talked about in #250

Copy link
Member

@justinmk justinmk left a comment

Choose a reason for hiding this comment

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

Nice, thanks!

@justinmk justinmk merged commit e5ce595 into neovim:master Jan 8, 2025
4 of 25 checks passed
@justinmk
Copy link
Member

justinmk commented Jan 8, 2025

Note that CI looks yucky right now, we're working on that :)

@kmod
Copy link
Contributor Author

kmod commented Jan 8, 2025

Haha yeah at first I got worried about how I broke so much without realizing it

Also wanted to say, thanks for the project! It's been very handy

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

performance issues
3 participants