HPCC-31511 Avoid keeping the workunit graphs in memory #18519

richardkchapman · 2024-04-10T11:28: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:

Compiled

github-actions · 2024-04-10T11:28:23Z

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

Jirabot Action Result:
Workflow Transition: Merge Pending
Updated PR

ghalliday · 2024-04-11T11:20:57Z

esp/platform/CMakeLists.txt

@@ -42,6 +42,7 @@ include_directories (
         ./../../system/jlib 
         ./../bindings/http/platform 
         ./../../common/environment 
+         ./../../common/workunit


I would have expected a need to add workunit to the link libraries as well.

Not sure why it builds without - probably because one of the libraries included already links to it. But I will add.

ghalliday · 2024-04-11T11:41:20Z

common/workunit/workunit.hpp

@@ -1788,5 +1788,6 @@ extern WORKUNIT_API void executeThorGraph(const char * graphName, IConstWorkUnit
 extern WORKUNIT_API TraceFlags loadTraceFlags(IConstWorkUnit * wu, const std::initializer_list<TraceOption> & y, TraceFlags dft);

 extern WORKUNIT_API bool executeGraphOnLingeringThor(IConstWorkUnit &workunit, unsigned wfid, const char *graphName);
+extern WORKUNIT_API bool workunitGraphCacheEnabled;


I'm not a fan of exported variables. We tend to provide get()/set() functions everywhere else except for roxie. I may not care enough to avoid merging though.

If you can come up with a good reason, I don't mind changing. But there are plenty of other exported variables around...

There are very few, except for roxie. (E.g. I don't think there are any in this file.)
It generates slightly worse code (an extra indirect) - not significant in this case, but in general worth avoiding. (Thread local access code is much worse...)

ghalliday · 2024-04-11T11:45:09Z

thorlcr/slave/thslavemain.cpp

@@ -393,6 +393,7 @@ int main( int argc, const char *argv[]  )
 #else
        globals.setown(loadConfiguration(globals, argv, "thor", "THOR", nullptr, nullptr, nullptr, false));
 #endif
+        workunitGraphCacheEnabled = globals->getPropBool("expert/@workunitGraphCacheEnabled", workunitGraphCacheEnabled);


I think this is too early. (See the comment in the code below....)
I would expect a similar call in the thor master.

@jakesmith Does the slave load the graph from the workunit? Does it reload it for each subgraph?

Not per subgraph, but per Thor 'job' (i.e. each workflow graph)

I think this is too early. (See the comment in the code below....)

yes, would be good to move within RegisterSelf next to similar expert tests,
and so that's bare-metal compatible, should use getExpertOptBool, i.e.:

I would expect a similar call in the thor master.

in master, a getExpertOptBool call should go in CJobManager::execute

jakesmith

@richardkchapman - please see additional comments re. Thor.

jakesmith · 2024-04-15T09:31:51Z

thorlcr/slave/thslavemain.cpp

@@ -393,6 +393,7 @@ int main( int argc, const char *argv[]  )
 #else
        globals.setown(loadConfiguration(globals, argv, "thor", "THOR", nullptr, nullptr, nullptr, false));
 #endif
+        workunitGraphCacheEnabled = globals->getPropBool("expert/@workunitGraphCacheEnabled", workunitGraphCacheEnabled);


I think this is too early. (See the comment in the code below....)

yes, would be good to move within RegisterSelf next to similar expert tests,
and so that's bare-metal compatible, should use getExpertOptBool, i.e.:

jakesmith · 2024-04-15T09:41:26Z

thorlcr/slave/thslavemain.cpp

@@ -393,6 +393,7 @@ int main( int argc, const char *argv[]  )
 #else
        globals.setown(loadConfiguration(globals, argv, "thor", "THOR", nullptr, nullptr, nullptr, false));
 #endif
+        workunitGraphCacheEnabled = globals->getPropBool("expert/@workunitGraphCacheEnabled", workunitGraphCacheEnabled);


I would expect a similar call in the thor master.

in master, a getExpertOptBool call should go in CJobManager::execute

jakesmith

@richardkchapman - looks good.

ghalliday

I think ok to merge. remaining question is about the default. I would opt for default the same in 9.4 and on in 9.6 via a separate PR.

Please squash. Thanks.

ghalliday · 2024-05-10T10:29:33Z

common/workunit/workunit.hpp

@@ -1788,5 +1788,6 @@ extern WORKUNIT_API void executeThorGraph(const char * graphName, IConstWorkUnit
 extern WORKUNIT_API TraceFlags loadTraceFlags(IConstWorkUnit * wu, const std::initializer_list<TraceOption> & y, TraceFlags dft);

 extern WORKUNIT_API bool executeGraphOnLingeringThor(IConstWorkUnit &workunit, unsigned wfid, const char *graphName);
+extern WORKUNIT_API bool workunitGraphCacheEnabled;


There are very few, except for roxie. (E.g. I don't think there are any in this file.)
It generates slightly worse code (an extra indirect) - not significant in this case, but in general worth avoiding. (Thread local access code is much worse...)

ghalliday · 2024-05-10T10:33:44Z

common/workunit/workunit.cpp

@@ -10411,8 +10411,11 @@ IPropertyTree * CLocalWUGraph::getXGMMLTreeRaw() const
    return p->getPropTree("xgmml");
 }

+bool workunitGraphCacheEnabled = false;


Should this default on in 8.4 and then off in 9.6 to help reduce the potential for unexpected regressions. Then it could be tested on development system to verify the impact.

Signed-off-by: Richard Chapman <[email protected]>

richardkchapman · 2024-05-15T09:06:40Z

I have changed the default to be true in this PR that targets 9.4, and squashed. Once merged and upmerged I can do a new PR to change the default in 9.6

ghalliday · 2024-05-17T12:21:53Z

@mckellyln this option will be available in 9.4.x. It would be worth testing it on the development systems once this build is deployed to see what effect it has on memory usage and performance.

richardkchapman requested a review from ghalliday April 10, 2024 11:28

ghalliday reviewed Apr 11, 2024

View reviewed changes

ghalliday requested a review from jakesmith April 11, 2024 11:46

jakesmith reviewed Apr 15, 2024

View reviewed changes

ghalliday requested review from ghalliday and jakesmith May 8, 2024 10:34

jakesmith approved these changes May 9, 2024

View reviewed changes

ghalliday reviewed May 10, 2024

View reviewed changes

HPCC-31511 Avoid keeping the workunit graphs in memory

16bc421

Signed-off-by: Richard Chapman <[email protected]>

richardkchapman force-pushed the HPCC-31511 branch from a19aeae to 16bc421 Compare May 15, 2024 09:05

ghalliday approved these changes May 17, 2024

View reviewed changes

ghalliday merged commit bbd581d into hpcc-systems:candidate-9.4.x May 17, 2024
50 of 51 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HPCC-31511 Avoid keeping the workunit graphs in memory #18519

HPCC-31511 Avoid keeping the workunit graphs in memory #18519

richardkchapman commented Apr 10, 2024

github-actions bot commented Apr 10, 2024

ghalliday Apr 11, 2024

richardkchapman May 1, 2024

ghalliday Apr 11, 2024

richardkchapman May 1, 2024

ghalliday May 10, 2024

ghalliday Apr 11, 2024

jakesmith Apr 11, 2024

jakesmith Apr 15, 2024

jakesmith Apr 15, 2024

richardkchapman May 1, 2024

jakesmith left a comment

jakesmith Apr 15, 2024

jakesmith Apr 15, 2024

jakesmith left a comment

ghalliday left a comment

ghalliday May 10, 2024

ghalliday May 10, 2024

richardkchapman commented May 15, 2024

ghalliday commented May 17, 2024

HPCC-31511 Avoid keeping the workunit graphs in memory #18519

HPCC-31511 Avoid keeping the workunit graphs in memory #18519

Conversation

richardkchapman commented Apr 10, 2024

Type of change:

Checklist:

Smoketest:

Testing:

github-actions bot commented Apr 10, 2024

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

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

richardkchapman commented May 15, 2024

ghalliday commented May 17, 2024