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

feat: compute minimum required dispatcher concurrency from the plan graph #5024

Merged
merged 5 commits into from
Jul 28, 2022

Conversation

adrian-thurston
Copy link
Contributor

@adrian-thurston adrian-thurston commented Jul 25, 2022

Compute the minimum dispatcher concurrency (number of goroutines servicing
execution graph) in a walk of the plan graph. This is:

  sum( number of predecessors of N )
  for every node N that becomes a root of the execution graph

Any amount less than this can result in dispatcher deadlock. Details can be
found in influxdata/idpe#15220.

At this time we also add goroutines to get the desired speedup in the presence
of parallelization. We aim to add a fixed amount of goroutines based on the
parallelization factor, but take into account that some goroutines might have
been added because parallel merge nodes are also roots.

Also, we allow the new feature flag queryConcurrencyIncrease to add additional
goroutines.

The existing feature flag queryConcurrencyQuota is removed, as it is unsafe to
reduce the number of goroutines below the computed minimum.

Since the parallelization planner rules are no longer directly specifying the
desired concurrency, we can now remove the concurrency and max allocation
fields from the dependencies. This was in the context as a method of passing
concurrency requirements from a planner rule to the execution engine. The max
allocation was never required to be in the context, it just followed along with
the query concurrency so all resource limits could be defined in the same
place.

To merge this into IDPE we simply need to REMOVE the blocks of code
that look like this from any planner rules.

       if execute.HaveExecutionDependencies(ctx) {
               execDeps := execute.GetExecutionDependencies(ctx)
               execDeps.ExecutionOptions.ConcurrencyLimit = factor * 2
       }

@adrian-thurston adrian-thurston requested a review from a team as a code owner July 25, 2022 23:00
@adrian-thurston adrian-thurston requested review from onelson and removed request for a team July 25, 2022 23:00
Copy link
Contributor

@onelson onelson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

Though as an aside, I wonder how this accounting plays with tableFind et al.

I recently had to patch around the behavior for yields in the input to table functions (ref: #4869) such that they are ignored within the sub-query. I don't recall if the sub-query's sub-plan would share attributes or what the impact might be.

@@ -20,10 +19,8 @@ type key int
const executionDependenciesKey key = iota

type ExecutionOptions struct {
OperatorProfiler *OperatorProfiler
Profilers []Profiler
DefaultMemoryLimit int64
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was the memory limit only of interest wrt the concurrency scaling?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

Though as an aside, I wonder how this accounting plays with tableFind et al.

I recently had to patch around the behavior for yields in the input to table functions (ref: #4869) such that they are ignored within the sub-query. I don't recall if the sub-query's sub-plan would share attributes or what the impact might be.

That's a very good point. I will investigate that before merging.

Comment on lines -381 to +483
es.resources.MemoryBytesQuota = defaultMemoryLimit
es.resources.MemoryBytesQuota = math.MaxInt64
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah. I see.

@adrian-thurston
Copy link
Contributor Author

@onelson with regard to tableFind we should be operating correctly.

Yields are skipped at the flux-expression level and there can only be a single flux pipeline in the tableFind expression. So when the plan is produced there should only be a single root in the graph. If it gets parallelized during planning then the concurrency will increase. Otherwise, it should have a concurrency quota of 1.

As an aside to the aside, we should probably watch out for problems with parallelization inside tableFind. I can't imagine there are problems, but it might be a good idea to search for issues before they arise.

…raph

Compute the minimum dispatcher concurrency (number goroutines servicing
execution graph) in a walk of the plan graph. This is:

  sum( number of predecessors of N )
  for every node N that becomes a root of the execution graph

Any amount less than this can result in dispatcher deadlock. Details can be
found in influxdata/idpe#15220.

At this time we also add goroutines to get the desired speedup in the presence
of parallelization. We aim to add a fixed amount of goroutines based on the
parallelization factor, but take into account that some goroutines might have
been added because parallel merge nodes are also roots.

Also, we allow the new feature flag queryConcurrencyIncrease to add additional
goroutines.

The existing feature flag queryConcurrencyQuota is removed, as it is unsafe to
reduce the number of goroutines below the computed minimum.

Since the parallelization planner rules are no longer directly specifying the
desired concurrency, we can now remove the concurrency and max allocation
fields from the dependencies. This was in the context as a method of passing
concurrency requirements from a planner rule to the execution engine. The max
allocation was never required to be in the context, it just followed along with
the query concurrency so all resource limits could be defined in the same
place.
Put new flag queryConcurrencyIncrease at the end (best I can tell is the
convention). Added Jonathan as point of contact.
@adrian-thurston adrian-thurston force-pushed the feat/minimum-dispatcher-concurrency branch from b4648f5 to 0296c5e Compare July 28, 2022 17:57
@adrian-thurston adrian-thurston merged commit ec80cc3 into master Jul 28, 2022
@onelson
Copy link
Contributor

onelson commented Jul 29, 2022

Great. tableFind is an anomaly as far as flux execution goes, so it makes sense to give it a little extra scrutiny. Thanks for checking it out for this.

@jacobmarble jacobmarble deleted the feat/minimum-dispatcher-concurrency branch January 4, 2024 17:04
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

Successfully merging this pull request may close these issues.

2 participants