Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve std::thread cleanup #1880

Merged
merged 6 commits into from
Jun 27, 2022
Merged

Conversation

olupton
Copy link
Collaborator

@olupton olupton commented Jun 24, 2022

  • NEURON builds using NVHPC were failing after Replace pthreads with std::thread and friends #1859 because worker threads were not always joined before shutdown.
    • It seems that NEURON does not deallocate its data structures at shutdown.
    • Unclear why this problem was specific to NVHPC
  • See Print memory footprint of each mechanism BlueBrain/CoreNeuron#833 (comment) for an example of a failing run.
  • Reorganise code in multicore.cpp to join threads at shutdown before the std::thread objects are destroyed.
  • Drop unused code with BENCH* macros
  • Drop unused allocation function wrappers. std::malloc is specified to be thread-safe.
  • The busywait branches have not been extensively tested, perhaps they can be dropped too?

@olupton
Copy link
Collaborator Author

olupton commented Jun 24, 2022

https://bbpgitlab.epfl.ch/hpc/coreneuron/-/pipelines/62004 is testing this with the Intel and NVIDIA compilers.

@codecov-commenter
Copy link

codecov-commenter commented Jun 24, 2022

Codecov Report

Merging #1880 (b50be5e) into master (68923c0) will decrease coverage by 0.00%.
The diff coverage is 84.40%.

@@            Coverage Diff             @@
##           master    #1880      +/-   ##
==========================================
- Coverage   47.13%   47.13%   -0.01%     
==========================================
  Files         543      543              
  Lines      112917   112900      -17     
==========================================
- Hits        53224    53213      -11     
+ Misses      59693    59687       -6     
Impacted Files Coverage Δ
src/scopmath/sparse_thread.c 95.93% <ø> (-0.07%) ⬇️
test/unit_tests/unit_test.cpp 100.00% <ø> (ø)
src/nrnoc/multicore.cpp 88.81% <83.96%> (+0.48%) ⬆️
src/nrnoc/init.cpp 87.99% <100.00%> (ø)
src/parallel/ocbbs.cpp 65.21% <100.00%> (+0.08%) ⬆️
share/lib/python/neuron/rxd/species.py 78.85% <0.00%> (+0.07%) ⬆️

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@azure-pipelines
Copy link

✔️ 9ca53e6 -> Azure artifacts URL

@olupton olupton requested a review from alkino June 24, 2022 13:41
@olupton olupton marked this pull request as ready for review June 24, 2022 14:14
@olupton
Copy link
Collaborator Author

olupton commented Jun 24, 2022

https://bbpgitlab.epfl.ch/hpc/coreneuron/-/pipelines/62004 is testing this with the Intel and NVIDIA compilers.

This looks fine.

@olupton
Copy link
Collaborator Author

olupton commented Jun 24, 2022

✔️ 9ca53e6 -> Azure artifacts URL

Launched https://github.com/neuronsimulator/nrn-modeldb-ci/actions/runs/2556232205 comparing this PR to neuron-nightly

@olupton olupton requested a review from nrnhines June 24, 2022 14:16
@olupton olupton requested a review from pramodk June 24, 2022 14:48
@azure-pipelines
Copy link

✔️ b50be5e -> Azure artifacts URL

Copy link
Member

@pramodk pramodk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@olupton olupton merged commit 914ad0a into master Jun 27, 2022
@olupton olupton deleted the olupton/more-thread-reorganisation branch June 27, 2022 08:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants