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

Detect Darwin with lighter method #135

Merged

Conversation

delphinus
Copy link
Collaborator

The plugin is detecting Darwin by system uname command. But the system() call is too slow, and I noticed it makes the starting method slow, also. In this PR, has('macunix') is used for detecting Darwin instead of system() call.

You can see this with +profile feature.

vim --cmd "profile start profile.txt" --cmd "profile file /path/to/plugin/webdevicons.vim" +q && vim profile.txt
SCRIPT  /path/to/plugin/webdevicons.vim
Sourced 1 time
Total time:   0.286557
 Self time:   0.001238

count  total (s)   self (s)
                            " Version: 0.7.0
                            " Webpage: https://github.com/ryanoasis/vim-devicons
                            " Maintainer: Ryan McIntyre <[email protected]>
                            " License: see LICENSE

    1              0.000003 let s:version = '0.7.0'
    1   0.284813   0.000301 let s:operatingsystem = system("uname -s")

                            " standard fix/safety: line continuation (avoiding side effects) {{{1
                            "========================================================================

...
  • Mac OS X 10.11.2
  • Vim 7.4.972 (MacVim snapshot 87)

Vim took 0.286557 second with its loading, and system("uname -s") occupied most of it, 0.284813 second.

@delphinus delphinus force-pushed the detect-darwin-with-lighter-method branch 2 times, most recently from 6480e63 to 12db550 Compare December 22, 2015 23:44
@ryanoasis
Copy link
Owner

Thanks! This looks interesting and like a good improvement. I just upgraded so the difference seems negligible to me but I will try on my old laptop.

Any ideas of has('macunix') vs has('mac') ?
seems like the former is for detecting a bit 'newer' mac os.

Ah here we go:

mac             Macintosh version of Vim.
macunix         Macintosh version of Vim, using Unix files (OS-X).

So... 'macunix' is probably the one to go with (OSX) I think 😄

Do you know anything about the reliability of this vs the system call?

I will read up on this more. So far looks like this would be a good change...

@delphinus
Copy link
Collaborator Author

Sorry, I was mistaken. has('mac') or has('macunix') means it to be built as MacVim. So both are false in a normal vim such as /usr/bin/vim even if on OS X Darwin.

After all, we have to call uname for detecting OS correctly, but it took a bit time. The starting time of my Vim doubled with this plugin. My suggestion for detecting OS logic is below...

  1. If has('macunix'), it is Darwin.
  2. If not has('macunix') and has('unix'), call uname -r.
  3. If output is Darwin, it is Darwin. If not, it is other unix system such as Linux.
  4. system() call will take a bit time. If you want to avoid this, you should set a variable (ex. g:devicons_operating_system) to detect OS without calling system().

I will rewrite PR for this logic.

@delphinus delphinus force-pushed the detect-darwin-with-lighter-method branch from 12db550 to 57eead7 Compare December 23, 2015 14:52
@delphinus
Copy link
Collaborator Author

Updated. I added a config value g:WebDevIconsUseSystemCallToDetectOS. Please review this.

@ryanoasis
Copy link
Owner

Sorry for long delay. Overall I like this change but have some questions and feedback.

The 2nd conditional seems a little odd to me. Couldn't it be eliminated if g:WebDevIconsUseSystemCallToDetectOS was used in the third conditional.

I think by default the system call should be done if we still aren't 100% from the info from feature-list

Maybe instead of g:WebDevIconsUseSystemCallToDetectOS being an on|off it could be a string to allow the user to just specify the system they are on and thus even avoid the system call when it is set. With this I think the system call will only fire if: unix file format and not mac vim and the variable not set. Also we could move the detection call so it doesn't fire for Dos fileformat. Thoughts?

@ryanoasis ryanoasis added this to the v0.8.0 milestone Feb 3, 2016
@delphinus
Copy link
Collaborator Author

Indeed. It is useless that system() call if the file is not ff=unix or if the plugin already knows OS. So I will change logic such below.

  1. I will add a variable g:WebDevIconsOS. This is not defined in default.
  2. If you open a file of ff=dos or ff=mac, it doesn't care.
  3. If you open a file of ff=unix,
    • If has('macunix'), the plugin use Apple Logo.
    • If g:WebDevIconsOS is defined, the plugin use this. When it is Darwin, use Apple Logo. When other, use Linux Logo.
    • If g:WebDevIconsOS is not defined, the plugin call system('uname -r') to detect OS.

@delphinus delphinus force-pushed the detect-darwin-with-lighter-method branch from 57eead7 to 5f3a4c5 Compare February 6, 2016 07:31
@delphinus
Copy link
Collaborator Author

Updated. Should I explain about g:WebDevIconsOS in README?

@delphinus delphinus force-pushed the detect-darwin-with-lighter-method branch from 5f3a4c5 to 467ae8e Compare February 6, 2016 07:37
@ryanoasis
Copy link
Owner

Awesome. Glad you agreed 😃

Feel free to add it to the readme, just know I might tweak it.

Thanks.

@ryanoasis
Copy link
Owner

feel free to test this out in 0.8.0 branch

Not fully tested but seems stable

@delphinus delphinus force-pushed the detect-darwin-with-lighter-method branch from 9aa854c to 6abf71f Compare February 7, 2016 23:45
@delphinus delphinus force-pushed the detect-darwin-with-lighter-method branch from 6abf71f to 46a928b Compare February 7, 2016 23:47
@delphinus
Copy link
Collaborator Author

readme updated. Please correct expression, if needed.

@ryanoasis ryanoasis merged commit 46a928b into ryanoasis:master Feb 8, 2016
@delphinus delphinus deleted the detect-darwin-with-lighter-method branch February 9, 2016 00:18
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