Skip to content

Default node order is cell contiguous (except for thread rootnodes).#3300

Merged
nrnhines merged 19 commits into
masterfrom
hines/cell-contiguous
May 30, 2025
Merged

Default node order is cell contiguous (except for thread rootnodes).#3300
nrnhines merged 19 commits into
masterfrom
hines/cell-contiguous

Conversation

@nrnhines

@nrnhines nrnhines commented Jan 6, 2025

Copy link
Copy Markdown
Member

Default cell contiguity on top of #3299 implies that in general, CvMemList.ml.size() == 1 for each cell. The exceptions
are that CvMembList.ml.size() == 2 whenever the extracellular mechanism is present or if a synapse (of the same type) is located both in the cell rootnode and somewhere else on the cell (note that cell rootnodes for a thread are adjacent).
The node ordering of each cell (after the root node) is section depth first.

This ordering fixes most of the performance loss with respect to 8.2. With the intel compiler, performance of the ringtest with
nrniv -python ringtest.py -rparm -nring 32 -npt 1 -compart 1 3 -method 2 -2nd_order_thresh is

version    runtime (s)
8.2        1.4
master     6.7
This PR    2.7

I speculate that the remainder of the performance issue is the use of DataHandle during sim time instead of raw double* and that will be dealt with in subsequent pull requests.

I would like this PR to be reviewed as a change to #3299 but not merged. It can be merged to the master after #3299 is merged into the master.

@nrnhines nrnhines requested a review from JCGoran January 6, 2025 17:55
@nrnhines nrnhines marked this pull request as draft January 6, 2025 17:56
@azure-pipelines

Copy link
Copy Markdown

✔️ 4f6cba4 -> Azure artifacts URL

@sonarqubecloud

sonarqubecloud Bot commented Jan 8, 2025

Copy link
Copy Markdown

Base automatically changed from hines/cvlocal-bug to master May 29, 2025 19:24
@azure-pipelines

Copy link
Copy Markdown

✔️ 67edba4 -> Azure artifacts URL

@codecov

codecov Bot commented May 29, 2025

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 68.34%. Comparing base (228e30c) to head (63b49b9).
⚠️ Report is 68 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3300      +/-   ##
==========================================
- Coverage   68.34%   68.34%   -0.01%     
==========================================
  Files         682      682              
  Lines      116580   116572       -8     
==========================================
- Hits        79680    79672       -8     
  Misses      36900    36900              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@github-actions

Copy link
Copy Markdown
Contributor

✔️ 67edba4 -> artifacts URL

@github-actions

Copy link
Copy Markdown
Contributor

✔️ 4a69c68 -> artifacts URL

@azure-pipelines

Copy link
Copy Markdown

✔️ 4a69c68 -> Azure artifacts URL

@github-actions

Copy link
Copy Markdown
Contributor

✔️ 170d40e -> artifacts URL

@github-actions

Copy link
Copy Markdown
Contributor

✔️ b62acb7 -> artifacts URL

@nrnhines nrnhines marked this pull request as ready for review May 30, 2025 00:17
@azure-pipelines

Copy link
Copy Markdown

✔️ b62acb7 -> Azure artifacts URL

Comment thread src/nrnoc/multicore.cpp Outdated
Comment thread test/external/CMakeLists.txt
@github-actions

Copy link
Copy Markdown
Contributor

✔️ 595a3b4 -> artifacts URL

@azure-pipelines

Copy link
Copy Markdown

✔️ 3d8a428 -> Azure artifacts URL

@nrnhines nrnhines enabled auto-merge (squash) May 30, 2025 14:10
@sonarqubecloud

Copy link
Copy Markdown

@azure-pipelines

Copy link
Copy Markdown

✔️ 63b49b9 -> Azure artifacts URL

@github-actions

Copy link
Copy Markdown
Contributor

✔️ 63b49b9 -> artifacts URL

@nrnhines nrnhines merged commit 07c7565 into master May 30, 2025
43 checks passed
@nrnhines nrnhines deleted the hines/cell-contiguous branch May 30, 2025 16:25
@JCGoran JCGoran mentioned this pull request Jul 7, 2025
29 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants