-
Notifications
You must be signed in to change notification settings - Fork 122
chore: added iterations opimization #1525
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
Conversation
lib/runner/run.js
Outdated
| }); | ||
| } | ||
| // if custom parallel iterations is true, then set the areIterationsParallelized to true | ||
| this.areIterationsParallelized = this.options?.customParallelIterations || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this state not saved in the Partition Manager instance?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because this state is only used by the run instance. based on it's value run instance will decide whether to use the partition manager or no.
| this.snrHash = null; // we populate it in the first SNR call | ||
|
|
||
| done(); | ||
| return done(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why return callback?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see we've moved some code to utils, but there's still a lot of duplicate code from the waterfall command. Why can't it queue the waterfall or find a better way to avoid this duplication?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have created a separate function to handle the SNR logic, which reduces duplicate code.
While we could make changes to the waterfall command, we prefer to avoid modifying it or introducing additional conditional statements. Doing so could negatively affect its performance and make the overall logic harder to understand. For this reason, we have chosen to implement a completely new command for parallel runs, even if it means some code duplication.
lib/runner/run.js
Outdated
| /** | ||
| * Starts a parallel iteration | ||
| * @param {Number} index - The index of the partition to run | ||
| * @param {Object} localVariables - Local variables for the iteration | ||
| * @param {Function} callback - The callback to call when the iteration is complete | ||
| */ | ||
| startParallelIteration (index, localVariables, callback) { | ||
| this.partitionManager.runSinglePartition(index, localVariables, callback); | ||
| }, | ||
|
|
||
| /** | ||
| * Stops a parallel iteration | ||
| * @param {Number} index - The index of the partition to stop | ||
| * @param {Function} callback - The callback to call when the iteration is complete | ||
| */ | ||
| stopParallelIteration (index, callback) { | ||
| this.partitionManager.stopSinglePartition(index, callback); | ||
| }, | ||
|
|
||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see these methods being used. Do we still need them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, these are methods exposed for external orchestration not used inside the code directly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that case, move these to the prototype object of parallel.command.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
| result && result._variables && | ||
| (this.state._variables = new sdk.VariableScope(result._variables)); | ||
|
|
||
| // persist the pm.variables for the next request in the current iteration |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's happening here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since each partition needs to track it's own variables, we are updating the variables for that partition only. So we do not affect other partitions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be a helper function in partition manager, something like: this.partitionManager.persistVariables(result._variables).
Co-authored-by: Udit Vasu <udit@pm.me>
Codecov Report❌ Patch coverage is ❌ Your patch status has failed because the patch coverage (91.52%) is below the target coverage (100.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## develop #1525 +/- ##
============================================
+ Coverage 38.59% 75.20% +36.61%
============================================
Files 46 49 +3
Lines 3537 3779 +242
Branches 1027 1081 +54
============================================
+ Hits 1365 2842 +1477
+ Misses 2074 713 -1361
- Partials 98 224 +126
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| // the process tick | ||
| backpack.ensure(userback, this) && userback(); | ||
|
|
||
| this.partitionManager && this.partitionManager.clearPools(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In what situations can this.partitionManager be undefined?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In Production code, never. but since partitionManager isn't a part of the main execution loop (which is waterfall), this is just a defensive measure for future proofing and run object manipulation.
let me know if this check should be removed.
| // the process tick | ||
| backpack.ensure(userback, this) && userback(); | ||
|
|
||
| this.partitionManager && this.partitionManager.clearPools(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe just .clear() or .dispose().
| // do so in a try block to ensure it does not hamper the process tick | ||
| backpack.ensure(userback, this) && userback(); | ||
| } | ||
| this.partitionManager && this.partitionManager.triggerStopAction(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe we should include this in this.partitionManager.dispose(), which clears and triggers the stop.
| result && result._variables && | ||
| (this.state._variables = new sdk.VariableScope(result._variables)); | ||
|
|
||
| // persist the pm.variables for the next request in the current iteration |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be a helper function in partition manager, something like: this.partitionManager.persistVariables(result._variables).
lib/runner/run.js
Outdated
| /** | ||
| * Starts a parallel iteration | ||
| * @param {Number} index - The index of the partition to run | ||
| * @param {Object} localVariables - Local variables for the iteration | ||
| * @param {Function} callback - The callback to call when the iteration is complete | ||
| */ | ||
| startParallelIteration (index, localVariables, callback) { | ||
| this.partitionManager.runSinglePartition(index, localVariables, callback); | ||
| }, | ||
|
|
||
| /** | ||
| * Stops a parallel iteration | ||
| * @param {Number} index - The index of the partition to stop | ||
| * @param {Function} callback - The callback to call when the iteration is complete | ||
| */ | ||
| stopParallelIteration (index, callback) { | ||
| this.partitionManager.stopSinglePartition(index, callback); | ||
| }, | ||
|
|
||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that case, move these to the prototype object of parallel.command.
Added performance optimization for iteration execution