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

Use memory-mapping with HDF5 files whenever possible #1628

Merged
merged 6 commits into from
Apr 3, 2018

Conversation

astrofrog
Copy link
Member

@astrofrog astrofrog commented Mar 22, 2018

This seems to work. I tested it out with @maartenbreddels's sdss_dereddened.hdf5 file and the scatter density plots worked ok. They were slightly sluggish but not dramatically.

@maartenbreddels - when you get a chance, can you review this? Thanks for the example implementation!

Open questions:

  • Should we only do memory mapping for arrays over a certain size, performance-wise?

  • Does this benefit from any disk I/O caching or does every array access result in an I/O operation?

I also noticed that the auto-merging isn't quite right at the moment, so I'll try and work on that soon, maybe in a separate PR.

@maartenbreddels
Copy link
Contributor

Yes, I'll take a look next week!

@maartenbreddels
Copy link
Contributor

LGTM.
I wouldn't worry about the array size, for small files the performance isn't an issue anyway. The only issue is actually OSX, their mmap support is really bad, as is their diskcaching strategy. Maybe we should have an config option, or keyword argument for when you don't want mmapping (for benchmarking, or when an issue arrises)

@astrofrog
Copy link
Member Author

@maartenbreddels - just to check, what kind of issues do you see on MacOS X typically? Just performance, or actual crashes?

@astrofrog astrofrog merged commit cb92601 into glue-viz:master Apr 3, 2018
@maartenbreddels
Copy link
Contributor

Performance only.

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.

2 participants