More robust memory mapping #370
Conversation
include/pisa/memory_source.hpp
Outdated
| private: | ||
| explicit MemorySource(std::unique_ptr<mio::mmap_source> source) : m_source(std::move(source)) {} | ||
|
|
||
| std::unique_ptr<mio::mmap_source> m_source; |
There was a problem hiding this comment.
Why a pointer? We could use mio::mmap_source.
There was a problem hiding this comment.
I mistakenly thought I couldn't use value but I can, thanks.
There was a problem hiding this comment.
I kept the pointer to separate implementation from the header.
| namespace pisa { | ||
|
|
||
| /// This is an owning memory source for any byte-based structures. | ||
| class MemorySource { |
There was a problem hiding this comment.
I am not against this class. Just for the sake of discussion, what is the additional value when compared to a mio::mmap_source?
There was a problem hiding this comment.
This is a good question. This is a very light wrapper but has the following advantages:
- Initial motivation was to have something less error-prone, in particular, something that gives a clear error when trying to map a non-existent file, but also something that fails quickly if the memory is empty.
- It's implementation agnostic, if we ever decide to use another backend or even provide alternative to loading everything in memory, we can easily do that.
- Now that I made some changes, I actually separated implementation and only a small header from
miois necessary to use it, the rest is in a binary. Same could be done forbinary_collectionlater on.
|
Worth mentioning that this closes #367 right? |
|
As mentioned before, I think there's more to be done, I think index/wand data need to be decoupled from the way we handle memory. It's not as much about the mapper as it is about |
|
@amallia any updates on this? |
I will follow up soon |
|
@amallia still waiting for comments to the latest push. |
Codecov Report
@@ Coverage Diff @@
## master #370 +/- ##
=======================================
Coverage 92.41% 92.41%
=======================================
Files 90 91 +1
Lines 4915 4918 +3
=======================================
+ Hits 4542 4545 +3
Misses 373 373
Continue to review full report at Codecov.
|
|
Sorry. It took a while but I was finally able to check it again and merge it. |
Addresses, in part, #357.
Fixes #367