Concurrency safe `FileImporter`
Opened this issue · 6 comments
We embed the jsonnet VM in our CI/CD infrastructure and instantiate a VM per generation cycle, but found that when using the same VM instance for all generations it's quite a bit faster (I expect because of caching as many of the generations use the same files). However, at the same time we found the FileImporter to be not concurrency safe. Would you be open to adding locking to the file importer?
As I was writing this it also occurred to me, is the jsonnet VM otherwise concurrency-safe? This particular issue we found quickly during development, but I'm curious if it would be more work than just making FileImporter concurrency safe.
@sbarzowski would know better but I think we didn't make it thread safe at all. It might be possible to put locks throughout the VM. I think our reasoning was that the language itself was single threaded so there was no point allowing a VM to process two executions at once. What if you wrapped the whole thing in a single global lock, or just serialised your interactions with it?
That would mean we undo any gain we get from concurrently generating, which is very significant.
If the gains you got were from cacheing then you'd still get those gains on account of the cache being maintained between runs
That's separate, we both run generation per "stack" concurrently, and when we did that we saw an additional improvement when using the same runtime. We could write our own importer, but if the rest of the runtime is not concurrency safe then it's of little use.
I see.
Any chance you can look at go race detector to see if it's only a small number of places that need protecting?
Another option might be to create n VMs but have them all accessing the same import cache, which is a smaller unit of code to protect.
I can't believe I haven't thought of that, agreed, go race detector should be able to catch this with sufficient amounts of iterations. Will try!