search

neovim / pynvim

Python client and plugin host for Nvim
http://pynvim.readthedocs.io/en/latest/
Apache License 2.0
1.53k stars 120 forks source link

`num_to_str` boolean check makes `Ultisnips` snippet engine laggy #523

Closed jsr-p closed 7 months ago

jsr-p commented 1 year ago

Hi

Today I cloned the repository and installed the package directly instead from PyPi.

After reopening nvim I noticed that nvim was very laggy in all python files. I found out that the issue was with my snippet engine Ultisnips. Deleting the package and reinstalling it from PyPi made the lag go away.

The version on PyPi is 4.3 and so I went through the git history to check where something significant changed. I located the issue to be this PR #506. This PR changes the num_to_str function to include a boolean check. The helper function is used in the LegacyVim object's eval function here. Browsing through the Ultisnips source code reveals that it uses vim.eval at many places. Thus, this change is non-trivial in terms of Ultisnips performance.

I wonder what could be done to circument this issue?

jsr-p commented 1 year ago

Thinking more about, I guess the #506 PR is not compliant with the vim.eval documentation.

The documentation states

vim.eval(str) python-eval Evaluates the expression str using the vim internal expression evaluator (see expression). Returns the expression result as: (1) a string if the Vim expression evaluates to a string or number (2) a list if the Vim expression evaluates to a Vim list (3) a dictionary if the Vim expression evaluates to a Vim dictionary Dictionaries and lists are recursively expanded.

In Python, booleans are implemented as a subclass of integers. In nvim, v:true and v:false evalutes to true and false (boolean values); for boolean values vim uses numbers. Therefore, by (1) and that booleans are integers in Python, vim.eval('v:true') should evaluate to the Python string "True" and not the boolean True. And therefore :py3 print(type(vim.eval('v:true'))) should print out <class 'str'> and not <class 'bool'>. Thus, the extra check in #506 is redundant, right?

justinmk commented 1 year ago

@jsr-p If reverting https://github.com/neovim/pynvim/pull/506 fixes a performance issue, please send a PR reverting it, with an explanation. I don't think https://github.com/neovim/pynvim/pull/506 is worth slower performance.

jsr-p commented 1 year ago

@justinmk I have switched to using LuaSnip but thanks for considering my comment on the performance!

wookayin commented 11 months ago

@justinmk I think this might need to be revisited around 0.5.0 release because this could be a breaking change and have some performance impacts. (technically speaking, #506 is a breaking change on the behavior)

justinmk commented 11 months ago

No objection to reverting https://github.com/neovim/pynvim/pull/506 . It wasn't given a strong justification