HPCC-32138 Generic remapping merge function to remap disk stats to spill stats #18819

shamser · 2024-06-27T09:34:08Z

…ent a generic mergeStats function that will remap to the stat names before setting them

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-06-27T09:34:24Z

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

Jirabot Action Result:
Workflow Transition: Merge Pending
Updated PR

jakesmith

@shamser - please see comments.

thorlcr/thorutil/thormisc.cpp

thorlcr/activities/hashdistrib/thhashdistribslave.cpp

thorlcr/thorutil/thbuf.cpp

jakesmith · 2024-07-11T10:35:14Z

thorlcr/thorutil/thbuf.cpp

        if (likely(iFileIO))
-            v = iFileIO->getStatistic(useKind);
-        v += inactiveStats.getStatisticValue(useKind);
+            v = iFileIO->getStatistic(kind);


this is potential thread unsafe. iFileIO could be null at this point, i.e. closeWriter could get in between the test and this line
Also shouldn't this still be v += ? inactive + current IFileIO stats?

re. the thread safety an alternative is not to examine the potentially changing thread-unsafe iFileIO here at all,
instead merge the stats into a stats container (e.g. inactive) when they are written, i.e. at the end of writeRowsFromInput().

@shamser - this potentially thread unsafe implementation seems to have been reintroduced in latest version

thorlcr/thorutil/thbuf.cpp

system/jlib/jstats.h

thorlcr/thorutil/thormisc.cpp

jakesmith

@shamser - looks good. Please squash.

jakesmith

@shamser - looks good.

jakesmith · 2024-07-16T09:16:20Z

@shamser - can you retarget this to 9.6 ?

jakesmith

@shamser - looks good.

jakesmith

approved.
@ghalliday - please merge.

ghalliday

@shamser I think this is double counting. Have we got tests to verify this isn't happening (e.g. set watchdog frequency to 1s to make it obvious)?

ghalliday · 2024-07-17T14:53:32Z

thorlcr/thorutil/thormisc.cpp

@@ -99,6 +99,11 @@ const StatisticsMapping hashDistribActivityStatistics({StNumLocalRows, StNumRemo
 const StatisticsMapping nsplitterActivityStatistics({}, spillStatistics, basicActivityStatistics);
 const StatisticsMapping spillingWriteAheadStatistics({}, spillStatistics);

+const StatKindMap diskToTempStatsMap


Should ideally have a comment to indicate which is the source and which is the target

ghalliday · 2024-07-17T15:05:23Z

thorlcr/thorutil/thbuf.cpp

@@ -2549,6 +2538,7 @@ class CSharedFullSpillingWriteAhead : public CInterfaceOf<ISharedRowStreamReader
        outputStream->flush();
        totalInputRowsRead.fetch_add(newRowsWritten);
        tempFileOwner->noteSize(iFileIO->getStatistic(StSizeDiskWrite));
+        ::mergeStats(inactiveStats, iFileIO);


I think this is going to double count the stats. Each time it is called it will add the stats from the previous call as well as the rows just written.

system/jlib/jstats.cpp

jakesmith · 2024-07-18T08:50:23Z

Have we got tests to verify this isn't happening (e.g. set watchdog frequency to 1s to make it obvious)?

good idea.
@ghalliday @shamser - would a performance suite run with this (and a low globalMemorySize) be a good candidate to run and examine to flush out issues with stats. ?

jakesmith

@shamser - 1 issue.
@ghalliday - could merge as is and revisit if you prefer.

thorlcr/thorutil/thbuf.cpp

ghalliday

@shamser
Squashing these commits made it very hard to understand what changes had been madE.

NOTE this comment from Jake before:

this is potential thread unsafe. iFileIO could be null at this point, i.e. closeWriter could get in between the test and this line
Also shouldn't this still be v += ? inactive + current IFileIO stats?

Member
@jakesmith jakesmith last week
re. the thread safety an alternative is not to examine the potentially changing thread-unsafe iFileIO here at all,
instead merge the stats into a stats container (e.g. inactive) when they are written, i.e. at the end of writeRowsFromInput().

getStatistic() is thread unsafe again

jakesmith

@shamser - please see comments re. getStatistic/iFileIO thread safety issue.

jakesmith

I think there's an issue with when/where the remapping is done in the splitter/CSharedFullSpillingWriteAhead code.

Has this been tested?
I ran a quick test and not see the stats. I was expecting (Size*SpillFile stats were missing).

jakesmith · 2024-07-19T16:56:25Z

thorlcr/thorutil/thbuf.cpp

@@ -2549,6 +2543,9 @@ class CSharedFullSpillingWriteAhead : public CInterfaceOf<ISharedRowStreamReader
        outputStream->flush();
        totalInputRowsRead.fetch_add(newRowsWritten);
        tempFileOwner->noteSize(iFileIO->getStatistic(StSizeDiskWrite));
+        CRuntimeStatisticCollection currentFileStats(inactiveStats.queryMapping());


minor: I think "inactiveStats" could do with renaming now, as it's active, "stats" is probably clearer.

is it worth having some functions like the following in jstats::

template <class INTERFACE> void updateStatsDelta(CRuntimeStatisticCollection & fullStats, CRuntimeStatisticCollection & deltaStats, INTERFACE * source) { CRuntimeStatisticCollection curStats(deltaStats.queryMapping()); mergeStats(curStats, source); fullStats.updateDelta(deltaStats, curStats); }

then this would be

updateStatsDelta(inactiveStats, previousFileStats, iFileIO);

jakesmith · 2024-07-19T17:00:12Z

thorlcr/thorutil/thbuf.cpp

@@ -2549,6 +2543,9 @@ class CSharedFullSpillingWriteAhead : public CInterfaceOf<ISharedRowStreamReader
        outputStream->flush();
        totalInputRowsRead.fetch_add(newRowsWritten);
        tempFileOwner->noteSize(iFileIO->getStatistic(StSizeDiskWrite));
+        CRuntimeStatisticCollection currentFileStats(inactiveStats.queryMapping());
+        ::mergeStats(currentFileStats, iFileIO);


I'm not sure this is right.
currenFileStats has a spillingWriteAheadStatistics mapping, those stats are not within iFileIO ..

I think you either need to store as a mapping based on diskLocalStatistic, and allow the caller to remap (that's what the splitter code is doing), or, remap here and have no remapping in the caller.
It is probably better to accrue the stats in this container unmapped, and only remap in gatherStatistics.

I agree with that - accumulate in the natural form, and map when it is added to an activity.

ghalliday

@jakesmith a couple of comments. I agree with your comments.

ghalliday · 2024-07-24T11:58:33Z

thorlcr/thorutil/thbuf.cpp

@@ -2549,6 +2543,9 @@ class CSharedFullSpillingWriteAhead : public CInterfaceOf<ISharedRowStreamReader
        outputStream->flush();
        totalInputRowsRead.fetch_add(newRowsWritten);
        tempFileOwner->noteSize(iFileIO->getStatistic(StSizeDiskWrite));
+        CRuntimeStatisticCollection currentFileStats(inactiveStats.queryMapping());
+        ::mergeStats(currentFileStats, iFileIO);


I agree with that - accumulate in the natural form, and map when it is added to an activity.

ghalliday · 2024-07-24T12:07:14Z

thorlcr/thorutil/thbuf.cpp

@@ -2549,6 +2543,9 @@ class CSharedFullSpillingWriteAhead : public CInterfaceOf<ISharedRowStreamReader
        outputStream->flush();
        totalInputRowsRead.fetch_add(newRowsWritten);
        tempFileOwner->noteSize(iFileIO->getStatistic(StSizeDiskWrite));
+        CRuntimeStatisticCollection currentFileStats(inactiveStats.queryMapping());


is it worth having some functions like the following in jstats::

template <class INTERFACE> void updateStatsDelta(CRuntimeStatisticCollection & fullStats, CRuntimeStatisticCollection & deltaStats, INTERFACE * source) { CRuntimeStatisticCollection curStats(deltaStats.queryMapping()); mergeStats(curStats, source); fullStats.updateDelta(deltaStats, curStats); }

then this would be

updateStatsDelta(inactiveStats, previousFileStats, iFileIO);

ghalliday

@jakesmith I think this is good enough to merge. We should separately review all the mappings and make them consistent and clearly structured.

ghalliday · 2024-07-26T11:37:47Z

thorlcr/thorutil/thbuf.cpp

@@ -2562,7 +2555,7 @@ class CSharedFullSpillingWriteAhead : public CInterfaceOf<ISharedRowStreamReader
    explicit CSharedFullSpillingWriteAhead(CActivityBase *_activity, unsigned _numOutputs, IRowStream *_input, bool _inputGrouped, const SharedRowStreamReaderOptions &_options, IThorRowInterfaces *rowIf, const char *_baseTmpFilename, ICompressHandler *_compressHandler)
        : activity(*_activity), numOutputs(_numOutputs), input(_input), inputGrouped(_inputGrouped), options(_options), compressHandler(_compressHandler), baseTmpFilename(_baseTmpFilename),
        meta(rowIf->queryRowMetaData()), serializer(rowIf->queryRowSerializer()), allocator(rowIf->queryRowAllocator()), deserializer(rowIf->queryRowDeserializer()),
-        inactiveStats(spillingWriteAheadStatistics)
+        inactiveStats(spillingWriteAheadStatistics), previousFileStats(spillingWriteAheadStatistics)


This isn't quite the right mapping - it shouldn't include StSizePeakTempDisk, but it is good enough for this PR.

I think changed in the other pending PR - which will therefore need rebasing once this is merged.

…ill stats Signed-off-by: Shamser Ahmed <[email protected]> Signed-off-by: Jake Smith <[email protected]>

shamser force-pushed the issue32138 branch from c4d2ad8 to a28d387 Compare June 27, 2024 09:34

shamser requested a review from jakesmith June 27, 2024 15:36

shamser marked this pull request as draft June 28, 2024 10:37

shamser force-pushed the issue32138 branch from a28d387 to bb4c2bc Compare June 28, 2024 11:25

shamser marked this pull request as ready for review June 28, 2024 11:39

shamser force-pushed the issue32138 branch 2 times, most recently from 776416d to 0878095 Compare July 2, 2024 13:28

shamser changed the base branch from candidate-9.6.x to candidate-9.8.x July 2, 2024 13:28

jakesmith reviewed Jul 11, 2024

View reviewed changes

shamser requested a review from jakesmith July 11, 2024 12:47

jakesmith approved these changes Jul 12, 2024

View reviewed changes

shamser force-pushed the issue32138 branch from 0a855ad to 30bdf6a Compare July 15, 2024 08:59

shamser requested a review from jakesmith July 15, 2024 09:01

shamser force-pushed the issue32138 branch from 30bdf6a to 0594162 Compare July 16, 2024 08:34

jakesmith approved these changes Jul 16, 2024

View reviewed changes

shamser force-pushed the issue32138 branch from 0594162 to 4a00712 Compare July 16, 2024 09:58

shamser changed the base branch from candidate-9.8.x to candidate-9.6.x July 16, 2024 09:59

shamser requested a review from jakesmith July 16, 2024 09:59

jakesmith reviewed Jul 16, 2024

View reviewed changes

jakesmith previously approved these changes Jul 16, 2024

View reviewed changes

ghalliday requested changes Jul 17, 2024

View reviewed changes

shamser requested a review from ghalliday July 17, 2024 16:21

jakesmith reviewed Jul 17, 2024

View reviewed changes

system/jlib/jstats.cpp Outdated Show resolved Hide resolved

shamser force-pushed the issue32138 branch from ce3bd86 to 385d8f3 Compare July 18, 2024 08:44

shamser requested a review from jakesmith July 18, 2024 08:45

jakesmith reviewed Jul 18, 2024

View reviewed changes

thorlcr/thorutil/thbuf.cpp Show resolved Hide resolved

ghalliday requested changes Jul 18, 2024

View reviewed changes

jakesmith requested changes Jul 18, 2024

View reviewed changes

shamser requested a review from jakesmith July 19, 2024 10:13

jakesmith requested changes Jul 19, 2024

View reviewed changes

ghalliday self-requested a review July 23, 2024 14:50

ghalliday reviewed Jul 24, 2024

View reviewed changes

jakesmith force-pushed the issue32138 branch from 8beb09f to 342d6c6 Compare July 26, 2024 09:38

jakesmith requested a review from ghalliday July 26, 2024 09:39

ghalliday approved these changes Jul 26, 2024

View reviewed changes

HPCC-32138 Generic remapping merge function to remap disk stats to sp…

da14967

…ill stats Signed-off-by: Shamser Ahmed <[email protected]> Signed-off-by: Jake Smith <[email protected]>

jakesmith force-pushed the issue32138 branch from 342d6c6 to da14967 Compare July 26, 2024 11:45

ghalliday merged commit b32d940 into hpcc-systems:candidate-9.6.x Jul 26, 2024
22 of 23 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HPCC-32138 Generic remapping merge function to remap disk stats to spill stats #18819

HPCC-32138 Generic remapping merge function to remap disk stats to spill stats #18819

shamser commented Jun 27, 2024 •

edited

Loading

github-actions bot commented Jun 27, 2024

jakesmith left a comment

jakesmith Jul 11, 2024

jakesmith Jul 11, 2024

jakesmith Jul 18, 2024

jakesmith left a comment

jakesmith left a comment

jakesmith commented Jul 16, 2024

jakesmith left a comment

jakesmith left a comment

ghalliday left a comment

ghalliday Jul 17, 2024

ghalliday Jul 17, 2024

jakesmith commented Jul 18, 2024 •

edited

Loading

jakesmith left a comment

ghalliday left a comment

jakesmith left a comment

jakesmith left a comment

jakesmith Jul 19, 2024

ghalliday Jul 24, 2024

jakesmith Jul 19, 2024

ghalliday Jul 24, 2024

ghalliday left a comment

ghalliday Jul 24, 2024

ghalliday Jul 24, 2024

ghalliday left a comment

ghalliday Jul 26, 2024

jakesmith Jul 26, 2024

HPCC-32138 Generic remapping merge function to remap disk stats to spill stats #18819

HPCC-32138 Generic remapping merge function to remap disk stats to spill stats #18819

Conversation

shamser commented Jun 27, 2024 • edited Loading

Type of change:

Checklist:

Smoketest:

Testing:

github-actions bot commented Jun 27, 2024

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

jakesmith left a comment

Choose a reason for hiding this comment

jakesmith left a comment

Choose a reason for hiding this comment

jakesmith commented Jul 16, 2024

jakesmith left a comment

Choose a reason for hiding this comment

jakesmith left a comment

Choose a reason for hiding this comment

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

jakesmith commented Jul 18, 2024 • edited Loading

jakesmith left a comment

Choose a reason for hiding this comment

ghalliday left a 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

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

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 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shamser commented Jun 27, 2024 •

edited

Loading

jakesmith commented Jul 18, 2024 •

edited

Loading