Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

enhansed threading (aka 'threads_ex') is broken #2792

Open
klirichek opened this issue Nov 26, 2024 · 0 comments
Open

enhansed threading (aka 'threads_ex') is broken #2792

klirichek opened this issue Nov 26, 2024 · 0 comments
Assignees

Comments

@klirichek
Copy link
Contributor

Usual threading behavior tries to occupy as many idle cores as it can.
When concurrency is limited by given 'threads=' option, we tried to not exceed given limitation.

This behavior was once changed: when scaling pseudo-sharding behavior, new param 'active threads' arrived. That is, threads which right now do something. Knowing size of thread-pool we also can achieve N of idle threads and use this knowledge for scheduling. Changed thing here is that N of idling threads are instance-wide; so if we have limited concurrency, if will be much more limited by N of idling threads. I.e. if we have 5 threads per query, but only 2 iddling threads - concurrency will be limited to 2.

Then behavior was changed (actually, broken) again. Now 'threads=' doesn't actually limit N of threads available for given query, but can spread over many cores, > concurrency.
That change is made by considering N of threads available for 'further processing'. Point for calculation is - when we query a disk chunk, can we parallelize (pseudo-share) the query, and how many threads we can occupy?
Previous behavior came from concurrency and N of working threads. Actually, if N of working (non-idle) threads exceeds concurrency - we can't parallelize. Good part here is that we obey concurrency. Bad clause is that N is instance wide, and it is easy to happen, that other queries are occupying threads right now, and because of them, we can't run more because of our concurrency. That is, say, we have 64 cores, and threads=4. If other 4 queries are running right now (and occupy 4 cores) - we just don't run our query in parallel because of it.

That behavior is definitely erroneous, and it should be fixed on the stage when N of running threads were introduced. That is - we should count threads belonging to current query first, to don't limit ourselves because of others. At least until we have enough idle threads.

That behaviour was overriden by another erroneous solution, that is: we don't limit when N of idling threads is enough to run with full given concurrency. That is counter-versa solution, which is opposite error: now we ignore N of working, until N of idling became too low. That is, say, we have 64 cores, 8 disk chunks, and threads=8. We run parallel query over all 8 disk chunks a time. They all working, so now we have 48 free cores. Then each subroutine, in turn, see 48 idle threads, and concurrency of 8, and see, it can run 8 pseudosharding tasks in parallel. Finally we finish with all 64 cores occupied (because every of them doesn't regard already occupied cores until it see enough idle to occupy all according threads param).

Enhansed threading (aka 'threads_ex') was introduced to override any hardware N of threads and scheduling, so that with provided 'threading model' every query behave finally fully reproducable on any instance ignoring N of actual cores available. That is - you can investigate abnormal query result taken from 128-cores machine on your tiny laptop with single-core processor.

That is implemented via 'scheduler', which provides the concurrency, and strict sequence of tasks which will be provided to each thread.

Thing was broken with latest change, since now we have N of idling threads in game, which rules the things outside the scheduler.

To fix it, we need to move calculation of possible parallel continuations to scheduler. That is, release scheduler can work anyway (that is - keep or break any rules). However, when we explicitly assign N of threads and N of batch - let this scheduler obey given numbers, and never loop into actually available N of cores, etc.)

Secondary (but most error-prone) task - is ti reimplement occupied-threads on per-query way, so that scheduler can make decision, using N = idle cores (instance-wide), M = cores, occupied by THIS query, C = concurrency for THIS query. So, that is threads for parallel execution should be min (C-M, N) (right now formulae is min (C,N). That way we will avoid breaking of 'threads=N' behavior.

klirichek added a commit that referenced this issue Dec 4, 2024
That is related to #2792, however doesn't address inter-scheduling
between idx and it's pseudoshards
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant