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

Efficient spike/spike-like event recording #372

Merged
merged 40 commits into from
Oct 20, 2020
Merged

Conversation

neworderofjamie
Copy link
Contributor

The standard GeNN idiom for recording spikes was to use synchronous cudaMemcpy to read the number of spikes for each population in the current timestep, then use another synchronous cudaMemcpy to read that many spikes and then copy the host data into a host-side data structure. Especially with simulation with a 0.1ms timestep and lots of populations, this pretty much prevents any chance of real-time. This PR introduces very simple system which lets you allocate any remaining GPU memory for spike recording meaning that, in many simulations, you can run for a large number of timesteps without any device->host memory transfers.

  1. Mark a neuron population for spike recording in C++:

    e->setSpikeRecordingEnabled(true);

    or Python:

    excitatory_pop.spike_recording_enabled = True
  2. Allocate a number of timesteps of spike recording buffer in C++:

    allocateRecordingBuffers(10000);

    or Python:

    model.load(num_recording_timesteps=10000)
  3. Pull the spike recording buffers in C++:

    pullRecordingBuffersFromDevice();

    or Python:

    model.pull_recording_buffers_from_device()
  4. Access recording data in C++ (where this helper function is in userprojects - you can do whatever you want with the data):

    writeTextSpikeRecording("spikes.csv", recordSpkE,
                                            Parameters::numOutput, 10000, 0.1,
                                            ",", true);

    or Python:

    spike_times, spike_ids = excitatory_pop.spike_recording_data

I think recording spikes is the most common and most inefficient case but this could easily be extended in future to record subsets of neurons/state variables..

From the upcoming PyGeNN paper, this disproportionately helps Python and slow CPUs where the copying of data and sticking it in a host-side data structure component is particularly costly:
microcircuit_overheads

* Allocate and zero shared memory for block's spikes
* Atomic or shared memory if spike is emitted
# Conflicts:
#	generate_swig_interfaces.py
#	include/genn/backends/cuda/backend.h
#	src/genn/backends/cuda/backend.cc
@neworderofjamie neworderofjamie added this to the GeNN 4.4.0 milestone Oct 12, 2020
@codecov
Copy link

codecov bot commented Oct 12, 2020

Codecov Report

Merging #372 into master will increase coverage by 0.20%.
The diff coverage is 94.76%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #372      +/-   ##
==========================================
+ Coverage   86.25%   86.46%   +0.20%     
==========================================
  Files          70       70              
  Lines       12072    12327     +255     
==========================================
+ Hits        10413    10658     +245     
- Misses       1659     1669      +10     
Impacted Files Coverage Δ
include/genn/genn/modelSpec.h 83.62% <ø> (ø)
include/genn/genn/neuronGroup.h 74.00% <50.00%> (-2.09%) ⬇️
include/genn/genn/code_generator/backendBase.h 90.47% <66.66%> (+0.23%) ⬆️
src/genn/backends/single_threaded_cpu/backend.cc 56.77% <69.23%> (+0.11%) ⬆️
src/genn/genn/neuronGroup.cc 92.92% <76.47%> (+0.23%) ⬆️
src/genn/backends/opencl/backend.cc 91.09% <86.36%> (-0.23%) ⬇️
include/genn/backends/opencl/backend.h 98.79% <100.00%> (+0.01%) ⬆️
...nclude/genn/backends/single_threaded_cpu/backend.h 76.00% <100.00%> (ø)
include/genn/genn/code_generator/backendSIMT.h 98.30% <100.00%> (ø)
include/genn/genn/code_generator/modelSpecMerged.h 98.56% <100.00%> (ø)
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cc7e03e...afcbda5. Read the comment docs.

@jamesturner246
Copy link
Member

I'm getting

In file included from /its/home/jt273/workspace/genn-recording/pygenn/genn_wrapper/include/../../../userproject/include/sharedLibraryModel.h:24:0,
                 from /its/home/jt273/workspace/genn-recording/pygenn/genn_wrapper/include/sharedLibraryModelNumpy.h:4,
                 from pygenn/genn_wrapper/generated/SharedLibraryModelNumpy_wrap.cpp:3132:
/its/home/jt273/workspace/genn-recording/pygenn/genn_wrapper/include/../../../userproject/include/spikeRecorder.h: In function ‘void writeTextSpikeRecording(const string&, const uint32_t*, unsigned int, unsigned int, double, const string&, bool, bool)’:
/its/home/jt273/workspace/genn-recording/pygenn/genn_wrapper/include/../../../userproject/include/spikeRecorder.h:223:43: error: invalid conversion from ‘int’ to ‘std::ios_base::openmode {aka std::_Ios_Openmode}’ [-fpermissive]
     std::ofstream stream(filename, append ? std::ofstream::app : 0);
                                    ~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~
In file included from /its/home/jt273/workspace/genn-recording/pygenn/genn_wrapper/include/../../../userproject/include/spikeRecorder.h:5:0,
                 from /its/home/jt273/workspace/genn-recording/pygenn/genn_wrapper/include/../../../userproject/include/sharedLibraryModel.h:24,
                 from /its/home/jt273/workspace/genn-recording/pygenn/genn_wrapper/include/sharedLibraryModelNumpy.h:4,
                 from pygenn/genn_wrapper/generated/SharedLibraryModelNumpy_wrap.cpp:3132:
/usr/include/c++/7/fstream:715:7: note:   initializing argument 2 of ‘std::basic_ofstream<_CharT, _Traits>::basic_ofstream(const string&, std::ios_base::openmode) [with _CharT = char; _Traits = std::char_traits<char>; std::__cxx11::string = std::__cxx11::basic_string<char>; std::ios_base::openmode = std::_Ios_Openmode]’
       basic_ofstream(const std::string& __s,
       ^~~~~~~~~~~~~~

when compiling PyGeNN. Isn't openmode a c-string?

@neworderofjamie
Copy link
Contributor Author

Well that last minute addition was clearly not great 😄 can you get latest and try?

Copy link
Member

@jamesturner246 jamesturner246 left a comment

Choose a reason for hiding this comment

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

Okay, it compiles and the results look sensible. Thought they looked short, but I wasn't setting save steps high enough.

API is fine, but maybe convenient for lazy people to set layer recording in the neuron population constructor? Also see the per-neuron push/pull recording buffer note.

I see you plan to do it for neuron state vars (and synapse state?) as well, which could be handy.

@@ -278,6 +300,14 @@ def delay_slots(self):
def size(self):
return self.pop.get_num_neurons()

Copy link
Member

Choose a reason for hiding this comment

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

Would be cool to have a per-layer pull_recording_buffers_from_device method, so you can selectively pull recording buffers without pulling anything else.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What would be the use case for doing that? If you've enabled recording on a neuron population wouldn't you always want the data at the end of the recording period?

Copy link
Member

Choose a reason for hiding this comment

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

Good point, I suppose you would.

@neworderofjamie
Copy link
Contributor Author

Turning over the metaphorical rock by adding tests uncovered a number of loose ends/bugs (including what appears to be a bug in NVIDIA's OpenCL implementation which needs further investigation). These are all now addressed and this should be good to go.

Copy link
Member

@tnowotny tnowotny left a comment

Choose a reason for hiding this comment

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

Did I miss it or should we add a few words about this to the manual? - otherwise approved.

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.

None yet

3 participants