Repository navigation
Avoid extra copies when pickling, and serialize DensityMap data in one block - #1127
Merged
Merged
Conversation
_get_as_binary() serialized into an ostringstream, copied that into a std::string and then into a Python bytes object, and _set_from_binary() copied the bytes into a std::string and then into an istringstream. Copy straight from the stream buffer into the bytes object, and read directly from the bytes object instead. Relates salilab#1080.
Write and read the voxel data with one call rather than one per voxel. The serialized bytes are unchanged. Relates salilab#1080.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #1127 +/- ##
===========================================
- Coverage 67.72% 67.72% -0.01%
===========================================
Files 1811 1811
Lines 154809 154809
Branches 18519 18520 +1
===========================================
- Hits 104852 104850 -2
- Misses 45245 45249 +4
+ Partials 4712 4710 -2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Relates #1080.
Two small changes that make pickling use less memory, and make it faster for density maps, without changing the pickle format:
_get_as_binary()serialized into anostringstream, then copied the data into astd::stringand again into abytesobject;_set_from_binary()copied thebytesinto astd::stringand again into anistringstream. The data are now copied once, from the stream buffer into thebytesobject, and read directly from thebytesobject (two small helper classes inswig_helpers_base.h).DensityMap::serialize()wrote and read the voxel data one value at a time; it now does so with a singlecereal::binary_datacall.Pickling a 256³
IMP.em.DensityMap(134 MB) and anIMP.Modelwith 10⁶ XYZR particles (41 MB); Release build, Python 3.11, median of 3; temporary memory as a multiple of the pickle size:dumpsloadsdumpsloadsPickling 100,000
Vector3Ds is 5-8% faster. Pickles are byte-for-byte the same as before (checked forModel, a restraint,VersionInfo,Vector3DandDensityMap), so old pickles load with this PR and vice versa.Protocol 5 out-of-band buffers (PEP 574), also suggested in #1080, are left for a follow-up. A prototype that wrapped every object's state in a
PickleBufferfrom a Python__reduce_ex__made pickling 100,000Vector3Ds with protocol 5 slower (0.16 s to 0.26 s), so out-of-band buffers should probably be used only for large objects; and reading them without a copy needsPy_buffer, which is only in the limited API from Python 3.11 (the Windows installer is built against the 3.10 limited API).Existing tests cover the changed code (e.g. the pickle tests in
modules/em/test/test_density_map.pyandmodules/kernel/test/test_version_info.py). The CI jobs pass locally (Ubuntu 24.04 Release and Debug, Ubuntu 26.04 Fast, clang standing in for macOS), as does a Fast build withIMP_PY_LIMITED_API=3.10, as used for the Windows installer.🤖 Generated with Claude Code