fix: init mp pool & grpc client once, use os.sched_setaffinity #1751
Conversation
Summary of ChangesHello @chandra-siri, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces significant performance enhancements and stability improvements to the microbenchmarks by optimizing resource initialization and process management. It ensures that critical components like multiprocessing pools and gRPC clients are set up efficiently, and worker processes are assigned dedicated CPU resources to minimize external interference, leading to more accurate and reliable benchmark results. Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request refactors the multiprocessing setup in the write benchmarks to initialize the process pool and clients only once per run, optimizing benchmark results by preventing setup overhead. From a security standpoint, the analysis could not be fully completed as several files listed in SECURITY_ANALYSIS_TODO.md were unreadable, meaning no vulnerabilities were found in the accessible files, but a comprehensive review was hindered. For code improvements, consider making the os.sched_setaffinity CPU affinity setting more robust and portable, and ensuring graceful cleanup of resources in worker processes.
|
|
||
| def _worker_init(bucket_type): | ||
| """Initializes a persistent event loop and client for each worker process.""" | ||
| os.sched_setaffinity(0, {i for i in range(20, 190)}) # Pin to cores 20-189 |
There was a problem hiding this comment.
The hardcoded core range range(20, 190) for os.sched_setaffinity is not portable and will raise a ValueError on systems with fewer than 190 cores, causing the benchmark to crash. Please consider making this more robust by checking against the number of available cores, for example by using os.cpu_count().
| def _worker_init(bucket_type): | ||
| """Initializes a persistent event loop and client for each worker process.""" | ||
| os.sched_setaffinity(0, {i for i in range(20, 190)}) # Pin to cores 20-189 | ||
| global worker_loop, worker_client, worker_json_client | ||
| if bucket_type == "zonal": | ||
| worker_loop = asyncio.new_event_loop() | ||
| asyncio.set_event_loop(worker_loop) | ||
| worker_client = worker_loop.run_until_complete(create_client()) | ||
| else: # regional | ||
| from google.cloud import storage | ||
|
|
||
| worker_json_client = storage.Client() |
There was a problem hiding this comment.
The _worker_init function initializes an event loop and clients for each worker process, but there's no corresponding cleanup logic. This can lead to resource leaks. While process termination will clean up resources, a graceful shutdown is better practice. Consider using atexit.register() in _worker_init to call a cleanup function that closes the clients and the event loop before the worker process exits.
…storage into writes_fixes
…storage into writes_fixes
|
/gcbrun(6bf0860) |
fix: init mp pool & grpc client once, use os.sched_setaffinity