feat: add time based benchmarks #1749
Conversation
product-auto-label
bot
added
size: l
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 establishes a foundational framework for time-based performance microbenchmarks within the project. The primary goal is to enable more accurate and continuous measurement of Google Cloud Storage read throughput under various conditions, moving beyond operation-count-based benchmarks to duration-based evaluations. This enhancement will provide deeper insights into the system's sustained performance characteristics. Highlights
Changelog
Activity
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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces time-based benchmarks for read operations. The changes include new configuration files (yaml and .py), a conftest.py for test setup, and the benchmark test logic itself. The implementation uses multiprocessing to run downloads in parallel.
My review focuses on improving the maintainability, portability, and performance of the benchmark code. Key suggestions include removing commented-out code, making benchmark names more descriptive, avoiding hardcoded values for CPU affinity to improve portability, and optimizing the download loop to reduce memory allocations and redundant calls, which is crucial for accurate performance measurement.
|
|
||
| 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, 180)}) # Pin to cores 20-189 |
There was a problem hiding this comment.
The CPU affinity is hardcoded to cores 20-179. This makes the benchmark non-portable and likely to fail on machines with fewer than 180 cores. Consider making this dynamic by querying the available cores using os.sched_getaffinity(0) and selecting a subset. This will make the benchmark more robust and runnable on different environments.
| offset = 0 | ||
| is_warming_up = True | ||
| start_time = time.monotonic() | ||
| warmup_end_time = start_time + params.warmup_duration | ||
| test_end_time = warmup_end_time + params.duration | ||
|
|
||
| while time.monotonic() < test_end_time: | ||
| current_time = time.monotonic() | ||
| if is_warming_up and current_time >= warmup_end_time: | ||
| is_warming_up = False | ||
| total_bytes_downloaded = 0 # Reset counter after warmup | ||
|
|
||
| if params.pattern == "rand": | ||
| offset = random.randint(0, params.file_size_bytes - params.chunk_size_bytes) | ||
|
|
||
| buffer = BytesIO() | ||
| await mrd.download_ranges([(offset, params.chunk_size_bytes, buffer)]) | ||
|
|
||
| if not is_warming_up: | ||
| total_bytes_downloaded += params.chunk_size_bytes | ||
|
|
||
| if params.pattern == "seq": | ||
| offset += params.chunk_size_bytes | ||
| if offset + params.chunk_size_bytes > params.file_size_bytes: | ||
| offset = 0 # Reset offset if end of file is reached | ||
|
|
There was a problem hiding this comment.
This while loop has a couple of inefficiencies that could affect benchmark accuracy:
- A new
BytesIObuffer is created on every iteration, which can cause significant memory allocation and garbage collection overhead. It's better to create it once outside the loop and reuse it. time.monotonic()is called twice per iteration. This can be reduced to one call.
The suggested change refactors the loop to address both issues.
offset = 0
is_warming_up = True
start_time = time.monotonic()
warmup_end_time = start_time + params.warmup_duration
test_end_time = warmup_end_time + params.duration
buffer = BytesIO()
while (current_time := time.monotonic()) < test_end_time:
if is_warming_up and current_time >= warmup_end_time:
is_warming_up = False
total_bytes_downloaded = 0 # Reset counter after warmup
if params.pattern == "rand":
offset = random.randint(0, params.file_size_bytes - params.chunk_size_bytes)
buffer.seek(0)
buffer.truncate(0)
await mrd.download_ranges([(offset, params.chunk_size_bytes, buffer)])
if not is_warming_up:
total_bytes_downloaded += params.chunk_size_bytes
if params.pattern == "seq":
offset += params.chunk_size_bytes
if offset + params.chunk_size_bytes > params.file_size_bytes:
offset = 0 # Reset offset if end of file is reached| # def _upload_worker(args): | ||
| # bucket_name, object_name, object_size = args | ||
| # storage_client = storage.Client() | ||
| # bucket = storage_client.bucket(bucket_name) | ||
| # blob = bucket.blob(object_name) | ||
|
|
||
| # try: | ||
| # blob.reload() | ||
| # if blob.size >= object_size: | ||
| # logging.info(f"Object {object_name} already exists and has the required size.") | ||
| # return object_name, object_size | ||
| # except Exception: | ||
| # pass | ||
|
|
||
| # logging.info(f"Creating object {object_name} of size {object_size} bytes.") | ||
| # # For large objects, it's better to upload in chunks. | ||
| # # Using urandom is slow, so for large objects, we will write the same chunk over and over. | ||
| # chunk_size = 100 * 1024 * 1024 # 100 MiB | ||
| # data_chunk = os.urandom(chunk_size) | ||
| # num_chunks = object_size // chunk_size | ||
| # remaining_bytes = object_size % chunk_size | ||
|
|
||
| # from io import BytesIO | ||
| # with BytesIO() as f: | ||
| # for _ in range(num_chunks): | ||
| # f.write(data_chunk) | ||
| # if remaining_bytes > 0: | ||
| # f.write(data_chunk[:remaining_bytes]) | ||
|
|
||
| # f.seek(0) | ||
| # blob.upload_from_file(f, size=object_size) | ||
|
|
||
| # logging.info(f"Finished creating object {object_name}.") | ||
| # return object_name, object_size | ||
|
|
||
|
|
||
| # def _create_files(num_files, bucket_name, object_size): | ||
| # """ | ||
| # Create/Upload objects for benchmarking and return a list of their names. | ||
| # """ | ||
| # object_names = [f"{_OBJECT_NAME_PREFIX}_{i}" for i in range(num_files)] | ||
|
|
||
| # args_list = [ | ||
| # (bucket_name, object_names[i], object_size) for i in range(num_files) | ||
| # ] | ||
|
|
||
| # # Don't use a pool to avoid contention writing the same objects. | ||
| # # The check for existence should make this fast on subsequent runs. | ||
| # results = [_upload_worker(arg) for arg in args_list] | ||
|
|
||
| # return [r[0] for r in results] | ||
|
|
There was a problem hiding this comment.
This file contains a large block of commented-out code. To improve readability and maintainability, this unused code should be removed. If it's intended for future use, consider moving it to a separate file or adding a TODO with an explanation.
| num_files = num_processes * num_coros | ||
|
|
||
| # Create a descriptive name for the parameter set | ||
| name = f"{pattern}_{bucket_type}_{num_processes}p_{file_size_mib}MiB_{chunk_size_mib}MiB" |
There was a problem hiding this comment.
The generated benchmark name does not include num_coros. While the current configuration only uses one coroutine, including this parameter in the name will make the benchmark results clearer and more robust if the number of coroutines is varied in the future.
| name = f"{pattern}_{bucket_type}_{num_processes}p_{file_size_mib}MiB_{chunk_size_mib}MiB" | |
| name = f"{pattern}_{bucket_type}_{num_processes}p_{num_coros}c_{file_size_mib}MiB_{chunk_size_mib}MiB" |
1 participant
feat: add time based benchmarks