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

Don't buffer all the data in memory when writing, reducing peak memory usage #256

Merged
merged 5 commits into from
Oct 27, 2021
Merged

Don't buffer all the data in memory when writing, reducing peak memory usage #256

merged 5 commits into from
Oct 27, 2021

Conversation

itamarst
Copy link
Contributor

Fixes #145.

Before:

Git master as of 2021-10-14

After:

Code in PR

In both cases running the script from #145 (comment) using fil-profile run example.py.

@itamarst
Copy link
Contributor Author

My one concern here is that Windows semantics might be a problem. I will submit a separate PR to switch CI to using GitHub Actions, then merge this forward.

@itamarst
Copy link
Contributor Author

I ran tests using the GitHub Actions setup in #257. It works fine on Windows, but fails on Python 2.7 where mmap objects don't support the buffer API.

There are two solutions, I am happy to implement both:

  1. Drop Python 2.7, my recommended solution. This mostly just requires adding the correct metadata to setup.py so people installing on Python 2 don't get the Python 3-only version.
  2. Add fallback path to this PR for Python 2.

Please let me know which you prefer and I will submit a PR or update a PR (but I recommend merging #257 first).

@itamarst
Copy link
Contributor Author

@ionrock OK this is now ready for review.

@ionrock ionrock merged commit ae90b58 into psf:master Oct 27, 2021
@@ -62,8 +82,8 @@ def _close(self):
# and allows the garbage collector to do it's thing normally.
self.__callback = None

# Closing the BytesIO stream releases memory. Important when caching
# big files.
# Closing the tempoary file releases memory and frees disk space.
Copy link

Choose a reason for hiding this comment

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

typo: s/tempoary/temporary/

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Large responses cause increased memory usage.
3 participants