Only use at most a single thread for search context freeing#111156
Only use at most a single thread for search context freeing#111156original-brownbear merged 1 commit intoelastic:mainfrom original-brownbear:limit-concurrent-free-context
Conversation
Forking to `GENERIC` makes sense here since we sporadically block for a macroscopic amount of time to protect transport threads, but in almost all cases the operation operates on the same data structures and is very fast. Since it's also very frequent, we shouldn't be creating a bunch of generic threads during a burst -> lets throttle to a single thread.
|
Pinging @elastic/es-search (Team:Search) |
|
Pinging @elastic/es-search-foundations (Team:Search Foundations) |
DaveCTurner
left a comment
There was a problem hiding this comment.
With this change we should be able to wait for all closes to complete in tests without an assertBusy(): just send another task down the queue and wait for it to complete.
Right on a per-node basis this is true, but needs new code in production I guess? |
Looking into actually doing this I wonder what this really gets us? It' a couple extra lines of code and in fact a slowdown for almost every test suite since we very rarely leak the generic task here. Also, it's not good enough for the multi-node case I believe where the task can be sort of queued on the wire as well as on the generic pool. |
DaveCTurner
left a comment
There was a problem hiding this comment.
Looking into actually doing this I wonder what this really gets us?
Using assertBusy() when there's a more direct way to wait for the condition we want is a drag on test performance in thousands of tiny ways. Even if a direct wait is a few µs slower than the first assertBusy() iteration, if that iteration doesn't succeed once every few hundred runs then we lose out in aggregate. Also I read a need for assertBusy() as a suggestion of a deeper design issue - I've burned a lot of time fixing fire-and-forget stuff that suddenly needs to not be so forgetful.
So yeah ok we can do this here, each assertBusy() in isolation doesn't make things that much worse, it's the fact that it's used ≥2k times and counting that is the problem.
|
Thanks David!
Believe me I'm 100% with you here and have actively removed a bunch of them for that very reason. |
Forking to
GENERICmakes sense here since we sporadically block for a macroscopic amount of time to protect transport threads, but in almost all cases the operation operates on the same data structures and is very fast. Since it's also very frequent, we shouldn't be creating a bunch of generic threads during a burst -> lets throttle to a single thread.Throttler logic is taken from the indices cluster service and we can maybe deduplicate in a follow-up.
relates #83515