add .sync() to flush writes to disk #14
Conversation
WalkthroughThis update introduces explicit synchronization and flushing capabilities for memory-mapped files on both Linux and Windows platforms. New Changes
Sequence Diagram(s)Memory-Mapped File Synchronization (Generic Flow)sequenceDiagram
participant User
participant MappedFile
participant OS
User->>MappedFile: sync(offset, size)
MappedFile->>OS: msync/FlushViewOfFile(offset, size)
OS-->>MappedFile: Success/Error
MappedFile-->>User: Return/Throw
Resizable Mapped File SynchronizationsequenceDiagram
participant User
participant ResizableMappedFile
participant UnderlyingMappedFile
participant OS
User->>ResizableMappedFile: sync(offset, size)
ResizableMappedFile->>UnderlyingMappedFile: sync(offset, size)
UnderlyingMappedFile->>OS: msync/FlushViewOfFile(offset, size)
OS-->>UnderlyingMappedFile: Success/Error
UnderlyingMappedFile-->>ResizableMappedFile: Return/Throw
ResizableMappedFile-->>User: Return/Throw
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (6)
🧰 Additional context used🧬 Code Graph Analysis (3)include/decodeless/detail/mappedfile_linux.hpp (1)
include/decodeless/detail/mappedfile_windows.hpp (1)
include/decodeless/mappedfile.hpp (1)
🪛 LanguageToolREADME.md[grammar] ~5-~5: Use correct spacing (QB_NEW_EN_OTHER_ERROR_IDS_5) [grammar] ~7-~7: Use hyphens correctly (QB_NEW_EN_OTHER_ERROR_IDS_29) [grammar] ~64-~64: There might be a mistake here. (QB_NEW_EN_OTHER) 🔇 Additional comments (20)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Was outright missing write permissions on windows and private/non-shared on linux.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
CMakeLists.txt(1 hunks)README.md(3 hunks)include/decodeless/detail/mappedfile_linux.hpp(4 hunks)include/decodeless/detail/mappedfile_windows.hpp(18 hunks)include/decodeless/mappedfile.hpp(4 hunks)test/src/mappedfile.cpp(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
README.md (1)
include/decodeless/detail/mappedfile_common.hpp (1)
decodeless(9-24)
include/decodeless/detail/mappedfile_linux.hpp (1)
include/decodeless/detail/mappedfile_windows.hpp (23)
offset(167-171)offset(167-167)offset(196-202)offset(196-197)offset(565-570)offset(565-565)offset(571-582)offset(571-572)offset(665-671)offset(665-665)size(643-658)size(643-643)size(694-711)size(694-694)m_size(189-189)m_size(487-487)m_size(691-691)m_size(692-692)m_address(161-161)m_address(434-434)m_address(444-450)m_address(564-564)LastError(48-49)
include/decodeless/mappedfile.hpp (1)
include/decodeless/detail/mappedfile_common.hpp (1)
decodeless(9-24)
include/decodeless/detail/mappedfile_windows.hpp (1)
include/decodeless/detail/mappedfile_linux.hpp (41)
LastError(30-31)m_address(127-127)m_address(165-182)offset(128-130)offset(128-128)offset(132-142)offset(132-133)offset(208-212)offset(208-209)offset(248-252)offset(248-248)static_cast(75-75)m_size(131-131)m_size(291-291)m_size(292-292)other(56-56)other(56-56)other(57-63)other(57-57)other(113-113)other(113-113)other(114-122)other(114-114)other(232-232)other(232-232)other(255-260)other(255-255)other(290-290)other(290-290)other(327-327)other(327-327)size(76-79)size(76-76)size(153-162)size(153-153)size(236-242)size(236-236)size(263-267)size(263-263)size(294-325)size(294-294)
🪛 LanguageTool
README.md
[grammar] ~5-~5: Use correct spacing
Context: ... Components can be used individually or combined. decodeless_mappedfile is a small cross platform file mapping ...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~7-~7: Use hyphens correctly
Context: ...ed. decodeless_mappedfile is a small cross platform file mapping abstraction. It pr...
(QB_NEW_EN_OTHER_ERROR_IDS_29)
[grammar] ~10-~10: Use articles correctly
Context: ... and growing a file mapping into it for really convenient way to write binary files. ...
(QB_NEW_EN_OTHER_ERROR_IDS_11)
[grammar] ~64-~64: There might be a mistake here.
Context: ...library with CMake integration. Use any of: - add_subdirectory(path/to/mappedfile) - include(FetchContent) FetchContent_Declare( decodeless_mappedfile GIT_REPOSITORY https://github.com/decodeless/mappedfile.git GIT_TAG release_tag GIT_SHALLOW TRUE ) FetchContent_MakeAvailable(decodeless_mappedfile) - find_package(decodeless_mappedfile REQUIRED CONFIG PATHS ...) Then, ``` target_li...
(QB_NEW_EN_OTHER)
🪛 markdownlint-cli2 (0.17.2)
README.md
66-66: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
70-70: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
81-81: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
87-87: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build (windows-latest, Release, cl)
🔇 Additional comments (13)
CMakeLists.txt (1)
1-1: Copyright-year refresh acknowledgedHeader update is accurate and introduces no functional impact.
README.md (1)
46-46: Good example size update.The increase from 4096 bytes to 1 GiB better demonstrates the capability of reserving large virtual address spaces without immediate physical memory allocation.
test/src/mappedfile.cpp (2)
29-29: Good use of static_cast over reinterpret_cast.Using
static_castfor void* conversions is safer and more appropriate thanreinterpret_cast.
32-58: Well-structured tests for writable mappings and sync functionality.The new tests appropriately cover the writable mapping and sync features. The comments acknowledging the limitations of testing sync operations (since reading from the same process accesses cached pages) show good awareness of the testing constraints.
Also applies to: 437-457
include/decodeless/mappedfile.hpp (2)
42-48: Good addition of sync requirements to writable_mapped_file concept.The addition of both sync() overloads ensures a consistent interface across platforms for flushing writable mapped files to disk.
58-66: Well-designed resizable_mapped_file concept.The new concept appropriately combines memory resizing capabilities with synchronization requirements, ensuring resizable files can be explicitly flushed to disk.
include/decodeless/detail/mappedfile_linux.hpp (3)
132-142: Correct implementation of range-based sync.The method properly handles page alignment requirements for msync and includes appropriate bounds checking with the assert.
197-199: Good simplification of mapping flags.Removing the mapFlags parameter and automatically selecting MAP_SHARED for writable and MAP_PRIVATE for read-only mappings simplifies the API while maintaining correct behavior.
243-252: Appropriate sync implementation for ResizableMappedFile.The conditional sync calls correctly handle the case where the file hasn't been mapped yet (size is 0).
include/decodeless/detail/mappedfile_windows.hpp (4)
107-110: Correct implementation of file buffer flushing.Using
FlushFileBuffersis the appropriate Windows API for ensuring file metadata and buffers are written to disk.
167-171: Proper implementation of view flushing.The method correctly uses
FlushViewOfFilewith appropriate pointer arithmetic for the offset.
186-202: Well-designed sync implementation for mapped files.The simplified protection flags align with the Linux implementation. The sync methods correctly flush the view first (asynchronously) then the file handle (synchronously with metadata), ensuring complete persistence.
659-671: Consistent sync implementation for ResizableMappedFile.The sync methods appropriately handle the optional view and follow the same pattern as MappedFile (flush view, then file metadata).
add writable_file::sync() and resizable_file::sync() with optional offset and size arguments drive-by fix for likely outdated NtQuerySection definition removed unused ntifs() dll module singleton renamed SectionPageProtection that used to shadow a createSection() parameter
Add writable_file::sync() and resizable_file::sync() with optional
offset and size arguments
Drive-by fix for likely outdated NtQuerySection definition
Removed unused ntifs() dll module singleton
Renamed SectionPageProtection that used to shadow a createSection()
parameter
Fix writable_file mapping permissions. Was outright missing write permissions on windows and private/non-shared
on linux.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
Chores