occivink / kakoune-snippets

Snippet support for kakoune
The Unlicense
46 stars 6 forks source link

Speed up snippets-directory-reload #33

Closed codesoap closed 5 years ago

codesoap commented 5 years ago

Since printf is no builtin in OpenBSD's sh (neither is echo), the snippets-directory-reload command was very slow. The calls of the printf command have been drastically reduced by buffering and utilizing awk.

I have tested the changes thoroughly by teeing everything the %sh{ ... } block prints into a file and comparing between the original and new state. The outputs were completely identical.

Since the printf command seems to be a builtin function in dash and the like, there is unfortunately no notable performance difference for the Linux users.

Here are some timings I recorded when using the kakoune-snippet-collection (I tried to select times, that were around the average for each scenario):

Times with OpenBSD's sh

# Not using the plugin:
$ time kak -e 'q'
kak -e 'q'  0.03s user 0.12s system 94% cpu 0.159 total

# Old version:
$ time kak -e 'q'
kak -e 'q'  1.02s user 4.57s system 83% cpu 6.684 total

# awk version:
$ time kak -e 'q'
kak -e 'q'  0.26s user 0.19s system 108% cpu 0.416 total

Times with dash

# Not using the plugin:
$ time kak -e 'q'
kak -e 'q'  0.08s user 0.16s system 134% cpu 0.178 total

# Old version:
$ time kak -e 'q'
kak -e 'q'  0.14s user 0.21s system 118% cpu 0.297 total

# awk version:
$ time kak -e 'q'
kak -e 'q'  0.10s user 0.20s system 98% cpu 0.305 total
codesoap commented 5 years ago

I just realized that the awk part is probably not even needed for the perfomance - the buffering would suffice. I initially introduced awk to speed up the doubleupsinglequotes function and didn't think about removing it after I started buffering.

I think the awk version improves readability in some respects, but if you'd prefer a version without awk I could remove it.

occivink commented 5 years ago

I like the style overall.

I'm a bit concerned that printf is not a builtin on OpenBSD's sh as I've been using it quite liberally. One of the main functions in expand.kak, snippets-expand-trigger also makes heavy use of it (in fact, I made sure that it only uses builtins) and so performance is also going to be poor on expansion. As I understand it, echo is not suitable as it's sensitive to certain input, so what's the alternative? Is it due to purity concerns that printf is not a builtin in OpenBSD?

For this particular command, ultimately I'd prefer rewriting the entire function in perl or something equally portable, so that we can get rid of the brittle globbing, but I'm happy to merge this for now.

codesoap commented 5 years ago

I'm not sure why OpenBSD's sh doesn't have the printf builtin and couldn't find any reasons in a quick search. Since the OpenBSD folks are very security focused, I think your guess that it's a purity concern might be the reason. I guess less code in sh means less potential for bugs and also suits the UNIX philosophy better...

echo isn't a builtin on OpenBSD either and was only marginally faster than printf in my tests.

I noticed, that the trigger expansion is slower with OpenBSD's sh (takes maybe 0.5s), but it's not so bad that I would consider it a blocker. I intended to take a look at the code there, though, to see if I can find room for improvement.

I'll see if I can write snippets-directory-reload as a perl script. I might take a day or two, since my perl skills are a bit rusty. I'll update this pull request when I've got a proposal or fail to write a perl implementation.

occivink commented 5 years ago

I noticed, that the trigger expansion is slower with OpenBSD's sh (takes maybe 0.5s)

Yeah that's pretty bad, luckily there should be some pretty easy gains to be had there.

I'll see if I can write snippets-directory-reload as a perl script.

You don't have to, I was just spelling my thoughts out loud. As long as we're not losing anything with this implementation I'm happy merging it. But first let me spend some time reviewing and testing this a bit.

On a similar note, I take it that there is no test builtin either?

codesoap commented 5 years ago

On a similar note, I take it that there is no test builtin either?

Doesn't look like it. Have a look at OpenBSD's sh(1) (printf, ... aren't mentioned).

You don't have to, I was just spelling my thoughts out loud.

No worries, I did't feel any pressure, but I find it a good opportunity to learn more about perl and I'm curious about the performance of a pure perl implementation.

To be honest I couldn't hold myself back and think I'm almost done with an implementation :D I'm gonna call it a night for now, though.

codesoap commented 5 years ago

I've got the pure perl implementation pushed. It didn't even get as big as I thought it would. I'm quite happy with it :-)

The performance seems to be even better than the original pure shell script implementation in combination with the builtin functions, so even the Linux users will profit this time.

Testing tip: change foreach my $snippet (@snippets) { to foreach my $snippet (sort @snippets) { to get the same ordering as in the shell script version.

occivink commented 5 years ago

Merged, thank you. I added the sort since it makes snippets-info slightly better