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

Speed up Vim startup #98

Closed
wants to merge 9 commits into from
Closed

Speed up Vim startup #98

wants to merge 9 commits into from

Conversation

rainux
Copy link

@rainux rainux commented Oct 21, 2011

Hi gmarik,

Here are some patches mainly focused on speed up Vim startup:

  1. Fix performance issue introduced by 55a5ef0.

    Although there are other ways to resolve this, I finally chose to introduce a new command BundleBind!. The benefit is not only easy to implement and has the best performance, but also make it easy to implement below features. However, the command name "BundleBind!" may not be a good choice and can be changed. I haven't chose "BundleLoad!" because it doesn't really load plugins when it get executed.

  2. Use a bind option to control whether bind a scripts bundle (thus it will be loaded by Vim in the later stage of startup) at Vim startup.

    This can be use on slower and rarely used scripts.

  3. Manually bind and load those scripts not bound at Vim startup with BundleBind!.

    It accept tags as arguments which can be set by Bundle command.

Please review and consider pull them :)

@rainux
Copy link
Author

rainux commented Oct 21, 2011

Oh, just forgot, the syntax of define options for Bundle command looks a bit ugly:

Bundle 'bash-support.vim', {'bind': 0, 'tags': ['bash', 'slow']}
Bundle 'c.vim', {'bind': 0, 'tags': ['c', 'slow']}

Since the Bundle command was defined to use <args> as parameters, the above syntax is really dictionary of VimL. Maybe we can use a custom syntax to make it clean and beautiful:

Bundle 'bash-support.vim' nobind @bash @slow
Bundle 'c.vim' nobind @bash @slow

But before work on it, we should have more definitions about it.

@gmarik
Copy link
Contributor

gmarik commented Oct 21, 2011

wow! 👍
That's quite a patch! )

@gmarik
Copy link
Contributor

gmarik commented Oct 21, 2011

will review it later today, or over the weekend!
Thanks! )

@gmarik
Copy link
Contributor

gmarik commented Oct 21, 2011

BTW, how do you measure performance increase with binding scripts?

@rainux
Copy link
Author

rainux commented Oct 21, 2011

Hmm? not really understand your meaning :) What's "binding scripts"?

@gmarik
Copy link
Contributor

gmarik commented Oct 21, 2011

well,

@rainux
Copy link
Author

rainux commented Oct 21, 2011

OK, it's clear.

  • bind option for Bundle command just intend to disable slower bundles be bound (means not add their runtimepath to Vim's &runtimepath), thus Vim will not load these bundles at startup. Although it useful for slower bundles such as c.vim, it isn't the solution to the performance issue I've mentioned with BundleBind! command.

  • I have put all vundle related commands to a vundle.vim like this:

    " Initialize vundle
    set runtimepath+=~/.vim/bundle/vundle
    call vundle#rc()
    
    " Bundles:
    " Generally Useful:
    Bundle 'git://github.com/rainux/vundle.git'
    Bundle 'git://github.com/kana/vim-textobj-user.git'
    ...
    BundleBind!
    

    Then source it at the top of ~/.vimrc. After that use this shell command to easily check time spent on vundle.vim:

    vim -c q --startuptime vim.log ; grep vundle\.vim vim.log
    

@gmarik
Copy link
Contributor

gmarik commented Oct 21, 2011

I see, thanks for details.
I'll play with it!

@rainux
Copy link
Author

rainux commented Oct 22, 2011

Sorry, just forget to provide a way to notice existing users update their vundle configuration to add the command BundleBind!, I'll work on this later.

@rainux
Copy link
Author

rainux commented Oct 23, 2011

Hey gmarik, after thinking on how to notice existing users use BundleBind!, I realized it's not necessary, we can just call it within a vundle/plugin/vundle.vim file. The bundles load behavior and orders of loading are exactly the same. I've rebased the branch to update this, you can only review the rebased commits showing right above this comment.

@rainux
Copy link
Author

rainux commented Oct 23, 2011

Oops, the new approach doesn't works exactly the same compare to the original way.

The problem here is, now we update 'runtimepath' in a plugin script which will be loaded after execute .vimrc, that means if user try to immediately reference any files provided by a scripts bundle in .vimrc, the attempt will be failed. For example use a color scheme in a scripts bundle.

So we have 2 options:

  • Run BundleBind! in vundle/plugin/vundle.vim, make features of scripts bundles partly available. Then show a warning message to tell our users they should add BundleBind! after last Bundle command in their .vimrc, to ensure all features of scripts bundles available.
  • Vundle itself doesn't run BundleBind! , just warn users they must add BundleBind! to make Vundle functional thus all their scripts bundles available.

How do you think?

@gmarik
Copy link
Contributor

gmarik commented Oct 24, 2011

Hey @rainux, so I looked into the code.

Here are my thoughts:

  • making vim startup time shorter is GOOD
  • added complexity - BAD

my main concern is with the complexity that gets added to get startup time improvements.

User has to tag scripts, and then manually BundleBind to get scripts loaded.
This seems like too much hassle(manual involvement) to me.
Fixing 1 time thing (startup) we're adding many more (like BundleBinding required scripts once they're used)

For instance in my case i don't start Vim often because i'm using --remote-tab-silent option.
So i'll get no benefit along with complexity (

But if you do consider this is important feature to have we could make Vundle "API" so you can this working as plugin.
Say vundle-lazy-loading.

Also i think you saw this #12 which is cool idea to play with.

Please let me know what you think.
(And sorry for being skeptical...)

@rainux
Copy link
Author

rainux commented Oct 24, 2011

Hey @gmarik, it looks like you miss-understand these patches.

Logically they are two parts, patches before "Introduce BundleBind! command to speed up Vim startup" (include itself) are really fixing the performance issue. The issue is kind of regression/trade-off for keep bundles get loaded same as the declaration order. The only thing this part added is the new BundleBind! command. Sure we should try fix issue by not introduce new feature/concept as possible as we can, but when the concept/complexity is just add a simple command without any argments to the configuration, and the benefit is clean, simple and efficient implementation, IMHO it is worth. That's why I finally chose this solution.

Also as I've mentioned in above comment, in most case we can call the BundleBind! command automatically and it works as before. The only case is if user's .vimrc try to reference a file which provided by a bundle, the attempt will be failed. In both case we can display a warning message to make user aware this implementation change.

The second part which implement manually bind slower bundles via tags is kind of "deep optimization of the Vim startup process", and it should be consider experimental due to the ugly syntax of tags definition. I pushed them because they also related to the "Speed up Vim startup" topic, and want to discuss here to work out a better "user interface". Actually the BundleBind! command also should accept name of a bundle as arguments, but it should be a different form compare to using tags, like:

BundleBind! c.vim
BundleBind! @slower

At last I don't think a simple feature like this is worth to be made as plugin. IMHO The benefit of plugin is user can turn on/off it, developer can separate complex code. But this feature is just about 40 lines code added 22 lines code deleted about the core behavior, and it is "off" by default. If user don't touch their configuration, everything works as before. What's the point to make it a plugin?

@gmarik
Copy link
Contributor

gmarik commented Oct 25, 2011

Ok, i'll give it another try then.
Could you please split it into separate pull requests then?
There are at least 3 i think:

  • the 2 ones you mentioned and
  • code formatting changes.

@rainux
Copy link
Author

rainux commented Oct 30, 2011

I just got another idea to improve this:

If we force use a separated config file like mentioned in #56, then the BundleBind! is not necessary and can be removed, this also make vundle acts more like bundler for Ruby.

The new usage should like:

# Add this to beginning of ~/.vimrc
set runtimepath^=~/.vim/bundle/vundle
call vundle#rc()

In vundle#rc() function we can read configuration from a config file like ~/.vim/Vundlefile, parse all the config commands like Bundle 'rails.vim', keep the declaration order of bundles, then finally update the &runtimepath. We can even use a custom DSL in Vundlefile, instead of VimL's ugly syntax for dictionaries. Everything is simple and clean, the only cost is, break the backward compatibility. I think it's OK since Vundle still in 0.x.

@gmarik
Copy link
Contributor

gmarik commented Nov 4, 2011

I'd do it like this

" Initialize vundle
set runtimepath+=~/.vim/bundle/vundle
call vundle#rc()

"tell Vundle not to modify runtimepath until BundleLoad
BundleLazy

" Bundles:
" Generally Useful:
Bundle 'rainux/vundle'
Bundle 'kana/vim-textobj-user'

BundleLoad

For someone concerned with startup.

I'd not modify default (slower but easier to setup) behaviour

it's same thing but BundleBind is somewhat confusing, atleast to me

@gmarik
Copy link
Contributor

gmarik commented Nov 4, 2011

Yeah and separate config file makes lots of sense in this case...

with
BundleFile ~/.vim/bundles.vim

Lazy rtp modification would be default

@rainux
Copy link
Author

rainux commented Nov 4, 2011

@gmarik Yeah I also mentioned "BundleBind" is not a good/clear name and can be changed.

Don't have interest to add yet another command, I think I'll give up this pull request and implement the separated config file in my fork. It still possible to provide backward compatibility in this approach, e.g. if there's no Vundlefile, keep current behavior.

@rainux rainux closed this Nov 4, 2011
@gmarik
Copy link
Contributor

gmarik commented Nov 4, 2011

Yeah, agreed adding extra command isn't a clear move

How about using special init function

" Initialize vundle
set runtimepath+=~/.vim/bundle/vundle

"tell Vundle not to modify runtimepath until BundleLoad
call vundle#rc().lazy()

" Bundles:
" Generally Useful:
Bundle 'rainux/vundle'
Bundle 'kana/vim-textobj-user'

BundleLoad

@rainux
Copy link
Author

rainux commented Nov 4, 2011

Essentially there's no difference between "add more command" and "add another init function", users still need to understand it then use it correctly to make it work. Why not we provide one and only one simple approach to resolve the problem clearly?

@gmarik
Copy link
Contributor

gmarik commented Nov 4, 2011

Because it changes nice, default behaviour?

Was thinking that we don't even need special command for that.
If ppl are concerned with speed - extract into a separate file and use BundleFile or whatever name it'll be.

@rainux
Copy link
Author

rainux commented Nov 4, 2011

Sure no special command is necessary, for the "only one simple approach" I mean use a separated config file like "Vundlefile".

@rainux
Copy link
Author

rainux commented Nov 5, 2011

It's very clear an O(2n - 1) algorithm introduced in 55a5ef0 is not a "nice behavior".

I have never saw an O(2n - 1) algorithm be used at anywhere.

@gmarik
Copy link
Contributor

gmarik commented Nov 5, 2011

Ok "nice behavior" isn't really a quality.
Simplicity is.
But, the algorithm might be improved.

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.

2 participants