Forkrun testing async io 7#73
Conversation
Refactor read and write operations to handle errors more robustly and improve the handling of data availability in the buffer.
Added step to upload generated frun.bash as an artifact.
Added NUMA node discovery and topology mapping functionality.
Refactor NUMA node handling and improve error reporting.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @jkool702, your pull request is larger than the review limit of 150000 diff characters
Summary of ChangesHello, 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 significantly enhances the Highlights
Changelog
Ignored Files
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 summary |
|
/gemini review |
![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 significant enhancements to forkrun's asynchronous I/O and NUMA handling capabilities. The changes include a sophisticated NUMA topology mapping mechanism in the main bash script, more robust I/O handling in the C loadables with better error checking and retry logic for transient conditions, and improved process management for worker spawning.
My review identified a potential issue in the new worker spawn loop where the global maximum worker limit (nWorkersMax) might not be respected, which could lead to resource over-consumption. I've also suggested a simplification for a developer utility script to improve its readability and maintainability. Overall, these are solid improvements that make the tool more robust and powerful.
Note: Security Review did not run due to the size of the PR.
| for (( ; node_workers[node_idx] < target; node_workers[node_idx]++ )); do | ||
| spawn_worker "$nWorkers" "$node_idx" | ||
| node_idx=$(( (node_idx + 1) % FORKRUN_NUM_NODES )) | ||
| ((nWorkers++)) | ||
| done |
There was a problem hiding this comment.
The worker spawn loop does not respect the global nWorkersMax limit. The per-node limit node_worker_max is calculated using integer division, which can lead to the total number of spawned workers exceeding nWorkersMax in some cases. For example, if nWorkersMax is 27 and FORKRUN_NUM_NODES is 28, node_worker_max becomes 1, allowing up to 28 workers to be spawned.
To fix this, you should add a check against nWorkersMax in the inner spawn loop to ensure the total number of workers does not exceed the configured maximum.
| for (( ; node_workers[node_idx] < target; node_workers[node_idx]++ )); do | |
| spawn_worker "$nWorkers" "$node_idx" | |
| node_idx=$(( (node_idx + 1) % FORKRUN_NUM_NODES )) | |
| ((nWorkers++)) | |
| done | |
| for (( ; node_workers[node_idx] < target && nWorkers < nWorkersMax; node_workers[node_idx]++ )); do | |
| spawn_worker "$nWorkers" "$node_idx" | |
| ((nWorkers++)) | |
| done |
| @@ -0,0 +1,22 @@ | |||
| . <( { echoFlag=false; while true; do IFS= read -r line; [[ "$line" == *'_forkrun_file_to_base64() {'* ]] && echoFlag=true; $echoFlag && echo "$line"; $echoFlag && [[ "$line" == '}'* ]] && break; done; } <frun.bash ) | |||
There was a problem hiding this comment.
This line to source the _forkrun_file_to_base64 function is very complex and hard to read, which could make it difficult to maintain. A simpler and more readable approach would be to use sed to extract the function definition.
. <(sed -n '/_forkrun_file_to_base64() {/,/}/p' frun.bash)
Summary of ChangesThis pull request significantly advances the Highlights
Changelog
Ignored Files
Activity
|
There was a problem hiding this comment.
Code Review
This pull request introduces significant enhancements for NUMA-aware processing in forkrun, including a sophisticated NUMA topology mapping in the bash script and corresponding logic in the C loadables. It also improves the robustness of I/O operations by handling partial reads/writes and potential race conditions more gracefully. The benchmark scripts have also been updated for better flexibility and accuracy. My review includes a couple of suggestions to improve maintainability in the C code and a utility script.
Note: Security Review did not run due to the size of the PR.
| static ssize_t robust_sendfile(int out_fd, int in_fd, off_t *offset, size_t count) { | ||
| size_t total = 0; | ||
| int retries = 0; | ||
| while (total < count) { | ||
| ssize_t s = sendfile(out_fd, in_fd, offset, count - total); | ||
| if (s < 0) { | ||
| if (errno == EINTR || errno == EAGAIN) continue; | ||
| if (errno == EINTR || errno == EAGAIN) { usleep(10); continue; } | ||
| return total > 0 ? (ssize_t)total : -1; | ||
| } | ||
| if (s == 0) break; // genuine EOF: file shorter than ack claimed | ||
| if (s == 0) { | ||
| if (retries++ < 1000) { usleep(100); continue; } | ||
| break; | ||
| } | ||
| retries = 0; | ||
| total += s; | ||
| } | ||
| return (ssize_t)total; | ||
| }; | ||
| } |
There was a problem hiding this comment.
This function, along with ring_copy_chunk and ring_copy_main, uses magic numbers for retry counts and sleep durations (e.g., 10, 1000, 100). This harms readability and makes tuning these values difficult. It would be better to define these as named constants at the top of the file.
For example:
#define ROBUST_IO_MAX_RETRIES 1000
#define ROBUST_IO_RETRY_SLEEP_US 100
#define ROBUST_IO_EINTR_SLEEP_US 10Then, these constants can be used throughout the file, improving maintainability.
| @@ -0,0 +1,22 @@ | |||
| . <( { echoFlag=false; while true; do IFS= read -r line; [[ "$line" == *'_forkrun_file_to_base64() {'* ]] && echoFlag=true; $echoFlag && echo "$line"; $echoFlag && [[ "$line" == '}'* ]] && break; done; } <frun.bash ) | |||
There was a problem hiding this comment.
This method of extracting the _forkrun_file_to_base64 function is fragile. It depends on the exact formatting of the function definition in frun.bash, including spacing. Any minor formatting change to that function in the source file could break this script. A more robust approach would be to use a tool like sed or awk for extraction, or to duplicate the function in this utility script if it's not expected to change often.
|
1 participant
@sourcery-ai summary