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

Fix leak in nrn_thread_free. #2541

Merged
merged 4 commits into from
Sep 28, 2023
Merged

Fix leak in nrn_thread_free. #2541

merged 4 commits into from
Sep 28, 2023

Conversation

1uc
Copy link
Collaborator

@1uc 1uc commented Sep 26, 2023

The array of Prop* is allocated unconditionally in thread_memblist_setup.

tml->ml->prop = new Prop*[mlcnt[i]];  // used for ode_map
if (!memb_func[i].hoc_mech) {
    CACHELINE_ALLOC(tml->ml->pdata, Datum*, mlcnt[i]);
}

Hence this commit adjust the logic in nrn_thread_free deallocate ml->prop unconditionally.

@1uc
Copy link
Collaborator Author

1uc commented Sep 26, 2023

I suspect that the allocation happens here:

nrn/src/nrnoc/multicore.cpp

Lines 503 to 508 in 88d7962

CACHELINE_ALLOC(tml->ml->nodelist, Node*, mlcnt[i]);
CACHELINE_ALLOC(tml->ml->nodeindices, int, mlcnt[i]);
tml->ml->prop = new Prop*[mlcnt[i]]; // used for ode_map
if (!memb_func[i].hoc_mech) {
CACHELINE_ALLOC(tml->ml->pdata, Datum*, mlcnt[i]);
}

@bbpbuildbot

This comment has been minimized.

@codecov
Copy link

codecov bot commented Sep 26, 2023

Codecov Report

Merging #2541 (9cc76c1) into master (8985d2b) will decrease coverage by 0.02%.
Report is 1 commits behind head on master.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #2541      +/-   ##
==========================================
- Coverage   61.49%   61.48%   -0.02%     
==========================================
  Files         623      623              
  Lines      119163   119163              
==========================================
- Hits        73281    73265      -16     
- Misses      45882    45898      +16     
Files Coverage Δ
src/nrnoc/multicore.cpp 90.01% <100.00%> (+0.16%) ⬆️

... and 7 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@azure-pipelines
Copy link

✔️ e2de83f -> Azure artifacts URL

@pramodk
Copy link
Member

pramodk commented Sep 27, 2023

@1uc : rebasing this on the master to get the CI part going.

@1uc 1uc marked this pull request as ready for review September 27, 2023 13:03
@bbpbuildbot

This comment has been minimized.

The array of `Prop*` is allocated unconditionally in `thread_memblist_setup`.

    tml->ml->prop = new Prop*[mlcnt[i]];  // used for ode_map
    if (!memb_func[i].hoc_mech) {
	CACHELINE_ALLOC(tml->ml->pdata, Datum*, mlcnt[i]);
    }

Hence this commit adjust the logic in `nrn_thread_free` deallocate `ml->prop`
unconditionally.
@1uc 1uc force-pushed the 1uc/fix-nrn_thread_free branch from 36f59bc to e057dee Compare September 27, 2023 16:30
@azure-pipelines
Copy link

✔️ 36f59bc -> Azure artifacts URL

@bbpbuildbot

This comment has been minimized.

@bbpbuildbot

This comment has been minimized.

@1uc 1uc force-pushed the 1uc/fix-nrn_thread_free branch from e057dee to 9cc76c1 Compare September 28, 2023 08:49
@azure-pipelines
Copy link

✔️ 9cc76c1 -> Azure artifacts URL

@pramodk pramodk enabled auto-merge (squash) September 28, 2023 15:06
@pramodk pramodk disabled auto-merge September 28, 2023 15:10
@pramodk pramodk merged commit 78b0d1b into master Sep 28, 2023
33 checks passed
@pramodk pramodk deleted the 1uc/fix-nrn_thread_free branch September 28, 2023 15:11
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