HPCC-30007 Introduce CPooledPersistent and "pooled" sink mode in roxie by ghalliday · Pull Request #17636 · hpcc-systems/HPCC-Platform

ghalliday · 2023-08-01T18:27:06Z

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 · 2023-08-01T18:27:31Z

https://track.hpccsystems.com/browse/HPCC-30007
Jira updated

ghalliday · 2023-08-01T18:27:49Z

This isn't ready to merge, but I think it would be worthwhile discussing the advantages/disadvantages before cleaning it up.

richardkchapman

Initial comments

richardkchapman · 2023-08-02T08:56:12Z

    CNodeMapping * next = nullptr;
 };

-typedef OwningSimpleHashTableOf<CNodeMapping, CKeyIdAndPos> CNodeTable;


Seems unrelated

richardkchapman · 2023-08-02T08:57:14Z


 //=================================================================================

+#define NEW_RESTARTABLE_THREAD


Should this be testing NEW_THREADEDPERSISTENT?

I assume it's in here so that we implement IThreaded and can thus make use of the noew global thread pool code...

richardkchapman · 2023-08-02T08:57:27Z

        else if (!probeManager && !graphDefinition.isSequential() && sinkMode != SinkMode::Sequential)
        {
-            if (sinkMode == SinkMode::ParallelPersistent || sinkMode == SinkMode::AutomaticPersistent)
+            if (sinkMode == SinkMode::ParallelPersistent || sinkMode == SinkMode::AutomaticPersistent || true)


Should be || NEW_THREADEDPERSISTENT ?

(or words to that effect)

richardkchapman · 2023-08-02T08:58:51Z


 // CThreadedPersistent
+#ifdef NEW_THREADEDPERSISTENT
+class GlobalThreadPool : public CInterfaceOf<IThreadFactory>


The JIRA mentioned that the global thread pool was to gradually shrink over time if threads went unused. I don't see any code here that would do that.

jakesmith

@ghalliday - a few comments.

jakesmith · 2023-08-02T15:00:21Z


+#define NEW_RESTARTABLE_THREAD
+#ifdef NEW_RESTARTABLE_THREAD
+class RestartableThread : public CInterface, public IThreaded


CInterfaceOf to avoid a vmt ?

jakesmith · 2023-08-02T15:49:43Z

+    GlobalThreadPool()
+    {
+        IExceptionHandler *exceptionHandler = nullptr;
+        unsigned defaultmax = 0;


so currently pool is unbound

jakesmith · 2023-08-02T15:52:09Z

+        unsigned delay = 0;
+        unsigned stacksize = 0; // i.e. default;
+        unsigned timeoutOnRelease = 0;
+        unsigned targetpoolsize = 0;


and targetpoolsize will default to defaultmax == 0, meaning it will not scale down

For the replacement of existing CThreadedPersistent semantics, where there is no upper limit to # of threads,
should defaultmax be left as 0, but targetpoolsize be set to something highish but low enough to reduce threads from peak ?

jakesmith · 2023-08-02T15:59:44Z

+
+void CThreadedPersistent::start()
+{
+    //MORE: Pass this instead of owner and catch exceptions in this class for later reporting?


it's unfortunate that the IExceptionHandler doesn't pass the handle back..
Or, the threadpool::join method should throw the exception if there is no handler..

jakesmith · 2023-08-02T16:03:44Z

+{
+    if (!joined)
+    {
+        globalThreadPool.join(handle, timeout);


I think the pool impl. of join should be changed to throw if there is no IExceptionHandler.

mckellyln

I think roxie sink can create 5000+ threads in a very short time, we should have idle threads go away after some amount of time.

ghalliday · 2025-11-12T17:25:28Z

@jakesmith I am not convinced that this really helps - I tried running thor using it and the timings were insignificant.
I might try running the performance suite tests that are designed to cause problems.
However, as it stands I don't think it will change any existing behaviour, and it would allow us to test roxie using the different modes.

Copilot

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

jakesmith

@ghalliday - not sure what the state of this PR is.
There are some v. old comments of mine/others.

It will not be used by default, correct?
Can it be rebased onto master ?

ghalliday · 2026-01-06T14:20:27Z

@jakesmith I think this is ok to merge - it should not have any effect unless the "pooled" sink mode is used.

Signed-off-by: Gavin Halliday <gavin.halliday@lexisnexis.com>

jakesmith

@ghalliday - looks good afaics.

ghalliday requested review from jakesmith, mckellyln and richardkchapman August 1, 2023 18:27

richardkchapman reviewed Aug 2, 2023

View reviewed changes

jakesmith reviewed Aug 2, 2023

View reviewed changes

mckellyln reviewed Aug 7, 2023

View reviewed changes

ghalliday force-pushed the issue30007 branch from d79263b to 5ce5510 Compare October 23, 2025 10:47

ghalliday changed the base branch from candidate-9.2.x to candidate-2.0.x October 23, 2025 10:48

ghalliday force-pushed the candidate-2.0.x branch from 8fb49b9 to 682b952 Compare October 25, 2025 16:07

ghalliday force-pushed the issue30007 branch 2 times, most recently from c9ee4bd to 94e9d96 Compare October 29, 2025 09:27

ghalliday force-pushed the candidate-2.0.x branch from 889b069 to 070106c Compare October 30, 2025 15:35

ghalliday force-pushed the issue30007 branch from 94e9d96 to 431da9a Compare November 12, 2025 17:22

ghalliday requested a review from jakesmith November 12, 2025 17:24

ghalliday marked this pull request as ready for review November 12, 2025 17:24

Copilot AI review requested due to automatic review settings November 12, 2025 17:24

Copilot AI reviewed Nov 12, 2025

View reviewed changes

Copilot started reviewing on behalf of ghalliday November 12, 2025 18:05 View session

Copilot finished reviewing on behalf of ghalliday November 12, 2025 18:09

ghalliday force-pushed the candidate-2.0.x branch from 7da64d0 to ae0c55d Compare November 14, 2025 10:53

jakesmith reviewed Nov 19, 2025

View reviewed changes

ghalliday force-pushed the candidate-2.0.x branch 2 times, most recently from cb6d603 to 4462083 Compare December 5, 2025 10:49

ghalliday force-pushed the candidate-2.0.x branch 2 times, most recently from 9acfb9e to cb2622f Compare December 12, 2025 14:04

ghalliday force-pushed the candidate-2.0.x branch from cb2622f to c048e24 Compare December 19, 2025 12:33

ghalliday force-pushed the candidate-2.0.x branch from c048e24 to a984695 Compare January 5, 2026 20:56

ghalliday force-pushed the candidate-2.0.x branch from a984695 to e981448 Compare January 6, 2026 09:29

ghalliday force-pushed the issue30007 branch 3 times, most recently from ffa04c4 to 7f5aa3f Compare January 6, 2026 14:16

ghalliday changed the title ~~HPCC-30007 Use a global thread pool for CThreadedPersistent~~ HPCC-30007 Introduce CPooledPersistent and "pooled" sink mode in roxie Jan 6, 2026

ghalliday requested a review from jakesmith January 6, 2026 14:19

ghalliday force-pushed the candidate-2.0.x branch from e981448 to d0de6b5 Compare January 6, 2026 15:33

HPCC-30007 Introduce CPooledPersistent and "pooled" sink mode in roxie

05b78b5

Signed-off-by: Gavin Halliday <gavin.halliday@lexisnexis.com>

ghalliday force-pushed the issue30007 branch from 7f5aa3f to 05b78b5 Compare January 6, 2026 17:04

jakesmith approved these changes Jan 6, 2026

View reviewed changes

ghalliday merged commit bbc7b68 into hpcc-systems:candidate-2.0.x Jan 9, 2026
49 of 52 checks passed


		//=================================================================================

		#define NEW_RESTARTABLE_THREAD

Conversation

ghalliday commented Aug 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Type of change:

Checklist:

Smoketest:

Testing:

Uh oh!

github-actions Bot commented Aug 1, 2023

Uh oh!

ghalliday commented Aug 1, 2023

Uh oh!

richardkchapman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jakesmith left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mckellyln left a comment

Choose a reason for hiding this comment

Uh oh!

ghalliday commented Nov 12, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

jakesmith left a comment

Choose a reason for hiding this comment

Uh oh!

ghalliday commented Jan 6, 2026

Uh oh!

jakesmith left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ghalliday commented Aug 1, 2023 •

edited

Loading