HPCC-32193 Fix some issues with spill stats in smart join activity #18866

shamser · 2024-07-10T12:07:49Z

The current temp file statistics for smartjoin did not include all stats from all temp files:

temp files were closed before its sizes were recorded in the stats
stats from some types of temp files were not being tracked such as overflowWriteFile from RHS
stats from temp files that were closed in CSpillableStreamBase were not preserved
peak temp file size was not tracked in CThorSpillableRowArray
make CThorRowCollectorBase use of stats from CThorSpillableRowArray for more accurate and simpler temp stats tracking (for CThorRowCollectorBase)

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-07-10T12:08:14Z

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

Jirabot Action Result:
Workflow Transition: Merge Pending
Updated PR

ghalliday

Broadly looks good. One question about thread safety

ghalliday · 2024-08-19T16:14:50Z

thorlcr/activities/lookupjoin/thlookupjoinslave.cpp

@@ -3018,6 +3035,13 @@ class CLookupJoinActivityBase : public CInMemJoinBase<HTHELPER, IHThorHashJoinAr
                activeStats.setStatistic(StNumSmartJoinDegradedToLocal, aggregateFailoversToLocal); // NB: is going to be same for all slaves.
            activeStats.setStatistic(StNumSmartJoinSlavesDegradedToStd, aggregateFailoversToStandard);
        }
+        if (overflowWriteFileIO)


Does there need to be a critical block incase the overflow file, or the rhsSlaveRows are killed when this is being called?

jakesmith

@shamser - please see comments

jakesmith · 2024-08-20T09:11:18Z

thorlcr/thorutil/thmem.hpp

@@ -413,7 +413,7 @@ class graph_decl CThorSpillableRowArray : private CThorExpandingRowArray, implem
    mutable CriticalSection cs;
    ICopyArrayOf<IWritePosCallback> writeCallbacks;
    size32_t compBlkSz = 0; // means use default
-
+    CRuntimeStatisticCollection inactiveStats; // reset after each kill


they're not really "inactive" at this point afaics, they're the current stats of CThorSpillableRowArray (they get updated every save()). I think would be better to rename to 'stats'

jakesmith · 2024-08-20T09:14:23Z

thorlcr/activities/lookupjoin/thlookupjoinslave.cpp

                overflowWriteStream.clear();
+                if (overflowWriteFileIO)
+                {
+                    mergeRemappedStats(PARENT::inactiveStats, overflowWriteFileIO, diskToTempStatsMap);


note: I think this is correct approach - utility class uses standard IO stats types, and the activity remaps them to what it wants to

thorlcr/activities/lookupjoin/thlookupjoinslave.cpp

jakesmith · 2024-08-20T09:21:49Z

thorlcr/activities/lookupjoin/thlookupjoinslave.cpp

@@ -2678,8 +2690,12 @@ class CLookupJoinActivityBase : public CInMemJoinBase<HTHELPER, IHThorHashJoinAr
            IOutputMetaData *inputOutputMeta = rightITDL->queryFromActivity()->queryContainer().queryHelper()->queryOutputMeta();
            // rows may either be in separate slave row arrays or in single rhs array, or split.
            rowcount_t total = rightCollector ? rightCollector->numRows() : (getGlobalRHSTotal() + rhs.ordinality());
+            if (rightCollector && rightCollector->hasSpilt())


I think should move above the if !isOOMException .. i.e. still want to capture stats.
It would be cleaner if there was a Owned (before the try) and the code in the exception handler only differentiated the exception type, e.g.:

catch (IException *e) { if (!isOOMException(e)) exception.setown(e); else { IOutputMetaData *inputOutputMeta = rightITDL->queryFromActivity()->queryContainer().queryHelper()->queryOutputMeta(); // rows may either be in separate slave row arrays or in single rhs array, or split. rowcount_t total = rightCollector ? rightCollector->numRows() : (getGlobalRHSTotal() + rhs.ordinality()); exception.setown(checkAndCreateOOMContextException(this, e, "gathering RHS rows for lookup join", total, inputOutputMeta, NULL)); } } if (rightCollector && rightCollector->hasSpilt()) mergeStats(PARENT::inactiveStats, rightCollector); if (exception) throw exception.getClear();

jakesmith · 2024-08-20T09:24:27Z

thorlcr/thorutil/thmem.cpp

@@ -234,6 +234,7 @@ class CSpillableStreamBase : public CSpillable
    unsigned spillCompInfo;
    CThorSpillableRowArray rows;
    Owned<CFileOwner> spillFile;
+    CRuntimeStatisticCollection inactiveStats;


these are not really inactiveStats at this stage, I'd rename 'stats'

comment missed? still think clearer if this is renamed

jakesmith · 2024-08-20T09:28:15Z

thorlcr/thorutil/thmem.cpp

 {
    throwOnOom = false;
 }

 CThorSpillableRowArray::CThorSpillableRowArray(CActivityBase &activity, IThorRowInterfaces *rowIf, EmptyRowSemantics emptyRowSemantics, StableSortFlag stableSort, rowidx_t initialSize, size32_t _commitDelta)
-    : CThorExpandingRowArray(activity, rowIf, ers_forbidden, stableSort, false, initialSize), commitDelta(_commitDelta)
+    : CThorExpandingRowArray(activity, rowIf, ers_forbidden, stableSort, false, initialSize), commitDelta(_commitDelta), inactiveStats(spillStatistics)


we have a mixture of some utility containers (inc jfile objects) that create standard stats (iCSmartRowBuffer creates regular IO stats). and others that now create 'spillStatistics'.
It would be clearer/more consistent, if we always let the utility classes create standard stats., and only in the activity code were they remapped.
i.e. I don't think CThorSpillableRowArray should itself capture "spillStatistics" and do the remapping. The activities that use them should do the remapping.

jakesmith

@shamser - please see comments.

I think it would aide the review process, to break this down into 2 or more separate PR's if it can be made consistent.
e.g. > make CThorRowCollectorBase use of stats from CThorSpillableRowArray for more accurate and simpler temp stats tracking (for CThorRowCollectorBase)
(and any knock on effects) could be own PR?

thorlcr/activities/lookupjoin/thlookupjoinslave.cpp

jakesmith · 2024-08-27T12:37:58Z

thorlcr/thorutil/thmem.cpp

@@ -234,6 +234,7 @@ class CSpillableStreamBase : public CSpillable
    unsigned spillCompInfo;
    CThorSpillableRowArray rows;
    Owned<CFileOwner> spillFile;
+    CRuntimeStatisticCollection inactiveStats;


comment missed? still think clearer if this is renamed

thorlcr/thorutil/thmem.cpp

jakesmith

@shamser - looks good, only one very minor comment.

jakesmith · 2024-09-03T15:14:44Z

thorlcr/thorutil/thmem.cpp

@@ -1638,11 +1648,8 @@ class CThorRowCollectorBase : public CSpillable
    Owned<CSharedSpillableRowSet> spillableRowSet;
    unsigned options = 0;
    unsigned spillCompInfo = 0;
-    RelaxedAtomic<unsigned> statOverflowCount{0};
    RelaxedAtomic<offset_t> statSizeSpill{0};


minor: now also looks unused

jakesmith

@shamser - looks good. Please squash.

jakesmith

@shamser - reapproving.

ghalliday · 2024-09-10T15:30:25Z

@jakesmith are we still happy for this to go in 9.6.x? Confident it will not cause a regression?

ghalliday

@shamser one question

ghalliday · 2024-09-10T15:33:14Z

thorlcr/activities/lookupjoin/thlookupjoinslave.cpp

@@ -3001,12 +3022,13 @@ class CLookupJoinActivityBase : public CInMemJoinBase<HTHELPER, IHThorHashJoinAr
        GetTempFilePath(tempFilename, "lookup_local");
        ActPrintLog("Overflowing RHS broadcast rows to spill file: %s", tempFilename.str());
        overflowWriteFile.setown(container.queryActivity()->createOwnedTempFile(tempFilename.str()));
-        overflowWriteStream.setown(createRowWriter(&(overflowWriteFile->queryIFile()), queryRowInterfaces(rightITDL), rwFlags));
+        overflowWriteFileIO.setown(overflowWriteFile->queryIFile().open(IFOcreate));


@shamser should this be being cleared at some point - otherwise I think it will stay open longer than you want.

@ghalliday I'm now clearing overflowWriteFileIO at the same time that overflowWriteStream is cleared (Line 2118).

ghalliday · 2024-09-13T13:22:53Z

@shamser I think you have accidentally included a vcpkg update. Please squash, but remove that extra update.

The current temp file statistics for smartjoin did not include all stats from all temp files: 1) temp files were closed before its sizes were recorded in the stats 2) stats from some types of temp files were not being tracked such as overflowWriteFile from RHS 3) stats from temp files that were closed in CSpillableStreamBase were not preserved 4) peak temp file size was not tracked in CThorSpillableRowArray 5) make CThorRowCollectorBase use of stats from CThorSpillableRowArray for more accurate and simpler temp stats tracking. Signed-off-by: Shamser Ahmed <[email protected]> HPCC-32193 Close overflowWriteFileIO when it's no longer needed Signed-off-by: Shamser Ahmed <[email protected]>

shamser force-pushed the issue32193 branch 2 times, most recently from 30cc34f to 2a67393 Compare July 16, 2024 08:36

shamser force-pushed the issue32193 branch from 2a67393 to 2f39239 Compare August 8, 2024 08:28

shamser changed the base branch from candidate-9.8.x to candidate-9.6.x August 8, 2024 08:29

shamser force-pushed the issue32193 branch 2 times, most recently from 5aaba8c to 333932c Compare August 8, 2024 09:40

shamser marked this pull request as ready for review August 8, 2024 09:40

shamser requested review from ghalliday and jakesmith August 8, 2024 09:40

ghalliday reviewed Aug 19, 2024

View reviewed changes

jakesmith reviewed Aug 20, 2024

View reviewed changes

shamser force-pushed the issue32193 branch from 16adfd2 to ad04504 Compare August 22, 2024 13:30

shamser requested a review from jakesmith August 22, 2024 13:30

jakesmith reviewed Aug 27, 2024

View reviewed changes

shamser requested a review from jakesmith September 2, 2024 10:22

jakesmith reviewed Sep 3, 2024

View reviewed changes

shamser requested a review from jakesmith September 4, 2024 15:31

jakesmith approved these changes Sep 5, 2024

View reviewed changes

shamser force-pushed the issue32193 branch from 41ae8b4 to 8c5b39e Compare September 6, 2024 13:35

shamser requested a review from jakesmith September 6, 2024 13:35

jakesmith approved these changes Sep 10, 2024

View reviewed changes

ghalliday reviewed Sep 10, 2024

View reviewed changes

shamser requested a review from ghalliday September 13, 2024 11:33

shamser force-pushed the issue32193 branch from 45f3f23 to 5b67f03 Compare September 13, 2024 15:19

ghalliday approved these changes Sep 16, 2024

View reviewed changes

ghalliday merged commit 0f9e143 into hpcc-systems:candidate-9.6.x Sep 16, 2024
52 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HPCC-32193 Fix some issues with spill stats in smart join activity #18866

HPCC-32193 Fix some issues with spill stats in smart join activity #18866

shamser commented Jul 10, 2024 •

edited

Loading

github-actions bot commented Jul 10, 2024

ghalliday left a comment

ghalliday Aug 19, 2024

jakesmith left a comment

jakesmith Aug 20, 2024

jakesmith Aug 20, 2024

jakesmith Aug 20, 2024

jakesmith Aug 20, 2024

jakesmith Aug 27, 2024

jakesmith Aug 20, 2024

jakesmith left a comment

jakesmith Aug 27, 2024

jakesmith left a comment

jakesmith Sep 3, 2024

jakesmith left a comment

jakesmith left a comment

ghalliday commented Sep 10, 2024

ghalliday left a comment

ghalliday Sep 10, 2024

shamser Sep 13, 2024

ghalliday commented Sep 13, 2024

HPCC-32193 Fix some issues with spill stats in smart join activity #18866

HPCC-32193 Fix some issues with spill stats in smart join activity #18866

Conversation

shamser commented Jul 10, 2024 • edited Loading

Type of change:

Checklist:

Smoketest:

Testing:

github-actions bot commented Jul 10, 2024

ghalliday 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

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

Choose a reason for hiding this comment

jakesmith left a comment

Choose a reason for hiding this comment

jakesmith left a comment

Choose a reason for hiding this comment

ghalliday commented Sep 10, 2024

ghalliday left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghalliday commented Sep 13, 2024

shamser commented Jul 10, 2024 •

edited

Loading