[AUTO] RHEL-141678 - aide multi-threading performance regression#56
[AUTO] RHEL-141678 - aide multi-threading performance regression#56Koncpa wants to merge 3 commits into
Conversation
Reviewer's GuideAdds a new beakerlib-based regression test that measures AIDE initialization performance with and without multi-threading and fails if multi-threading is more than 20% slower, guarding against the RHEL-141678 regression. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The timing variables
multithread_timeandsinglethread_timeare assigned insiderlRuncommands, which execute in a subshell, so those variable values will not persist in the main script and the subsequent comparison logic will not work as intended; consider measuring time outsiderlRunor capturing it via command substitution. - Using
SECONDSwith a single run ofaide --initmay lead to very coarse or noisy measurements on fast systems; consider running each configuration multiple times and comparing a min/average to reduce flakiness in the 20% threshold check.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The timing variables `multithread_time` and `singlethread_time` are assigned inside `rlRun` commands, which execute in a subshell, so those variable values will not persist in the main script and the subsequent comparison logic will not work as intended; consider measuring time outside `rlRun` or capturing it via command substitution.
- Using `SECONDS` with a single run of `aide --init` may lead to very coarse or noisy measurements on fast systems; consider running each configuration multiple times and comparing a min/average to reduce flakiness in the 20% threshold check.
## Individual Comments
### Comment 1
<location path="Regression/RHEL-141678-aide-threading-performance/runtest.sh" line_range="16-21" />
<code_context>
+
+ rlPhaseStartTest "Test AIDE performance with and without multi-threading"
+ # Time the execution of "aide --init" with default multi-threading
+ rlRun "SECONDS=0; aide --init -c aide.conf; multithread_time=\$SECONDS" "Running aide --init with multi-threading"
+ rlLog "Multi-threaded execution time: \$multithread_time seconds"
+ rlRun "rm -f aide.db.new.gz"
+
+ # Time the execution of "aide --init" with multi-threading disabled
+ rlRun "SECONDS=0; aide --init -c aide.conf -W 0; singlethread_time=\$SECONDS" "Running aide --init without multi-threading"
+ rlLog "Single-threaded execution time: \$singlethread_time seconds"
+
</code_context>
<issue_to_address>
**issue (bug_risk):** rlRun arguments are misordered, so the description strings are being treated as expected return codes.
`rlRun` has the form `rlRun COMMAND [EXPECT_RET=0 [MESSAGE ...]]`. Here the second argument is being used for the message, so the string is actually parsed as `EXPECT_RET` (coerced to `0`) and the message is lost. Please make the expected return code explicit and move the descriptions to the third argument, e.g.:
```bash
rlRun "SECONDS=0; aide --init -c aide.conf; multithread_time=$SECONDS" 0 "Running aide --init with multi-threading"
rlRun "SECONDS=0; aide --init -c aide.conf -W 0; singlethread_time=$SECONDS" 0 "Running aide --init without multi-threading"
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| rlRun "SECONDS=0; aide --init -c aide.conf; multithread_time=\$SECONDS" "Running aide --init with multi-threading" | ||
| rlLog "Multi-threaded execution time: \$multithread_time seconds" | ||
| rlRun "rm -f aide.db.new.gz" | ||
|
|
||
| # Time the execution of "aide --init" with multi-threading disabled | ||
| rlRun "SECONDS=0; aide --init -c aide.conf -W 0; singlethread_time=\$SECONDS" "Running aide --init without multi-threading" |
There was a problem hiding this comment.
issue (bug_risk): rlRun arguments are misordered, so the description strings are being treated as expected return codes.
rlRun has the form rlRun COMMAND [EXPECT_RET=0 [MESSAGE ...]]. Here the second argument is being used for the message, so the string is actually parsed as EXPECT_RET (coerced to 0) and the message is lost. Please make the expected return code explicit and move the descriptions to the third argument, e.g.:
rlRun "SECONDS=0; aide --init -c aide.conf; multithread_time=$SECONDS" 0 "Running aide --init with multi-threading"
rlRun "SECONDS=0; aide --init -c aide.conf -W 0; singlethread_time=$SECONDS" 0 "Running aide --init without multi-threading"
This PR was generated by the QE automation pipeline and requires human review.
Summary
This PR introduces a new regression test for RHEL-141678, which addresses a performance degradation in
aidewhen using its default multi-threading capabilities.Test Description
The new test, located at
Regression/RHEL-141678-aide-threading-performance, measures and compares the execution time ofaide --initwith and without multi-threading enabled (-W 0flag).The test fails if the multi-threaded execution is more than 20% slower than the single-threaded execution, thus preventing future performance regressions of this nature.
How to Run
aideandbeakerlib.rhts-run.shortmt.Reference Tests
Sanity/aide-db-update-and-compareSummary by Sourcery
Tests: