Split nrn_setup.cpp in phase1 / phase2 #283
Conversation
|
Can one of the admins verify this patch? |
pramodk
left a comment
There was a problem hiding this comment.
overall separation looks good. Once you are done with major changes, let me know and I will start reviewing the code details.
|
Please retest |
ohm314
left a comment
There was a problem hiding this comment.
Great work! I think we are definitely on the right track.
pramodk
left a comment
There was a problem hiding this comment.
Wrok in progress, sill reviewing two files
coreneuron/io/phase1.cpp
Outdated
| } | ||
|
|
||
| void Phase1::shift_gids(int imult) { | ||
| int zz = imult * maxgid; // offset for each gid |
There was a problem hiding this comment.
what we are doing is : let say we have a model with XX cells then when we duplicate it by factor if 5 then we make new max did 5 x XX. So simply, new_maxgid would be fine?
|
Yeah, with the suggestions I provided modifications at one place only and did not change all usages. You can go conversation one by one, apply change locally on your computer (instead of github) and then resolve conversation. |
|
I agree with @pramodk 's general point that the small nitpicky changes should be applied. They don't change your overall code structure and are easy to apply (just annoying). This is also true for my small nitpicky changes and my request to add documentation to the various functions you touched. |
pramodk
left a comment
There was a problem hiding this comment.
My major comment is about memory duplication, see comments.
8b31a42 to
556dfb2
Compare
|
Please retest |
|
@alkino : I see that GPU build causing all failures. Without PGI modules we can’t build that. Can you disable GPU builds for time being? |
|
Please retest |
0c29771 to
5bd8c5c
Compare
|
Please retest |
1 similar comment
|
Please retest |
- this was resulting in segfault in the GFluct3 test
- if mechanism / mod file is different, we can't read data
because sizes between neuron and coreneuron will be different.
This will result into segfault / memory error.
- check mechanism compatibility right after mechanism information
is available
- artificial cells do not have nodeindices
…cpp) (BlueBrain/CoreNeuron#283) Major refactoring of monolithic nrn_setup.cpp into two separate phases that allows to re-use the code for file based transfer as well as memory transfer: * Rewrite phase1, phase2 and move into separate .cpp files * Remove global static user variables * Add OMP_Mutex wrapper for handling OpenMP mutex * VecPlay now use std::vector instead of raw pointers - Give ownership of y_, t_ to VecPlayContinuous * Remove lock / unlock from fixed_vector as it was not used * Remove things related to endianess : code as well as tests * Use name of phase instead of numbers with template arguments * Check mechanism compatibility before reading data - if mechanism / mod file is different, we can't read data because sizes between neuron and coreneuron will be different. This will result into segfault / memory error. - check mechanism compatibility right after mechanism information is available Co-authored-by: Omar Awile <[email protected]> Co-authored-by: pramodk <[email protected]> Thanks @alkino for major work! CoreNEURON Repo SHA: BlueBrain/CoreNeuron@f0acbda
…euron#502) - acc_memcpy_to_device is used to copy the device pointer - so we have to use an address of the device pointer and size of pointer ie. `sizeof(IvocVect*)` - note that IvocVect is already copied by previous acc_copyin call - this issue was introduced in BlueBrain/CoreNeuron#283 fixes BlueBrain/CoreNeuron#501 CoreNEURON Repo SHA: BlueBrain/CoreNeuron@b8d9fcd
IvocVect*toIvocVecttype fory_andt_variables.as directory name #306 Bugfix : add missing link to libscopmath #307testcorenrn readme updated with script and manually ran all tests on GPU with file transfer mode without any error,
This is because the bug was introduced in
direct memory modeand tests inside Jenkins CI are executed in file transfer mode. As part of Run CoreNEURON MOD file functional tests in online mode #310, we should make sure to run tests in online/offline mode.Before this PR in master we get:
While running online mode we get:
The GDB stack trace shows:
And looking at backtrace we get:
fixed in b2c4400 : artificial cells don't have nodeindices.
This models works in file transfer mode but with in-memory transfer we get:
false alarm ! - also fixed by b2c4400!
Currently we are getting:
Issue was that mod files had
bbcore_writefunction body commented and hence bbcore iarray / darray were not correct : https://github.com/pramodk/modeldb-bulb3d-sim/pull/8/files~@alkino : intermediate vector shows
15% increase in the memory usage with the intermediate copies (?). Those vectors get removed after setup phase but we will need a strategy to reduce this. I will create separate ticket and not block this PR.I WAS WRONG! I was comparing file based transfer with master vs. in-memory transfer with this PR. Obviously in-memory transfer has more overhead. Apples-to-apples comparison (after disabling reports because it was disabled in later case), with file transfer mode I get:
So almost similar memory usage. All good!