HPCC-32480 Capture "look ahead" timings for unordered concat (parallel funnel) #19164

shamser · 2024-09-27T15:44:00Z

Type of change:

This change is a bug fix (non-breaking change which fixes an issue).
This change is a new feature (non-breaking change which adds functionality).
This change improves the code (refactor or other change that does not change the functionality)
This change fixes warnings (the fix does not alter the functionality or the generated code)
This change is a breaking change (fix or feature that will cause existing behavior to change).
This change alters the query API (existing queries will have to be recompiled)

Checklist:

Smoketest:

Send notifications about my Pull Request position in Smoketest queue.
Test my draft Pull Request.

Testing:

github-actions · 2024-09-27T15:44:18Z

Jira Issue: https://hpccsystems.atlassian.net//browse/HPCC-32480

Jirabot Action Result:
Workflow Transition To: Merge Pending
Updated PR

jakesmith

@shamser - I am not sure what is being considered lookahead or blocked time is correct at the moment - please see comments.

jakesmith · 2024-10-18T14:21:24Z

common/thorhelper/thorcommon.hpp

@@ -315,7 +315,7 @@ class SimpleActivityTimer
    cycle_t startCycles;
    cycle_t &accumulator;
 protected:
-    const bool enabled;
+    bool enabled;


might be better to make mutable.

jakesmith · 2024-10-18T16:32:45Z

thorlcr/activities/funnel/thfunnelslave.cpp

@@ -85,6 +85,7 @@ class CParallelFunnel : implements IRowStream, public CSimpleInterface
                inputStream = funnel.activity.queryInputStream(inputIndex);
                while (!stopping)
                {
+                    LookAheadTimer timer(funnel.activity.getActivityTimerAccumulator(), funnel.activity.queryTimeActivities());


not sure about what this is collecting, but think:

it should include the time it spent in startInput

should not include the time it may block in pushMulti

basically, lookahead time should only be the time it spend reading the the input ahead (which in a single thread regular act. would be done inline and be part of total cycles).

jakesmith · 2024-10-18T16:33:46Z

thorlcr/activities/funnel/thfunnelslave.cpp

        size32_t rowSize = thorRowMemoryFootprint(serializer, row);

        bool waitForSpace = false;
        // only allow a single writer at a time, so only a single thread is waiting on the semaphore - otherwise signal() takes a very long time
        {
-
+            BlockedActivityTimer timer(activity.getActivityTimerAccumulator(), activity.queryTimeActivities());


why measure this blocking time (on crit), but not on waitSem blockage?

I am not sure this time should be considered blocked at all.

As it stands, if there are 3 inputs to the funnel, the 1st will gain this crit instantly, and then (if full) hold the crit and wait on the fullSem semaphore (but will not count toward blocked time)
The other 2 input handlers will both block here and count toward blocked time.
If there are 100 inputs, 99 will add blocked time.

But I'm not sure any should consider this blocked time, they are not "blocking" the downstream act except for the very small amount of time they spend in the crit adding to 'rows'.
If they (the input handlers) are blocked on fullSem it's because the downstream act isn't pulling or pulling fast enough - this act. is not blocking it.

jakesmith

@shamser - please see comments.

It would be helpful in these less than obvious cases, if there was a comment explaining the logic of why lookahead or blocked - and how it will factor in to the local cycles calculation.

jakesmith · 2024-10-22T10:41:30Z

thorlcr/activities/funnel/thfunnelslave.cpp

                started = true;
                inputStream = funnel.activity.queryInputStream(inputIndex);
                while (!stopping)
                {
                    numRows = 0;
                    for (;numRows < chunkSize; numRows++)
                    {
+                        LookAheadTimer timer(funnel.activity.getActivityTimerAccumulator(), funnel.activity.queryTimeActivities());


I think for efficiency should move this outside of the for loop.

jakesmith · 2024-10-22T10:46:03Z

thorlcr/activities/funnel/thfunnelslave.cpp

@@ -198,6 +201,7 @@ class CParallelFunnel : implements IRowStream, public CSimpleInterface
        if (waitForSpace)
        {
            CriticalBlock b(writerCrit);
+            BlockedActivityTimer timer(activity.getActivityTimerAccumulator(), activity.queryTimeActivities());


I am still not sure about this. My previous comment [https://github.com//pull/19164#discussion_r1806771480]:

But I'm not sure any should consider this blocked time, they are not "blocking" the downstream act except for the very small amount of time they spend in the crit adding to 'rows'.
If they (the input handlers) are blocked on fullSem it's because the downstream act isn't pulling or pulling fast enough - this act. is not blocking it.

It is not blocking the downstream act. at this point, it is blocked because it is full, because the downstream act. hasn't pulled enough out.

Ok. That makes sense.

jakesmith · 2024-10-22T11:00:02Z

thorlcr/activities/funnel/thfunnelslave.cpp

        OwnedConstThorRow row = rows.dequeue();
+        timer.leave();


this blocked time will be the time blocked because the input-handlers haven't started or kept up and pushed rows in time.
I think this is correct, with lookahead time tracking the time it spent for input, that time (that will be added to totaltime)

blocked time is any time it spent dawdling waiting for the input handlers to catch up and push more.

I don't think the leave() mechanism is useful here though (and would be better to be removed until/if it really is).
It would better to scope the BlockedActivityTimer as normal.

jakesmith · 2024-10-22T11:04:35Z

thorlcr/activities/funnel/thfunnelslave.cpp

-                try { startInput(i); }
+                try
+                {
+                    LookAheadTimer timer(slaveTimerStats, timeActivities);


should not include for i == 0, because it is not called async/on a separate, see deferred comment below

jakesmith

@shamser - 1 minor comment, please look at and squash.

jakesmith · 2024-10-29T19:03:58Z

thorlcr/activities/funnel/thfunnelslave.cpp

@@ -384,7 +383,8 @@ class FunnelSlaveActivity : public CSlaveActivity
            {
                try
                {
-                    LookAheadTimer timer(slaveTimerStats, timeActivities);
+                    // n.b. i>0 is started asynchronously, so track look ahead time
+                    LookAheadTimer timer(slaveTimerStats, (i==0) ? false : timeActivities);


minor: better to avoid constructing LookAheadTimer and coding as something like:

if (i == 0) // 1st input is started synchronously, so time already included in start() timing. startInput(i); else { LookAheadTimer timer(slaveTimerStats, timeActivities); startInput(i); }

…l funnel) Signed-off-by: Shamser Ahmed <[email protected]>

jakesmith

@shamser - looks good.

github-actions · 2024-11-11T15:47:48Z

Jirabot Action Result:
Added fix version: 9.10.0
Workflow Transition: 'Resolve issue'

shamser force-pushed the issue32480 branch from feed3d7 to 9be33c2 Compare October 16, 2024 12:21

shamser changed the base branch from candidate-9.8.x to master October 16, 2024 12:21

shamser marked this pull request as ready for review October 16, 2024 12:22

shamser requested a review from jakesmith October 16, 2024 12:23

jakesmith reviewed Oct 18, 2024

View reviewed changes

shamser requested a review from jakesmith October 22, 2024 09:00

jakesmith reviewed Oct 22, 2024

View reviewed changes

shamser requested a review from jakesmith October 29, 2024 10:19

jakesmith reviewed Oct 29, 2024

View reviewed changes

HPCC-32480 Capture "look ahead" timings for unordered concat (paralle…

bf09241

…l funnel) Signed-off-by: Shamser Ahmed <[email protected]>

shamser force-pushed the issue32480 branch from 41866ee to bf09241 Compare October 30, 2024 09:49

shamser requested a review from jakesmith October 30, 2024 09:50

jakesmith approved these changes Oct 30, 2024

View reviewed changes

ghalliday approved these changes Nov 11, 2024

View reviewed changes

ghalliday merged commit 5c6a5ef into hpcc-systems:master Nov 11, 2024
53 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HPCC-32480 Capture "look ahead" timings for unordered concat (parallel funnel) #19164

HPCC-32480 Capture "look ahead" timings for unordered concat (parallel funnel) #19164

shamser commented Sep 27, 2024 •

edited

Loading

github-actions bot commented Sep 27, 2024

jakesmith left a comment

jakesmith Oct 18, 2024

jakesmith Oct 18, 2024 •

edited

Loading

jakesmith Oct 18, 2024

jakesmith Oct 18, 2024

jakesmith left a comment

jakesmith Oct 22, 2024

jakesmith Oct 22, 2024

shamser Oct 24, 2024

jakesmith Oct 22, 2024

jakesmith Oct 22, 2024

jakesmith left a comment

jakesmith Oct 29, 2024

jakesmith left a comment

github-actions bot commented Nov 11, 2024

HPCC-32480 Capture "look ahead" timings for unordered concat (parallel funnel) #19164

HPCC-32480 Capture "look ahead" timings for unordered concat (parallel funnel) #19164

Conversation

shamser commented Sep 27, 2024 • edited Loading

Type of change:

Checklist:

Smoketest:

Testing:

github-actions bot commented Sep 27, 2024

jakesmith left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakesmith Oct 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakesmith left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakesmith left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakesmith left a comment

Choose a reason for hiding this comment

github-actions bot commented Nov 11, 2024

shamser commented Sep 27, 2024 •

edited

Loading

jakesmith Oct 18, 2024 •

edited

Loading