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

[RDY] os: implement VimL libcall with {mch,os}_libcall #802

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@aktau
Copy link
Member

aktau commented Jun 2, 2014

The old mch_libcall was removed from neovim. This is a partial
reimplementation on top of libuv. It doesn't catch exceptions (windows) nor
signals (unix) though, so it's quite a bit more prone to crashing if the
loadable library throws an exception or crashes. Still, it should be fine
for well-behaved libraries. Requested by @Shougo.

@aktau

This comment has been minimized.

Copy link
Member

aktau commented Jun 2, 2014

@justinmk @tarruda this is the PR of the changes I mentioned in issue #795.

@tarruda

This comment has been minimized.

Copy link
Member

tarruda commented Jun 2, 2014

@Shougo would you like to test this branch?

@justinmk

This comment has been minimized.

Copy link
Member

justinmk commented Jun 2, 2014

mch_libcall could be removed I think

@aktau

This comment has been minimized.

Copy link
Member

aktau commented Jun 2, 2014

mch_libcall could be removed I think

Done, and rebased against master.

@Shougo

This comment has been minimized.

Copy link
Contributor

Shougo commented Jun 2, 2014

@Shougo would you like to test this branch?

I tested your branch and worked vimproc and vimshell.
Thank you!!

@aktau

This comment has been minimized.

Copy link
Member

aktau commented Jun 2, 2014

I tested your branch and worked vimproc and vimshell.

Good news!

@tarruda

This comment has been minimized.

Copy link
Member

tarruda commented Jun 2, 2014

👍

@aktau aktau changed the title [RFC] os: implement VimL libcall with {mch,os}_libcall [RDY] os: implement VimL libcall with {mch,os}_libcall Jun 2, 2014

@Shougo

This comment has been minimized.

Copy link
Contributor

Shougo commented Jun 2, 2014

I benchmarked your branch by vimproc's benchmark.vim.

  • system() is faster than Vim. Almost 0.310...(Vim) vs 0.207...(neovim)
  • vimproc#system()'s(libcall) performance is almost same with Vim.
@aktau

This comment has been minimized.

Copy link
Member

aktau commented Jun 2, 2014

system() is faster than Vim. Almost 0.310...(Vim) vs 0.207...(neovim)

I suspect that's either libuv's or @tarruda's magic at work, nothing to do with libcall ;).

vimproc#system() performance is almost same with Vim.

I reckoned that, I suspect most of the work is in the loading and unloading of the library. If that's the case and it needs to be faster, we could conceivably keep loaded libraries in a dictionary, but that would increase the complexity and I don't think it's worth it for a feature that's not often used. The same speed sounds good for this one, don't you think?

@Shougo

This comment has been minimized.

Copy link
Contributor

Shougo commented Jun 2, 2014

The same speed sounds good for this one, don't you think?

Not bad for me.

https://groups.google.com/d/msg/vim_dev/F2z5I9tayKY/UNGZ1-kSvq4J
https://groups.google.com/forum/#!topic/vim_dev/kS8BkiqqwyY

But if neovim supports FFI, it will be good.

@tarruda

This comment has been minimized.

Copy link
Member

tarruda commented Jun 2, 2014

system() is faster than Vim. Almost 0.310...(Vim) vs 0.207...(neovim)

That is because of the new implementation of os_call_shell on top of libuv. I bet that will be even faster once system is refactored to use pipes instead of temporary files as explained in #473

@Shougo

This comment has been minimized.

Copy link
Contributor

Shougo commented Jun 2, 2014

That is because of the new implementation of os_call_shell on top of libuv. I bet that will be even faster once system is refactored to use pipes instead of temporary files as explained in #473

It is good news for other plugins!

@Hinidu

This comment has been minimized.

Copy link
Contributor

Hinidu commented Jun 4, 2014

There are some uses of FEAT_LIBCALL. I think we should define it in config.h.in or remove entirely that has('libcall') will return true.

@aktau

This comment has been minimized.

Copy link
Member

aktau commented Jun 4, 2014

There are some uses of FEAT_LIBCALL. I think we should define it in config.h.in or remove entirely that has('libcall') will return true.

I'm not sure if I understand you. With this PR, libcall would be implemented for all platforms, which means that has('libcall') should always return true. (if that's even a thing). I agree that we should remove FEAT_LIBCALL from the source though, as it's no longer necessary to #ifdef.

@Hinidu

This comment has been minimized.

Copy link
Contributor

Hinidu commented Jun 4, 2014

has('libcall') will return false because in definition of f_has in eval.c we have now:

#ifdef FEAT_LIBCALL
    "libcall",
#endif

So we should remove FEAT_LIBCALL here and everywhere or define it in config.h.in.

@aktau

This comment has been minimized.

Copy link
Member

aktau commented Jun 4, 2014

So we should remove FEAT_LIBCALL here and everywhere or define it in config.h.in.

Ok, I just Ag'ed over the source. I think the best thing is to just remove the FEAT_LIBCALL #ifdef's everywhere and to make libcall like any other non-conditional vim function.

@Hinidu

This comment has been minimized.

Copy link
Contributor

Hinidu commented Jun 4, 2014

👍

@aktau

This comment has been minimized.

Copy link
Member

aktau commented Jun 4, 2014

@Hinidu done, can you review that?

To all others: I added the EMSG() that used to be in mch_libcall to libcall_common. It seems to me that these things should be in the vim layer, not the os layer.

@Hinidu

This comment has been minimized.

Copy link
Contributor

Hinidu commented Jun 4, 2014

aktau@5934529 is ok 👍

@Hinidu

This comment has been minimized.

Copy link
Contributor

Hinidu commented Jun 7, 2014

@aktau while working on #814 I found that in this PR you delete all occurences of USE_DLOPEN except one:

/* shared library access */
#if defined(HAVE_DLFCN_H) && defined(USE_DLOPEN)
# include <dlfcn.h>
#endif

So I think we can delete that #include in this PR too if you don't mind.

aktau added some commits Jun 2, 2014

os: implement VimL libcall with {mch,os}_libcall
The old mch_libcall was removed from neovim. This is a partial
reimplementation on top of libuv. It doesn't catch exceptions (windows) nor
signals (unix) though, so it's quite a bit more prone to crashing if the
loadable library throws an exception or crashes. Still, it should be fine
for well-behaved libraries. Requested by @Shougo.
os: remove legacy mch_libcall
Remove as much leftover cruft as possible. Tried to see which globals are
now not used anymore.
libcall: remove libcall ifdefs
Remove all the legacy code that related to mch_libcall in some way.
os_libcall is implemented on top of libuv now.
@aktau

This comment has been minimized.

Copy link
Member

aktau commented Jun 7, 2014

So I think we can delete that #include in this PR too if you don't mind.

I don't mind at all, thanks for noticing. I have removed those lines, rebased for good measure and force-pushed.

@Hinidu

This comment has been minimized.

Copy link
Contributor

Hinidu commented Jun 7, 2014

I don't mind at all, thanks for noticing. I have removed those lines, rebased for good measure and force-pushed.

Thank you, good work 👍

@justinmk

This comment has been minimized.

Copy link
Member

justinmk commented Jun 28, 2014

Merged

@justinmk justinmk closed this Jun 28, 2014

@Shougo

This comment has been minimized.

Copy link
Contributor

Shougo commented Jun 28, 2014

Great!

@aktau aktau referenced this pull request Jul 3, 2014

Merged

[WIP] July Newsletter #52

4 of 4 tasks complete

dwb pushed a commit to dwb/neovim that referenced this pull request Feb 21, 2017

clangcheck: removed flags not accepted by clang-check (neovim#802)
The extra cmd line flags "-- -Wall -Wextra" are not accepted and
processed by clang-check, moreover they actually prevent it from working
correctly.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment