-
Notifications
You must be signed in to change notification settings - Fork 970
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
Loadgen startup fix #4087
Loadgen startup fix #4087
Conversation
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.
Looks good! I think we can clean up the start function a little more though.
src/simulation/LoadGenerator.cpp
Outdated
LoadGenerator::start(GeneratedLoadConfig& cfg) | ||
{ | ||
// Setup config for soroban modes | ||
if (!mStarted && cfg.isSoroban()) |
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.
start
should be a no-op except on the first invocation, so why not add if (mStarted) return;
at the beginning and get rid of the checks in the individual ifs. Currently the only functions that get called multiple times in start
are createRootAccount
and updateMinBalance
. If we need to call those periodically they probably don't belong in start
(but I don't think we do).
src/simulation/LoadGenerator.cpp
Outdated
GeneratedLoadConfig::areTxsRemaining() const | ||
{ | ||
return (isCreate() && nAccounts != 0) || (!isCreate() && nTxs != 0); | ||
createRootAccount(); |
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.
Now that we have a single start function, we should change if (!mRoot)
to releaseAssert(!mRoot)
in createRootAccount()
.
src/simulation/LoadGenerator.cpp
Outdated
&VirtualTimer::onFailureNoop); | ||
} | ||
else | ||
if (!mStartTime) |
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 think we should change this to releaseAssert(!mStartTime)
(assuming you implement my other comment where we exit early in start()
).
498c2da
to
ad064ba
Compare
@SirTyson added your suggestions, so this should be ready now |
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.
LGTM, one small nit.
ad064ba
to
fce809d
Compare
r+ fce809d |
Resolves #4071