HPCC-29817 Eliminate seeks, scans & wildseeks from KeyStatsCollector and track cache stats in hthor #17541

shamser · 2023-07-03T14:05:47Z

…

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:

jakesmith

@shamser - looks in good in general, please see feedback.

jakesmith · 2023-07-10T18:01:04Z

common/thorhelper/thorcommon.hpp

@@ -672,6 +673,108 @@ class THORHELPER_API IndirectCodeContext : implements ICodeContext
 protected:
    ICodeContext * ctx;
 };
+class CThorBaseContextLogger : public CSimpleInterfaceOf<IContextLogger>


trivial/formatting: we norrmally have a new line before a new class definition.

system/jlib/jstats.cpp

jakesmith · 2023-07-10T18:39:04Z

common/thorhelper/thorcommon.hpp

+    mutable CRuntimeStatisticCollection stats;
+
+public:
+    CThorBaseContextLogger() : stats(jhtreeCacheStatistics)


To make this a more generliazed stats logger context, it would be good to rename it (as it's not just 'Thor') - call it something like CStatsContextLogger, and pass in the mapping:

CThorBaseContextLogger(CRuntimeStatisticCollection &_mapping) : mapping(_mapping)

jakesmith · 2023-07-10T18:42:23Z

common/thorhelper/thorcommon.hpp

+    {
+        va_list args;
+        va_start(args, format);
+        CTXLOGva(MCdebugProgress, unknownJob, NoLogMsgCode, format, args);


I think worth passing in a LogMsgJobInfo & to the ctor and using here and in logOperatorExceptionVA below.
then can the CThorContextLogger::CTXLOG and CThorContextLogger::logOperatorExceptionVA can be deleted.

jakesmith · 2023-07-10T18:46:47Z

ecl/hthor/hthorkey.cpp

@@ -3036,11 +3012,11 @@ class KeyedLookupPartHandler : extends ThreadedPartHandler<MatchSet>, implements
    Owned<IKeyManager> manager;
    IAgentContext &agent;
    DistributedKeyLookupHandler * tlk;
-
+    CHThorContextLogger &contextLogger;


minor: unless needs to be an implementation reference, better if it was an interface reference (IContextLogger &)
Declaring it as a CHThorContextLogger makes it look like it needs to / code relies on it.

and similarly in a few other places

jakesmith · 2023-07-10T19:04:44Z

ecl/hthor/hthor.ipp

+// improvement: override constructor to store setHttpIdHeaderNames, override CTXLOG & logOperatorExceptionVA to log LogMsgJobInfo
+class CHThorContextLogger : public CThorBaseContextLogger
+{
+};


With a rename of the base to CStatsContextLogger (+ passing in mapping + job),
I think this can be deleted in favour of using CStatsContextLogger.
Thor's derivation can remain for it's specialization if need be.

jakesmith · 2023-07-10T19:09:25Z

ecl/hthor/hthorkey.cpp

    unsigned postFiltered;
-    unsigned wildseeks;
+    CHThorContextLogger contextLogger;


Does there need to be a logger per activity type that uses one..
Could there just be 1 in the agent (which is how it's done in roxie) ?

jakesmith · 2023-07-10T19:14:14Z

roxie/ccd/ccdcontext.cpp

@@ -1179,7 +1179,7 @@ class InlineXmlDataReader : public WorkUnitRowReaderBase

 //---------------------------------------------------------------------------------------

-static const StatisticsMapping graphStatistics({});
+static const StatisticsMapping roxieGraphStatistics({});


no need to rename, related to comment re. moving Thor mapping (back to Thor code),

jakesmith · 2023-07-10T19:28:06Z

roxie/ccd/ccdactivities.cpp

+                unsigned __int64 seeks = stats.getStatisticValue(StNumIndexSeeks);
+                unsigned __int64 scans = stats.getStatisticValue(StNumIndexScans);
+                unsigned __int64 skips = stats.getStatisticValue(StNumIndexSkips);
+                logctx.CTXLOG("Indexread returning result set %d rows from %" I64F"u seeks, %" I64F"u scans, %" I64F"u skips", processed-processedBefore, seeks, scans, skips);


may not be trivial, but wondering if the 4 incidents of this type of code ( I think they're same with different messages?) could be commoned up.

Possibly but would prefer to keep as it is to stop the scope of this jira from creeping.

jakesmith · 2023-07-10T19:29:49Z

thorlcr/thorutil/thormisc.hpp

 public:
-    CThorContextLogger() : stats(jhtreeCacheStatistics)
+    CThorContextLogger()
    {
        if (globals->hasProp("@httpGlobalIdHeader"))


perhaps pass a IPropertyTree in to CStatsContextLogger too and move this code there.

jakesmith

@shamser - made 1st review pass at this, it does look a lot cleaner overall, but I think IContextLogger/IRoxieContextLogger and their implementations need looking at/refactoring.
See comments.

jakesmith · 2023-08-08T08:41:48Z

system/jlib/jlog.hpp

+    const LogMsgJobInfo & logMsgJobInfo;
+    LogTrace logTrace;
+public:
+    IContextLogger(const IPropertyTree *cfg, const LogMsgJobInfo & _logMsgJobInfo=unknownJob) : logMsgJobInfo(_logMsgJobInfo)


interfaces should really not have concrete implementations.
I realize IContextLogger already had some, but with a ctor it looks pointedly wrong.

Whatever is implementing IContextLogger should have the ctor and implement the concrete implementations.

In this case, if we want a default implementation with a ctor, there should be a CDefaultContextLogger or similar, and other implementations can derive from it.

I see that IRoxieContextLogger also broke this rule, but given a default IContextLogger implementation, implementations of IRoxieContextLogger can also derive from that default implementation and override what they want to (i.e. the "ContextLogger" implementation in ccd.hpp)

jakesmith · 2023-08-08T08:44:02Z

common/thorhelper/roxiehelper.ipp

@@ -59,6 +59,7 @@ enum TracingCategory
 class LogItem;
 interface IRoxieContextLogger : extends IContextLogger
 {
+    IRoxieContextLogger(IPropertyTree *cfg) : IContextLogger(cfg) {}


see related comments in jlog.hpp re. IContextLogger.
interfaces should really not have constructors and concrete implementations.

jakesmith · 2023-08-08T08:53:16Z

roxie/ccd/ccd.hpp

    ContextLogger(const ContextLogger &);  // Disable copy constructor
 public:
    IMPLEMENT_IINTERFACE;

-    ContextLogger() : stats(accumulatedStatistics, true)
+    ContextLogger() :  IRoxieContextLogger(topology), stats(accumulatedStatistics, true)


see other comments re. interfaces shouldn't implement.
I know this is not entirely new, but looks very wrong now with interface taking ctor.
There should instead be a base class that implements IRoxieContextLogger and inherit from it.

jakesmith

@shamser - looks cleaner, but it would best if this non-trivial refactoring of these IContextLogger and IRoxieContextLogger implementations were in a separate PR.

And I think can be done without removing roxie's extended interface (IRoxieContextLogger), which should also be pure.

The implementations could share a common base, either by the concrete implementations implementing the pure methods and calling the common base methods, or to avoid all the trivial 1-liner method implementations, the base class could be a template class, that takes either IContextLogger or IRoxieContextLogger, e.g.:

template class <INTERFACE>
class CDefaultContextLogger : public CInterfaceOf<INTERFACE>
{
..
}

class CBaseRoxieContextLogger : public CDefaultContextLogger<IRoxieContextLogger>
{
..
}

That way, the roxie code will not need changing to use a class and can continue to use IRoxieContextLogger (which will now be purely abstract).

ghalliday

I don't see any problems.

ghalliday · 2023-09-22T09:57:46Z

@shamser the smoke test is failing because it fails to build.

jakesmith

@shamser - looks good, 2 minor comments

jakesmith · 2023-09-22T12:42:32Z

common/thorhelper/thorcommon.hpp

+class CStatsContextLogger : public CSimpleInterfaceOf<IContextLogger>
+{
+protected:
+    const LogMsgJobInfo & job;


it may not make any practical difference (if the passed in LogMsgJobInfo stay in scope/aren't destroyed),
but I think when keeping a LogMsgJobInfo we normally copy it (only by reference when not taking a copy/referenced copy) - e.g. see LogMsg class member.

jakesmith · 2023-09-22T12:52:13Z

system/jlib/jstats.h

@@ -944,4 +943,6 @@ class jlib_decl RuntimeStatisticTarget : implements IStatisticTarget
 extern jlib_decl StringBuffer & formatMoney(StringBuffer &out, unsigned __int64 value);
 extern jlib_decl stat_type aggregateStatistic(StatisticKind kind, IStatisticCollection * statsCollection);

+extern jlib_decl const StatisticsMapping jhtreeCacheStatistics;


trivial: would be easier to find if moved with the other jstats.h StatisticMappings (around line 494)

jakesmith

@shamser - looks good, please squash.

ghalliday · 2023-09-28T19:07:57Z

@shamser needs rebasing before it can be merged

…and track cache stats in hthor Signed-off-by: Shamser Ahmed <[email protected]>

jakesmith

@shamser - looks good

jakesmith · 2023-10-04T09:07:47Z

common/thorhelper/thorcommon.hpp

+    Owned<ISpan> activeSpan;
+    mutable CRuntimeStatisticCollection stats;
+public:
+    CStatsContextLogger(const CRuntimeStatisticCollection  &_mapping, const LogMsgJobInfo & _job=unknownJob) : job(_job), stats(_mapping) {}


trivial/formatting: double space before &_mapping

jakesmith · 2023-10-04T09:08:18Z

common/thorhelper/thorcommon.hpp

+public:
+    CStatsContextLogger(const CRuntimeStatisticCollection  &_mapping, const LogMsgJobInfo & _job=unknownJob) : job(_job), stats(_mapping) {}
+
+    virtual void CTXLOGva(const LogMsgCategory & cat, const LogMsgJobInfo & job, LogMsgCode code, const char *format, va_list args) const override  __attribute__((format(printf,5,0)))


trivial/formatting: double space before __attribute

shamser changed the title ~~HPCC-29492 Eliminate querySeeks, queryScan & queryWildSeeks from ContextLogger~~ HPCC-29817 Eliminate querySeeks, queryScan & queryWildSeeks from ContextLogger Jul 3, 2023

shamser force-pushed the issue29817 branch from 9d4deab to 0708914 Compare July 3, 2023 14:09

shamser requested a review from jakesmith July 4, 2023 09:03

jakesmith reviewed Jul 10, 2023

View reviewed changes

shamser force-pushed the issue29817 branch 10 times, most recently from a63c70a to 2b74f3b Compare July 20, 2023 10:13

shamser requested a review from jakesmith August 1, 2023 10:06

jakesmith requested changes Aug 8, 2023

View reviewed changes

shamser force-pushed the issue29817 branch 2 times, most recently from 4a8c9b5 to a72b6ae Compare August 14, 2023 12:20

shamser requested a review from jakesmith August 14, 2023 13:06

jakesmith reviewed Aug 15, 2023

View reviewed changes

jakesmith mentioned this pull request Sep 14, 2023

HPCC-30103 Make IContextLogger and IRoxieContextLogger pure #17727

Closed

39 tasks

shamser changed the title ~~HPCC-29817 Eliminate querySeeks, queryScan & queryWildSeeks from ContextLogger~~ HPCC-29817 Eliminate querySeeks, queryScan & queryWildSeeks from KeyStatsCollector Sep 21, 2023

shamser force-pushed the issue29817 branch from a72b6ae to aec549b Compare September 21, 2023 16:49

shamser changed the base branch from candidate-9.2.x to candidate-9.4.x September 21, 2023 16:50

shamser force-pushed the issue29817 branch from aec549b to a3d679b Compare September 21, 2023 17:00

shamser changed the title ~~HPCC-29817 Eliminate querySeeks, queryScan & queryWildSeeks from KeyStatsCollector~~ HPCC-29817 Eliminate seeks, scans & wildseeks from KeyStatsCollector and track cache stats in hthor Sep 21, 2023

shamser requested review from jakesmith and ghalliday September 21, 2023 17:02

shamser force-pushed the issue29817 branch 2 times, most recently from a167289 to 5260a0a Compare September 21, 2023 17:19

ghalliday approved these changes Sep 22, 2023

View reviewed changes

shamser force-pushed the issue29817 branch 2 times, most recently from b10392c to ee606f3 Compare September 22, 2023 11:23

jakesmith reviewed Sep 22, 2023

View reviewed changes

shamser force-pushed the issue29817 branch 2 times, most recently from fc1c60a to cdfe748 Compare September 22, 2023 13:51

shamser requested a review from jakesmith September 22, 2023 14:36

jakesmith approved these changes Sep 25, 2023

View reviewed changes

shamser force-pushed the issue29817 branch from 357b21f to 8d6f849 Compare September 25, 2023 09:27

HPCC-29817 Eliminate seeks, scans & wildseeks from KeyStatsCollector …

ef0ab47

…and track cache stats in hthor Signed-off-by: Shamser Ahmed <[email protected]>

shamser force-pushed the issue29817 branch from 8d6f849 to ef0ab47 Compare October 3, 2023 12:56

shamser requested review from ghalliday and jakesmith October 3, 2023 12:57

ghalliday approved these changes Oct 4, 2023

View reviewed changes

jakesmith approved these changes Oct 4, 2023

View reviewed changes

ghalliday merged commit 8d6574d into hpcc-systems:candidate-9.4.x Oct 4, 2023
49 of 50 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HPCC-29817 Eliminate seeks, scans & wildseeks from KeyStatsCollector and track cache stats in hthor #17541

HPCC-29817 Eliminate seeks, scans & wildseeks from KeyStatsCollector and track cache stats in hthor #17541

shamser commented Jul 3, 2023 •

edited

Loading

jakesmith left a comment

jakesmith Jul 10, 2023

jakesmith Jul 10, 2023

jakesmith Jul 10, 2023

jakesmith Jul 10, 2023

jakesmith Jul 10, 2023

jakesmith Jul 10, 2023

jakesmith Jul 10, 2023

jakesmith Jul 10, 2023

shamser Sep 21, 2023

jakesmith Jul 10, 2023

jakesmith left a comment

jakesmith Aug 8, 2023

jakesmith Aug 8, 2023

jakesmith Aug 8, 2023

jakesmith left a comment

ghalliday left a comment

ghalliday commented Sep 22, 2023

jakesmith left a comment

jakesmith Sep 22, 2023

jakesmith Sep 22, 2023

jakesmith left a comment

ghalliday commented Sep 28, 2023

jakesmith left a comment

jakesmith Oct 4, 2023

jakesmith Oct 4, 2023

HPCC-29817 Eliminate seeks, scans & wildseeks from KeyStatsCollector and track cache stats in hthor #17541

HPCC-29817 Eliminate seeks, scans & wildseeks from KeyStatsCollector and track cache stats in hthor #17541

Conversation

shamser commented Jul 3, 2023 • edited Loading

Type of change:

Checklist:

Smoketest:

Testing:

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

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

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

ghalliday left a comment

Choose a reason for hiding this comment

ghalliday commented Sep 22, 2023

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

jakesmith left a comment

Choose a reason for hiding this comment

ghalliday commented Sep 28, 2023

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

shamser commented Jul 3, 2023 •

edited

Loading