-
Notifications
You must be signed in to change notification settings - Fork 303
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
HPCC-32933 Fix execute timings for loop activity #19264
base: master
Are you sure you want to change the base?
Conversation
Track execute timings are incurred by CATCH_NEXTROW. And remove execute timings in getNextRow as it is executed by the CNextRowLoader thread. Signed-off-by: Shamser Ahmed <[email protected]>
Jira Issue: https://hpccsystems.atlassian.net//browse/HPCC-32933 Jirabot Action Result: |
@@ -439,6 +438,7 @@ class CLoopSlaveActivity : public CLoopSlaveActivityBase | |||
} | |||
CATCH_NEXTROW() | |||
{ | |||
ActivityTimer t(slaveTimerStats, timeActivities); |
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 this change looks correct in principle, but in practice, unless the lookahead time that is being performed by the feeder, the timing calculation may well be more inaccurate.
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.
@shamser - please see comment.
Signed-off-by: Shamser Ahmed <[email protected]>
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.
@shamser - see comment.
{ | ||
ret.setown(curInput->nextRow()); // more cope with groups somehow.... | ||
LookAheadTimer t(slaveTimerStats, timeActivities); |
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.
hm, I think it's a bit more nuanced than this.
curInput is only the inputStream initially.. it then changes, the 2nd iteration of the loop, is not processing the input, but the result of the previous iteration. As it stands if it's doing many iterations, it will count it all against look ahead time.
I think you need to set a flag and only do until curInput changes.
Track execute timings are incurred by CATCH_NEXTROW. And remove execute timings in getNextRow as it is executed by the CNextRowLoader thread. Track lookahead time.
Type of change:
Checklist:
Smoketest:
Testing: