kristijanhusak / vim-packager

Vim plugin manager that utilizes "jobs" and "pack" features.
MIT License
245 stars 9 forks source link

remove join() call from packager#utils#system() #7

Closed deathlyfrantic closed 5 years ago

deathlyfrantic commented 5 years ago

systemlist() will accept a list as an argument so the join is only necessary if the command should be run through the shell, which doesn't seem to be necessary anywhere this is currently used.

Removing this does have actual performance benefits based on some profiling I did. To create the following profile data, I used these commands:

:profile start $filename
:profile func *packager
:PackStatus (three times)
:profile stop

PackStatus is a command in my personal vimrc (see here) that calls packager#init(), adds all of my plugins to it, then calls packager#status().

I ran PackStatus three times just to get more data to average. Nothing changed between each of the three individual runs, and the only change between the two groups was removing the join() call in packager#utils#system().

For brevity I'm only going to include the time for the packager#utils#system() call - its performance is all we care about anyway for this particular commit. It is by far the most expensive (in terms of time) function in the entire process.

With join:

FUNCTIONS SORTED ON SELF TIME
count  total (s)   self (s)  function
  564   5.511609   0.050797  packager#utils#system()

Without join:

FUNCTIONS SORTED ON SELF TIME
count  total (s)   self (s)  function
  564   4.050826   0.065706  packager#utils#system()

So just removing the join() call saves us about 1.5s over the course of three runs, or approximately 0.5s per run. For reference, I have 47 plugins. (Also note this means this function is called four times per plugin [47 4 3 = 564] - I'm going to try to reduce that in another commit.)

deathlyfrantic commented 5 years ago

This doesn't work in all cases, it turns out. Specifically the output of s:plugin.get_last_update() ends up quoted, which then breaks s:packager.open_sha().

kristijanhusak commented 5 years ago

@deathlyfrantic i'm assuming you are using neovim, where it works without the join. It is required for vim, because it works in a different way.