Reduce MAX_TERMINATED_THREADS default, improve memory use on manycore…

[biddisco]

… cpus

The default value of 1000 is too high and causes problems on many core machines. there does not seem to be any great advantage to having a high number of terminated tasks waiting for cleanup. Stack usage explodes when this number is large.

[hkaiser]

@biddisco What new default value for this configuration constant did you have in mind?

[hkaiser]

What new default value for this configuration constant did you have in mind?

@biddisco Sorry, I should have asked: how did you determine the new value you propose. reducing it down to 25 seems to be a radical change...

[biddisco]

On our 32 core nodes, the previous value of 1000 could leave 32,000 uncleaned up tasks (and their stacks).
Including High priority queues as we do, pushes that up to 64k tasks unclaimed.

Moving to KNL, we will be using upwards of 300K tasks in the cleanup zone, and with large stack sizes, this is a big problem.

All tasks must be cleaned up sooner or later and the previous limit of 1000 was pulled from a hat anyway. We tested with 100 and found memory issues went away, but a value of 25 seems more realistic. I would prefer lower than that since larger values provide no gain other than a small saving of detour into the cleanup code. Smaller values are more likely to allow cache coherent reuse of memory than larger values.

[hkaiser]

I would prefer lower than that since larger values provide no gain other than a small saving of detour into the cleanup code.

Thanks for this explanation of your rationale.

The main reason why this delayed cleanup was put in place to begin with was to avoid acquiring a kernel lock in the scheduler. In the end the selected default value should give a reasonable compromise between having to acquire a kernel lock in the scheduler and the memory overheads eventually imposed by not-cleaned-up threads.

Are we sure that having to acquire a kernel lock every 25 executed tasks wouldn't cause too much overhead? May be we should change the cleanup logic in a way that it would be executed whenever we have to acquire that lock for other reasons anyways?

[sithhell]

Any plans on how to move forward with this?

[biddisco]

• Add a CMake option to set the number, and make the default 100 (say)
  or
• Add a command line option to set it at runtime with a default of 100 (say)

The first is easier, the second gives more flexibility. I will do the first unless several of you prefer the second

[hkaiser]

• Add a CMake option to set the number, and make the default 100 (say)
  or
• Add a command line option to set it at runtime with a default of 100 (say)

The first is easier, the second gives more flexibility. I will do the first unless several of you prefer the second

Why not do the second with a cmake option to set the default?

[hkaiser]

@biddisco Why did you close this? Doesn't this give you any performance benefit anymore?

[biddisco]

This is absolutely critical. Without this patch we cannot run our code. However, I made a reasonable request for a change and it was not accepted, and I must work on other deadlines, so when I get time, I will revisit this, but for now the PR is going nowhere and can be deleted.

[hkaiser]

I'll reopen this and might be able to find some time to work on it.

[hkaiser]

@biddisco: please review

[biddisco]

Nitpicking: I have added one commit because a boolean option like HPX_WITH_CUDA might not be available and so HPX_HAVE_CUDA will not be set. With options that are not optional, such as MAX__TERMINATED_THREADS - there is not need for WITH/HAVE - this breaks convention in naming.
There is a define and it must be set to something, so simply

HPX_SCHEDULER_MAX_TERMINATED_THREADS=blah

is enough

[biddisco]

There were some changes to the STRINGIZE function that caused a conflict when I merged master, so I have rebased onto master and cleaned the problem. Please check that the vars I've used are correct. I might have broken something

[biddisco]

I'm not sure if the final commit was lost yesterday, or maybe I pushed it to the wrong repo. Please check the use of BOOST_STRINGIZE that I have changed to HPX_PP_STRINGIZE which I believe is the correct new usage.

[hkaiser]

@biddisco: BOOST_STRINGIZE(x) must be replaced by HPX_PP_STRINGIZE(HPX_PP_EXPAND(x)) - I thought I made this change, did it get lost as well?

[biddisco]

Fixed.

[biddisco]

question - where does this var come from "HPX_THREAD_QUEUE_MAX_TERMINATED_THREADS"

[hkaiser]

@biddisco: this is used to check the execution environment. The syntax:

${HPX_THREAD_QUEUE_MAX_TERMINATED_THREADS:1000}

means: 'check the env for HPX_THREAD_QUEUE_MAX_TERMINATED_THREADS and use the corresponding value if found, otherwise use 1000 (see here for more details).

[biddisco]

Anyone want to review/merge this. the review button is greyed out for me since I started it.