adding memory mapped file to store_read to improve store read performance #55
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds memory-mapped file support to improve store read performance by using mmap for faster file access when reading records from the store.
- Added memory mapping initialization during store file setup with fallback to regular file I/O
- Modified the read method to use memory-mapped reading when available
- Implemented a new
__read_until_mmapmethod that reads from memory-mapped files in chunks
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| # Use memory mapping for faster reads on large files | ||
| try: | ||
| if os.path.getsize(self.store) > 0: | ||
| self.store_mem = mmap.mmap(self.store_file.fileno(), 0) | ||
| else: | ||
| self.store_mem = None | ||
| except (ValueError, OSError): | ||
| self.store_mem = None | ||
| self.logger.warning("Could not create memory map for store, falling back to file I/O") |
There was a problem hiding this comment.
Missing import statement for mmap and os modules. The code uses mmap.mmap() and os.path.getsize() but these modules are not imported at the top of the file.
| self.logger.warning("Could not create memory map for store, falling back to file I/O") | ||
| logger.info("Store for file init: " + self.store) | ||
|
|
||
| def close(self): |
There was a problem hiding this comment.
The close() method should be updated to properly close the memory-mapped file (self.store_mem) before closing the regular file handle to prevent resource leaks.
| def close(self): | |
| def close(self): | |
| if self.store_mem is not None: | |
| self.store_mem.close() |
| chunk = self.store_mem[cursor:end_pos] | ||
|
|
||
| if len(chunk) == 0: | ||
| return data |
There was a problem hiding this comment.
Potential issue when data is None and an empty chunk is encountered. The method would return None instead of an empty bytes object, which may cause inconsistent behavior compared to the original __read_until() method.
| return data | |
| return data if data is not None else b'' |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting
| # Use memory-mapped file if available for faster reads | ||
| if self.store_mem is not None: | ||
| record = self.__read_until_mmap(position) | ||
| else: |
There was a problem hiding this comment.
When the store file already contains data at initialization, __init__ creates a fixed-size memory map. read then unconditionally routes every request through __read_until_mmap. After any subsequent save, position for the new record is greater than len(self.store_mem), so __read_until_mmap returns None on the first iteration and the caller receives no bytes for the record. In short, any records appended after the memory map is created are unreadable unless the process is restarted or the mapping is resized. Consider remapping when the file grows or falling back to direct file I/O when the requested position exceeds the map length.
Useful? React with 👍 / 👎.
This PR adds memory-mapped file support to improve store read performance by using mmap for faster file access when reading records from the store.
Added memory mapping initialization during store file setup with fallback to regular file I/O
Modified the read method to use memory-mapped reading when available
Implemented a new __read_until_mmap method that reads from memory-mapped files in chunks