tpope/vim-classpath

Run maven et al. in jobs if available

Opened this issue · 5 comments

This would greatly alleviate startuptime costs when jobs are available, at the penalty of not syncing path with the results until after the job completes.

Thoughts? I think I can work up a start.

I think from about here

to the end of the function would need some changes; when we don't have jobs, we proceed as normal. When we do, we want to recreate this logic with a job call-back which also assigns 'path' on the correct buffer. The original function would probably have to return &l:path unmodified

tpope commented

This would greatly alleviate startuptime costs when jobs are available, at the penalty of not syncing path with the results until after the job completes.

Changing the behavior from deterministic to nondeterministic is a massive penalty. I could maybe get behind using the stale cached version while the new one is computed (though that only mitigates the nondeterminism) and/or providing an opt-in async API to give users enough rope to hang themselves.

When we do, we want to recreate this logic with a job call-back which also assigns 'path' on the correct buffer. The original function would probably have to return &l:path unmodified

Am I reading this correctly that you want to provide backwards compatibility by returning a bogus value? I do not approve. My initial inclination is to add an optional callback argument to classpath#detect(), but depending on how things shake out a new function may be more appropriate. Don't sweat this part; compared to jobstart()/job_start()/system() trifecta that needs to be implemented, ironing out the API will be a piece of cake. Feel free to start with an async only backwards incompatible implementation and we can decide from there how to proceed. I'm also not opposed to dropping Vim 7 support entirely.

I could maybe get behind using the stale cached version while the new one is computed (though that only mitigates the nondeterminism) and/or providing an opt-in async API to give users enough rope to hang themselves.

Agreed, fair enough. Not sure what the "opt-in async API" should look like—a variable that says "prefer aysnc over sync"?

Don't sweat this part; compared to jobstart()/job_start()/system() trifecta that needs to be implemented, ironing out the API will be a piece of cake. Feel free to start with an async only backwards incompatible implementation and we can decide from there how to proceed. I'm also not opposed to dropping Vim 7 support entirely.

Well, that certainly simplifies making a prototype :)

I do agree that doing something bogus is not good. One option you mentioned is the stale cache—but what happens when this doesn't exist? Probably for a prototype I would just leave path alone until the job gives us what we need.

jobstart()/job_start()

Argh, I forget about neovim everytime... I take it compatibility is a goal?

tpope commented

Agreed, fair enough. Not sure what the "opt-in async API" should look like—a variable that says "prefer aysnc over sync"?

I was thinking exposing a lower level function that the user could invoke directly, but we'd still need an option to disable the built-in behavior. Cross that bridge when we come to it.

Don't sweat this part; compared to jobstart()/job_start()/system() trifecta that needs to be implemented, ironing out the API will be a piece of cake. Feel free to start with an async only backwards incompatible implementation and we can decide from there how to proceed. I'm also not opposed to dropping Vim 7 support entirely.

Well, that certainly simplifies making a prototype :)

I do agree that doing something bogus is not good. One option you mentioned is the stale cache—but what happens when this doesn't exist? Probably for a prototype I would just leave path alone until the job gives us what we need.

The first time should always block, probably.

jobstart()/job_start()

Argh, I forget about neovim everytime... I take it compatibility is a goal?

Story of my life. Neovim does need to be supported. You could keep the system() version around for that but the end result would probably going be more complicated than just supporting both job APIs.

Alright, I'll at least mock-up a function that does one of the three, and we can go from there?

The first time should always block, probably.

Not sure I follow: are you saying if there's no cache, do a blocking load (system) no matter what?