Lvardt cell CvMembList.ml.size() should be the number of contiguous regions of the fixed step thread Memb_list #3299
Conversation
|
✔️ d73896e -> Azure artifacts URL |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3299 +/- ##
=======================================
Coverage 68.36% 68.37%
=======================================
Files 682 682
Lines 116555 116574 +19
=======================================
+ Hits 79686 79706 +20
+ Misses 36869 36868 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
From a high level perspective, do you happen to have some performance numbers related to this change, or do we also need #3300 to be able to see a (substantial) difference? Also, are the improvements consistent on both Linux and MacOS?
Regarding the CI failure on MacOS, I don't really see how it's related to this change (something about fmt? But the PR doesn't touch those parts at all), so we can probably ignore it.
Ironically, this is performant equivalent to #3300 for a single cell model. (For a single cell model global vardt and local vardt are conceptually equivalent since in both cases there is only a single instance of the cvode integrator). I say "ironically" because the my first sight of the issue was for the single cell modeldb model 267666 discrepancy between master local and global cv:advance_tn time and instructions retired: The reason this PR is not an endpoint solution to the problem is that default node ordering is globally section depth first which interleaves sections of different cells. Here is a ringtest sim time comparison between master, this pr, and #3300 using the intel compiler I have not checked the mac performance.
I believe that was localized to the new NMODL and I didn't look closely about where the specific failing CI's were getting that version. I do notice that locally my PR and the master are both getting 49fdfe6c from https://github.com/BlueBrain/nmodl |
|
✔️ ac1c6a9 -> Azure artifacts URL |
|
✔️ 8e1e0e5 -> Azure artifacts URL |
|
|
✔️ 32921b2 -> Azure artifacts URL |
|
@JCGoran I'm ready to start pushing through this series of PR's of which this is the first step. At the end of the series the 9.0 performance will equal or exceed 8.2. I want to keep the PR's localized to specific cvode submodules in order to avoid a mega PR that would be difficult to understand as a squashed blob. |
|
✔️ ef7cce5 -> artifacts URL |
|
|
✔️ ef7cce5 -> Azure artifacts URL |
There are serious performance problems with the local variable time step method in comparison with 8.2 This PR is a prerequisite for fixing one of them.
The 9.0 implementation for local variable time step (each cell is integrated by its own cvode instance) demanded that a cell's
CvMembList.ml.size()could be either 1 orMemb_list .nodecount.Thus, if there were multiple cells in a thread, the
ml.size() == nodecount, and eachMemb_listinstance was limited to a single Node*.A subsequent PR (#3300) will default sort the fixed step nodes so that Nodes of each cell will be as contiguous as possible. Presently, the default is that only nodes in a Section are contiguous.
Ringtest performance on Linux comparing 8.2.7, current master, and this series ending with #3314 is