lewis6991 / impatient.nvim

Improve startup time for Neovim
MIT License
1.19k stars 28 forks source link

Introduce cachepack module to optionally replace mpack usage #17

Closed chase closed 3 years ago

chase commented 3 years ago

I've created a module that serializes and deserializes the cache using only LuaJIT's FFI and no requirement of mpack.

It is usually as fast or sometimes faster than mpack.

This PR makes it the default with the ability to load using mpack by using _G.use_cachepack = true before impatient.nvim is setup.


I think it would be reasonable to drop the mpack requirement if testing goes well among other users, that way we can revert the tests to the original ones.

clason commented 3 years ago

Would that be of interest as an alternative to https://github.com/neovim/neovim/pull/15566?

lewis6991 commented 3 years ago

This is just a highly specialized subset of mpack and as such isn't really a viable alternative. neovim/neovim#15566 is a better solution for general use.

clason commented 3 years ago

Sorry, that was poorly phrased; it's not a replacement, if only for the lack of C access. I meant it the other way around: would you still prefer to use this when that PR is merged?

lewis6991 commented 3 years ago

Probably. I'd expect general performance to be similar, but without the need to load an additional lua module.

shadmansaleh commented 3 years ago

neovim crashes with bus error with this on Android ;|

Atlest I can keep using mpack there .

chase commented 3 years ago

neovim crashes with bus error with this on Android ;|

@shadmansaleh That's quite the edge case :laughing: From what I can tell, that's a segmentation fault. Does your neovim build use LuaJIT?

shadmansaleh commented 3 years ago

Yes with luajit. I'm using termux as terminal. They have 0.5 in their repo but I'm using neovim master build . Though pretty sure the 0.5 build in their repo will act the same since both are built in same way.

lewis6991 commented 3 years ago

Is there any way we can check the user is running android? If so, then we can default to mpack.

chase commented 3 years ago

Is there any way we can check the user is running android? If so, then we can default to mpack.

Just installed termux (very neat tool, actually) to try this out. We can probably use this to check if it's running on Android: os.getenv('ANDROID_DATA') ~= nil

shadmansaleh commented 3 years ago

https://github.com/termux/termux-packages/issues/2346 is open for a while . I could try to make a patch for termux so has('android') works there .

We can use an environment variable like chase suggested too .

Though if there isn't significant performance difference between mpack and cachepack, can we use cachepack as fallback when mpack isn't available ? I feel like over all mpack will be more stable. Also libmpack-lua is getting included by neovim anyway in near future.

chase commented 3 years ago

termux/termux-packages#2346 is open for a while . I could try to make a patch for termux so has('android') works there .

We can use an environment variable like chase suggested too .

Though if there isn't significant performance difference between mpack and cachepack, can we use cachepack as fallback when mpack isn't available ? I feel like over all mpack will be more stable. Also libmpack-lua is getting included by neovim anyway in near future.

The purpose is not to require mpack at all, because when libmpack-lua lands in neovim 0.6 and 0.6 is released as stable then impatient.nvim will also no longer be required.

By in large most platforms function with cachepack just fine on the current stable release of neovim. If anyone doesn't want to use cachepack (like you) you can simply use _G.use_cachepack = false before you require('impatient')

lewis6991 commented 3 years ago

The purpose is not to require mpack at all, because when libmpack-lua lands in neovim 0.6 and 0.6 is released as stable then impatient.nvim will also no longer be required.

That's not necessarily true. There are currently no concrete plans for a lua cache to be included in 0.6 (yet).

What I expect it is that impatient will drive some lower level changes in core, like vendoring libmpack, increasing the performance of nvim_get_runtime_file(), cacheing top level runtime directory paths, restoring the preload module, adding lua module loading to startuptime, etc. A luacache would likely be the last step to this.

Cachepack uses some pretty light use of the FFI. I'm interested on why this doesn't work on android. Is the FFI module borked or something?

chase commented 3 years ago

That's not necessarily true. There are currently no concrete plans for a lua cache to be included in 0.6 (yet).

What I expect it is that impatient will drive some lower level changes in core, like vendoring libmpack, increasing the performance of nvim_get_runtime_file(), cacheing top level runtime directory paths, restoring the preload module, adding lua module loading to startuptime, etc. A luacache would likely be the last step to this.

Fair enough :sweat_smile:

Cachepack uses some pretty light use of the FFI. I'm interested on why this doesn't work on android. Is the FFI module borked or something?

Termux is a jailed environment under a chroot on a non-standard distro running inside of a sandboxed app on Android, I'm certain there are plenty of things that can go wrong there.

chase commented 3 years ago

@shadmansaleh I've modified the LunarVim fork of impatient to use mmap instead of relying on ffi.new (which is likely causing the bounds issue): https://github.com/chase/LunarVim/commit/1107a3457016e60546ebc2302a0d84d30123ac3e

image

@lewis6991 These changes are a little more extreme, but if you're comfortable with some of the Linux-specifc code and such, this also gives a minor performance gain

lewis6991 commented 3 years ago

I'm all for it as long as it doesn't break anything.

shadmansaleh commented 3 years ago

If anyone doesn't want to use cachepack (like you) you can simply use _G.use_cachepack = false before you require('impatient')

Of course I'm already using that.

Cachepack uses some pretty light use of the FFI. I'm interested on why this doesn't work on android. Is the FFI module borked or something?

I'm interested in that too . ffi does seem to work correctly . I didn't find where it didn't work before this.

I tried dumping data while it's unpacking it works for a while they dies in the middle.

Also strangely cachepack works just fine if I use int instead of double :P

Termux is a jailed environment under a chroot

Not exactly a chroot . It uses regular android filesystem. But since it by default doesn;t have access to root user any regular user based restrictions from unix applies to termux. Though I agree a lot can go wrong .

chase commented 3 years ago

If anyone doesn't want to use cachepack (like you) you can simply use _G.use_cachepack = false before you require('impatient')

Of course I'm already using that.

Great :smiley:

Also strangely cachepack works just fine if I use int instead of double :P

Honestly, that's incredibly strange. LuaJIT handles numbers as double when passing to through FFI Highly unlikely this is the root cause, but are you using 32-bit ARM?

shadmansaleh commented 3 years ago

32-bit ARM?

Yep . humm I haven't yet tested on the 64bit one could be isolated to 32bit process.

Also I'm wondering why we need ffi ? Can't we serialize and deserialize directly from lua ?

chase commented 3 years ago

32-bit ARM?

Yep . humm I haven't yet tested on the 64bit one could be isolated to 32bit process.

Probably means invalid truncation, unfortunately I cannot test since I don't own any 32-bit Android devices.

Also I'm wondering why we need ffi ? Can't we serialize and deserialize directly from lua ?

No. It's incredibly slow.

shadmansaleh commented 3 years ago

Probably means invalid truncation, unfortunately I cannot test since I don't own any 32-bit Android devices.

Did you manage to replicate the crash on your device ?

No. It's incredibly slow.

Ok that's valid.

chase commented 3 years ago

Did you manage to replicate the crash on your device ?

Afraid not. Did try a few things, although the mmap bit seems to load things faster it doesn't seem to be the fix since my original code works just fine. I am using stable nvim however, using pkg install neovim.

So far the only place where it seems not to work is on your Android device :shrug:

shadmansaleh commented 3 years ago

So far the only place where it seems not to work is on your Android device 🤷

Ok then you can stop worrying about it . I'll let you know if I find something.

shadmansaleh commented 3 years ago

Probably of topic sorry.

@lewis6991 since libmpack-lua is now exposed through vim.mpack in master . Can we add a check for that and use vim.mpack if that exists ?

lewis6991 commented 3 years ago

Yep