HPCC-32947 Capture and report lookahead timings for project #19275
Conversation
The modification records time spent in the strands as lookahead timing. This is necessary to ensure the local execute timings for project and upstream activities are correct. Ideally, this lookahead timings would be recorded a separate statistic such as "background activity". However, a new statistic will not be created at this time to avoid complicating the statistics. Signed-off-by: Shamser Ahmed <[email protected]>
|
Jira Issue: https://hpccsystems.atlassian.net//browse/HPCC-32947 Jirabot Action Result: |
| @@ -35,7 +35,7 @@ class CProjecStrandProcessor : public CThorStrandProcessor | |||
| } | |||
| STRAND_CATCH_NEXTROW() | |||
| { | |||
| ActivityTimer t(slaveTimerStats, timeActivities); | |||
| LookAheadTimer t(parent.slaveTimerStats, timeActivities); | |||
There was a problem hiding this comment.
as discussed, this will cause threading problems. I don't think any of the other uses of LookAheadTimer (in pending PR's are thread unsafe?).
If it were to do this, it could collect the timing in the local lookahead member of slaveTimerStats, and then aggregate in the activity, in the same way CThorStrandedActivity::queryTotalCycles() const is currently aggregating totalCycles
BUT, this aggregated lookahead time is going to mess up the timing afaict. e.g. 4 strands, each taking 1 sec, will be seen as 4 secs total, but they are concurrent. The time spent in the project act. will not be 4 seconds, it will be however long it spent gettings rows out of the strands as a whole.
I'm not sure where the correct place to time is, and will depend when the strands are being combined, but I think think this is will be correct (or was)
There was a problem hiding this comment.
I am addressing the threading LookAhead timing update issue.
However, I believe strands correct timing would be the LookAheadCycles. Because recording as TotalCycles cause problem: if we were to record the strand execute time as TotalCycles (as opposed to LookAheadCycles), it causes problems with downstream local execute time calculation (because aggregation of the strand time as TotalCycles is likely to cause the upstream activity's total time to be less.
There was a problem hiding this comment.
I think you can accurately accumulate lookahead time here, but the placement of the ActivityTimer is capturing lookahead time and the project time at the moment. Imagine time to read from input is negligible, but the transform if relatively slow, this will now be seen a large lookahead time - which is incorrect.
I am still not sure how you accurately capture the total time the project (not the input) spent, because they are all overlapping and you do not want the aggregate total, but the time spent getting the rows out, which will depend on how they are combined etc.
Signed-off-by: Shamser Ahmed <[email protected]>
|
@shamser - notes from discussion we had today with Gavin. The stranded variety of project is little used. Fixing the timing problem relating to it therefore have a much lower priority. |
The modification records time spent in the strands as lookahead timing. This is necessary to ensure the local execute timings for project and upstream activities are correct.
Ideally, this lookahead timings would be recorded a separate statistic such as "background activity". However, a new statistic will not be created at this time to avoid complicating the statistics.
Type of change:
Checklist:
Smoketest:
Testing: