Skip to content
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.

Sign up for GitHub

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-32947 Capture and report lookahead timings for project #19275

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
+13 −2
Open

HPCC-32947 Capture and report lookahead timings for project #19275

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
14ea038
HPCC-32947 Capture and report lookahead timings for project
shamser Nov 5, 2024
73dd3d4
HPCC-23947 Changes following review
shamser Nov 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion thorlcr/activities/project/thprojectslave.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ class CProjecStrandProcessor : public CThorStrandProcessor
}
STRAND_CATCH_NEXTROW()
{
ActivityTimer t(slaveTimerStats, timeActivities);
LookAheadTimer t(parent.slaveTimerStats, timeActivities);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

for (;;)
{
if (parent.queryAbortSoon())
Expand Down
Loading