HPCC-32108 Gather and report regex cache statistics #19193

dcamper · 2024-10-11T15:25:00Z

This PR also opens up ISectionTimer so that other code can define and set nested statistics. In addition, a general type of "peak number/count" statistic has been defined.

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:

Manual test via workunit submission, staring at ECL Watch, and reviewing wutool output.

github-actions · 2024-10-11T15:25:21Z

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

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

shamser

There are just a few minor suggestions.

shamser · 2024-10-14T09:32:24Z

common/thorhelper/thorstats.cpp

@@ -20,34 +20,63 @@
 #include "thorstats.hpp"
 #include "jdebug.hpp"

+// Available sets of statistics for a nested section
+const StatisticsMapping defaultNestedSectionStatistics({StCycleLocalExecuteCycles, StTimeLocalExecute, StNumStarts, StNumStops});
+const StatisticsMapping genericCacheNestedSectionStatistics({StCycleLocalExecuteCycles, StTimeLocalExecute, StNumStarts, StNumStops, StNumCacheAdds, StNumCacheHits, StNumPeakCacheObjects});


genericCacheNestedSectionStatistics should build on defaultNestedSectionStatistics so that if defaults are ever changed, the changes are reflected in genericCacheNestedSectionStatistics:

const StatisticsMapping genericCacheNestedSectionStatistics(defaultNestedSectionStatistics, {StNumCacheAdds, StNumCacheHits, StNumPeakCacheObjects})

Suggested change

const StatisticsMapping genericCacheNestedSectionStatistics({StCycleLocalExecuteCycles, StTimeLocalExecute, StNumStarts, StNumStops, StNumCacheAdds, StNumCacheHits, StNumPeakCacheObjects});

const StatisticsMapping genericCacheNestedSectionStatistics({StCycleLocalExecuteCycles, StTimeLocalExecute, StNumStarts, StNumStops, StNumCacheAdds, StNumCacheHits, StNumPeakCacheObjects});

Fixed. Note that defaultNestedSectionStatistics is actually the last arg to the next constructor rather than the first.

shamser · 2024-10-14T09:44:36Z

common/thorhelper/thorstats.cpp

 }

 unsigned __int64 ThorSectionTimer::getStartCycles()
 {
-    starts.addAtomic(1);
+    nested.queryStatistic(StNumStarts).addAtomic(1);


minor:

nested.addStatisticAtomic(StNumStarts, 1)

shamser · 2024-10-14T09:45:35Z

common/thorhelper/thorstats.cpp

-    elapsed.addAtomic(delay);
-    stops.addAtomic(1);
+    nested.queryStatistic(StCycleLocalExecuteCycles).addAtomic(delay);
+    nested.queryStatistic(StNumStops).addAtomic(1);


minor: nested.addStatisticAtomic should be used.

shamser · 2024-10-14T09:46:48Z

common/thorhelper/thorstats.cpp

+
+void ThorSectionTimer::mergeStatistic(__int64 kind, unsigned __int64 value)
+{
+    nested.queryStatistic(static_cast<StatisticKind>(kind)).merge(value, queryMergeMode(static_cast<StatisticKind>(kind)));


nested.mergeStatistic(static_cast(kind), value) should be used. MergeMode will be handled by CRuntimeStatistic

shamser · 2024-10-14T09:47:10Z

common/thorhelper/thorstats.cpp

+
+void ThorSectionTimer::setStatistic(__int64 kind, unsigned __int64 value)
+{
+    nested.queryStatistic(static_cast<StatisticKind>(kind)).set(value);


minor: preferable to use nested.setStatistic()

shamser · 2024-10-14T09:47:28Z

common/thorhelper/thorstats.cpp

+
+void ThorSectionTimer::addStatistic(__int64 kind, unsigned __int64 value)
+{
+    nested.queryStatistic(static_cast<StatisticKind>(kind)).addAtomic(value);


minor: preferable to use nested.addStatisticAtomic()

shamser · 2024-10-14T09:52:06Z

ecl/hqlcpp/hqlhtcpp.cpp

@@ -18828,7 +18828,16 @@ void HqlCppTranslator::buildTimerBase(BuildCtx & ctx, CHqlBoundExpr & boundTimer
    HqlExprArray registerArgs;
    registerArgs.append(*getSizetConstant(activityId));
    registerArgs.append(*createConstant(name));
-    OwnedHqlExpr call = bindFunctionCall(registerTimerId, registerArgs);
+    OwnedHqlExpr call;
+    if (statsOption == 0)


clearer to

if (statsOption==ThorStatDefault)

I agree, but making ThorStatDefault visible would require including a header and I am loathe to do so at this point. I did add a comment here, nothing what the zero actually means. I can, of course, include the header and use the enum value name instead if you really think it is worth it; let me know.

ecl/hqlcpp/hqlhtcpp.cpp

ghalliday

@dcamper looks like a good change. A few minor comments (including those from Shamser), but looks like it is close to ready to merge.

ghalliday · 2024-10-14T14:41:54Z

common/thorhelper/thorstats.hpp

    StringAttr name;
+    CRuntimeStatisticCollection & nested;


picky: From this classes' point of view it is not nested, so "stats" would be a simpler/clearer name.

ghalliday · 2024-10-14T14:44:24Z

thorlcr/graph/thgraph.cpp

@@ -361,6 +361,20 @@ ISectionTimer * CActivityCodeContext::registerTimer(unsigned activityId, const c
    return timer;
 }

+ISectionTimer * CActivityCodeContext::registerStatsTimer(unsigned activityId, const char * name, unsigned int statsOption)


cleanup: I would reimplement the function above by calling this with the default argument. Reduces code, and ensure the new function is tested in many existing situations.

Thanks for catching this. It was on my TODO and I missed it.

rtl/eclrtl/eclregex.cpp

ghalliday · 2024-10-14T14:52:01Z

ecl/hqlcpp/hqlcpp.cpp

@@ -6127,7 +6129,9 @@ void HqlCppTranslator::doBuildCall(BuildCtx & ctx, const CHqlBoundTarget * tgt,
        const char * name = str(external->queryId());
        if (getStringValue(nameTemp, queryAttributeChild(external, timerAtom, 0)).length())
            name = nameTemp;
-        buildHelperTimer(ctx, boundTimer, name);
+        // Grab optional ThorStatOption enum (as an int)
+        int statOption = getIntValue(queryAttributeChild(external, timerAtom, 1), 0);


type should be consistent with the call above (int v __int64)

ghalliday · 2024-10-14T14:52:45Z

roxie/ccd/ccdcontext.cpp

@@ -2147,6 +2147,18 @@ class CRoxieContextBase : implements IRoxieAgentContext, implements ICodeContext
        }
        return timer;
    }
+    virtual ISectionTimer * registerStatsTimer(unsigned activityId, const char * name, unsigned int statsOption)


again (and in all other non-trivial occasions) worth re-implementing the old function by calling the new.

dcamper · 2024-10-14T15:56:21Z

@ghalliday Changes made, please re-review. Thanks!

shamser

Looks good.

dcamper · 2024-10-15T20:53:01Z

Aside: Force-pushed an update to resolve a conflict when I rebased with the latest master branch updates.

ghalliday

@dcamper looks good. A few final comments (which can be ignored). Please squash.

ghalliday · 2024-10-15T13:49:34Z

roxie/ccd/ccdserver.cpp

@@ -1309,6 +1316,18 @@ class CRoxieServerActivity : implements CInterfaceOf<IRoxieServerActivity>, impl
        }
        return timer;
    }
+    virtual ISectionTimer *registerStatsTimer(unsigned activityId, const char * name, unsigned int statsOption)
+    {
+        CriticalBlock b(statscrit); // reuse statscrit to protect functionTimers - it will not be held concurrently


trivial: another case where the old could call the new

Argh. I missed that one. Thanks for catching it.

rtl/eclrtl/eclregex.cpp

dcamper · 2024-10-16T12:04:41Z

@ghalliday Squashed. Thanks!

ghalliday

@dcamper see comment

ghalliday · 2024-10-16T14:00:22Z

common/thorhelper/thorstats.cpp

@@ -20,34 +20,63 @@
 #include "thorstats.hpp"
 #include "jdebug.hpp"

+// Available sets of statistics for a nested section
+const StatisticsMapping defaultNestedSectionStatistics({StCycleLocalExecuteCycles, StTimeLocalExecute, StNumStarts, StNumStops});
+const StatisticsMapping genericCacheNestedSectionStatistics({StNumCacheHits, StNumPeakCacheObjects}, defaultNestedSectionStatistics);


Missing StNumCacheAdds...

dcamper requested review from ghalliday and shamser October 11, 2024 15:25

shamser requested changes Oct 14, 2024

View reviewed changes

dcamper requested a review from shamser October 14, 2024 13:16

ghalliday reviewed Oct 14, 2024

View reviewed changes

dcamper requested a review from ghalliday October 14, 2024 15:56

shamser approved these changes Oct 15, 2024

View reviewed changes

dcamper force-pushed the hpcc-32108-regex-cache-stats branch from 1685b28 to 4ec6e9b Compare October 15, 2024 20:50

ghalliday approved these changes Oct 16, 2024

View reviewed changes

dcamper force-pushed the hpcc-32108-regex-cache-stats branch from 4ec6e9b to fde7031 Compare October 16, 2024 12:04

ghalliday reviewed Oct 16, 2024

View reviewed changes

HPCC-32108 Gather and report regex cache statistics

0f6e244

dcamper force-pushed the hpcc-32108-regex-cache-stats branch from fde7031 to 0f6e244 Compare October 16, 2024 14:05

ghalliday merged commit 474e817 into hpcc-systems:master Oct 16, 2024
51 of 52 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HPCC-32108 Gather and report regex cache statistics #19193

HPCC-32108 Gather and report regex cache statistics #19193

dcamper commented Oct 11, 2024

github-actions bot commented Oct 11, 2024

shamser left a comment

shamser Oct 14, 2024

dcamper Oct 14, 2024

shamser Oct 14, 2024

shamser Oct 14, 2024

shamser Oct 14, 2024

shamser Oct 14, 2024

shamser Oct 14, 2024

shamser Oct 14, 2024

dcamper Oct 14, 2024

ghalliday left a comment

ghalliday Oct 14, 2024

ghalliday Oct 14, 2024

dcamper Oct 14, 2024

ghalliday Oct 14, 2024

dcamper Oct 14, 2024

ghalliday Oct 14, 2024

dcamper commented Oct 14, 2024

shamser left a comment

dcamper commented Oct 15, 2024

ghalliday left a comment

ghalliday Oct 15, 2024

dcamper Oct 16, 2024

dcamper commented Oct 16, 2024

ghalliday left a comment

ghalliday Oct 16, 2024

dcamper Oct 16, 2024

	const StatisticsMapping genericCacheNestedSectionStatistics({StCycleLocalExecuteCycles, StTimeLocalExecute, StNumStarts, StNumStops, StNumCacheAdds, StNumCacheHits, StNumPeakCacheObjects});
	const StatisticsMapping genericCacheNestedSectionStatistics({StCycleLocalExecuteCycles, StTimeLocalExecute, StNumStarts, StNumStops, StNumCacheAdds, StNumCacheHits, StNumPeakCacheObjects});

HPCC-32108 Gather and report regex cache statistics #19193

HPCC-32108 Gather and report regex cache statistics #19193

Conversation

dcamper commented Oct 11, 2024

Type of change:

Checklist:

Smoketest:

Testing:

github-actions bot commented Oct 11, 2024

shamser 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

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

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

dcamper commented Oct 14, 2024

shamser left a comment

Choose a reason for hiding this comment

dcamper commented Oct 15, 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

dcamper commented Oct 16, 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