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

Only assign memmap within boundaries for write_binary #2796

Merged
merged 6 commits into from May 21, 2024

Conversation

h-mayorquin
Copy link
Collaborator

This is like #1781 but for writing. I am writing the tutorial for how to use the job_kwargs and the memory heap reports crazy allocations for the following code:

from pathlib import Path
from spikeinterface.core import write_binary_recording
from spikeinterface.core import generate_recording



minutes = 30
recording = generate_recording(num_channels=384, durations=[60.0 * minutes], seed=0)


job_kwargs = dict(n_jobs=1, total_memory="1G", progress_bar=False, verbose=False)
file_path = Path(__file__).parent / "recording_test_n_jobs.dat"
write_binary_recording(recording=recording, file_paths=[file_path], **job_kwargs)

image

Reason being that ta memmap for the whole array (80 GiB) is reserved even if it not used. A possible consequence of this massive heap allocation is that maybe the system will overswap. With the current PR this heap allocations disappears.

image

This will make my life easier for tracking the efficiency of processing and I think is a small increase in complexity.

@h-mayorquin
Copy link
Collaborator Author

Performance:

this branch:

image

The main branch:

image

Changes are within the standard deviation of each other.

@h-mayorquin h-mayorquin marked this pull request as ready for review May 3, 2024 00:14
@h-mayorquin h-mayorquin self-assigned this May 3, 2024
@h-mayorquin h-mayorquin added core Changes to core module performance Performance issues/improvements labels May 3, 2024
@samuelgarcia
Copy link
Member

thanks. This is really cool.

memmap_obj.flush()

memmap_obj.close()
Copy link
Member

Choose a reason for hiding this comment

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

Do we need this ?
the memmap_ojob is deleted no ?
This is a bit confusing because the file is stil open. Are we sure that this line do not close the underlying file ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I am not sure but the file is not closed, otherwise the tests would not work.

@samuelgarcia
Copy link
Member

Merci beaucoup beaucoup!
I ask for a small question.
We have lot of work to do on the neo side as well!!!

@samuelgarcia samuelgarcia merged commit 2325958 into SpikeInterface:main May 21, 2024
11 checks passed
@h-mayorquin h-mayorquin deleted the improve_write_to_binary branch May 21, 2024 13:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Changes to core module performance Performance issues/improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants