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

query_namespaces performance improvements #417

Merged
merged 2 commits into from
Nov 13, 2024
Merged

Conversation

jhamon
Copy link
Collaborator

@jhamon jhamon commented Nov 13, 2024

Problem

Want to improve the performance of the rest implementation of query_namespaces

Solution

  • Add pytest-benchmark dev dependency and some basic performance tests to interrogate the impact of certain changes. For now these are only run on my local machine, but in the future these could potentially be expanded into an automated suite.
  • Pass _preload_content=False to tell the underlying generated code not to instantiate response objects for all the intermediate results.
  • Use ThreadPoolExecutor instead of older ThreadPool implementation from multiprocessing. This involved some changes to the generated code, but the benefit of this approach is that you get back a concurrent.futures.Future instead of an ApplyResult which is much more ergonomic. I'm planning to extract the edited files out of the code gen process very shortly, so there shouldn't be a concern about modifying generated files in this case. I gated this approach behind a new kwarg, async_threadpool_executor, that lives alongside async_req; eventually I would like to replace all usage of async_req's ThreadPool with ThreadPoolExecutor to bring the rest and grpc implementations closer together, but I can't do that in this PR without creating a breaking change.

The net effect of these changes seems to be about ~18% performance improvement.

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update
  • Infrastructure change (CI configs, etc)
  • Non-code change (docs, etc)
  • None of the above: (explain here)

@jhamon jhamon force-pushed the jhamon/query_namespaces_perf branch from 93396c3 to 340b62f Compare November 13, 2024 15:49
@jhamon jhamon marked this pull request as ready for review November 13, 2024 15:51
Copy link
Contributor

@austin-denoble austin-denoble left a comment

Choose a reason for hiding this comment

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

Very nice work. Extracting the generated classes out so we can actually modify them is great.

The net effect of these changes seems to be about ~18% performance improvement.
🎉

Copy link
Contributor

@rohanshah18 rohanshah18 left a comment

Choose a reason for hiding this comment

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

Great work, LGTM!

@jhamon jhamon merged commit 4a99468 into main Nov 13, 2024
84 of 85 checks passed
@jhamon jhamon deleted the jhamon/query_namespaces_perf branch November 13, 2024 16:08
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.

3 participants