windows support and mappedfile tests #2
Conversation
WalkthroughThe changes update CMake configuration to fetch a new dependency and update existing ones, and significantly expand Changes
Sequence Diagram(s)sequenceDiagram
participant TestRunner
participant BenchmarkSuite
participant OS_API
participant DecodelessAbstractions
TestRunner->>BenchmarkSuite: Run WriteFile/WriteMemory/MapFile/MapMemory tests
alt On Windows
BenchmarkSuite->>OS_API: Use Windows APIs (CreateFileW, VirtualAlloc, etc.)
else On Linux/macOS
BenchmarkSuite->>OS_API: Use POSIX APIs (open, mmap, etc.)
end
BenchmarkSuite->>DecodelessAbstractions: Optionally use decodeless::memory_writer, resizable_file, etc.
OS_API-->>BenchmarkSuite: Return operation results
DecodelessAbstractions-->>BenchmarkSuite: Return abstraction results
BenchmarkSuite-->>TestRunner: Report benchmark outcomes
Poem
Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. ✨ Finishing Touches
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
CMakeLists.txt(1 hunks)benchmark.cpp(10 hunks)
🔇 Additional comments (3)
benchmark.cpp (3)
27-35: Consider using std::filesystem::current_path for exePath on Windows.The current implementation of
exePath()on Windows usesGetModuleFileNameW, butstd::filesystem::current_path()could provide a simpler cross-platform solution for some use cases.The current implementation correctly gets the executable path on both platforms, but for some scenarios where just the current directory is needed, consider
std::filesystem::current_path()as an alternative.
314-380: Well-implemented cross-platform benchmarks.The addition of extensive cross-platform benchmarks for file and memory operations provides valuable performance comparisons between Windows and Unix-like systems. The code is well-structured with appropriate conditional compilation.
The implementation correctly handles:
- Platform-specific memory allocation (VirtualAlloc vs mmap)
- Platform-specific file operations (CreateFile vs open)
- Memory mapping differences (MapViewOfFile vs mmap)
- Page size detection (GetSystemInfo vs sysconf)
This comprehensive set of benchmarks will be valuable for comparing performance characteristics across platforms.
Also applies to: 382-454, 456-591, 593-699, 701-775, 777-845
151-153:Details
❓ Verification inconclusive
Implement proper file flushing for Windows when using writer.
Similar to the previous "??" comment, implement the equivalent Windows operation for the
decodeless::file_writer.#ifdef _WIN32 - // ?? + // Ensure data is flushed to disk + f.flush(); // Assuming flush() is available in file_writer #else sync(); #endif
🏁 Script executed:
#!/bin/bash # Check if decodeless::file_writer has a flush method rg -A 10 -B 10 "class file_writer" --include="*.hpp"Length of output: 496
🏁 Script executed:
#!/bin/bash # Search for the file_writer class definition in headers rg 'class[[:space:]]*file_writer' -g '*.hpp' -A 10 -B 10 # Search for any flush method declarations in headers rg 'flush[[:space:]]*\(' -g '*.hpp'Length of output: 394
🏁 Script executed:
#!/bin/bash # Include ignored files to ensure headers are searched rg -u 'class[[:space:]]*file_writer' -A 10 -B 10 rg -u 'flush[[:space:]]*\('Length of output: 248
Verify Windows flush support for
decodeless::file_writerWe weren’t able to locate the definition of
decodeless::file_writerin the headers to confirm whether it provides aflush()method or implements an OS‐level flush. Please verify and, if needed, extend the implementation:• Confirm that
decodeless::file_writer(in the appropriate header) declares aflush()member.
• Ifflush()only clears C++ buffers, add a Windows‐specific call to flush OS buffers (e.g.FlushFileBuffers).
• Updatebenchmark.cpp(around lines 151–153) as follows:#ifdef _WIN32 - // ?? + // Ensure data is flushed to disk on Windows + f.flush(); // flush C++ buffers + // TODO: call OS buffer flush (e.g., FlushFileBuffers on f’s handle) #else sync(); #endif
| FetchContent_Declare( | ||
| decodeless_mappedfile | ||
| GIT_REPOSITORY https://github.com/decodeless/mappedfile.git | ||
| GIT_TAG 6550dc283b3a01de1c0d02c79bfd6422812d70e8 | ||
| GIT_SHALLOW TRUE) | ||
| FetchContent_MakeAvailable(decodeless_mappedfile) |
There was a problem hiding this comment.
Add missing decodeless_mappedfile to target_link_libraries.
The new dependency decodeless_mappedfile is correctly declared and fetched, but not linked with the decodeless_benchmark target. Since the benchmark code uses types like decodeless::resizable_file and decodeless::resizable_memory from this library, it should be added to the target_link_libraries section.
target_link_libraries(decodeless_benchmark PRIVATE
decodeless::writer
decodeless::offset_ptr
+ decodeless::mappedfile
gtest_main
nanobench
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| FetchContent_Declare( | |
| decodeless_mappedfile | |
| GIT_REPOSITORY https://github.com/decodeless/mappedfile.git | |
| GIT_TAG 6550dc283b3a01de1c0d02c79bfd6422812d70e8 | |
| GIT_SHALLOW TRUE) | |
| FetchContent_MakeAvailable(decodeless_mappedfile) | |
| target_link_libraries(decodeless_benchmark PRIVATE | |
| decodeless::writer | |
| decodeless::offset_ptr | |
| decodeless::mappedfile | |
| gtest_main | |
| nanobench | |
| ) |
🤖 Prompt for AI Agents (early access)
In CMakeLists.txt around lines 32 to 37, the decodeless_mappedfile dependency is
fetched but not linked to the decodeless_benchmark target. To fix this, add
decodeless_mappedfile to the target_link_libraries command for
decodeless_benchmark so the benchmark target correctly links against this
library and can use its types.
| .run("MapViewOfFile", | ||
| [&] { | ||
| HANDLE hFile = CreateFileW(resultMmap.path.generic_wstring().c_str(), | ||
| GENERIC_READ | GENERIC_WRITE, 0, nullptr, CREATE_ALWAYS, | ||
| FILE_ATTRIBUTE_NORMAL, nullptr); | ||
| CHECK(hFile != INVALID_HANDLE_VALUE); | ||
|
|
||
| size_t elements = numIntsToWrite; | ||
| size_t size = sizeof(int32_t) * elements; | ||
| LARGE_INTEGER liSize; | ||
| liSize.QuadPart = size; | ||
| CHECK(SetFilePointerEx(hFile, liSize, nullptr, FILE_BEGIN)); | ||
| CHECK(SetEndOfFile(hFile)); | ||
|
|
||
| HANDLE hMap = CreateFileMappingW(hFile, nullptr, PAGE_READWRITE, 0, 0, nullptr); | ||
| CHECK(hMap != nullptr); | ||
|
|
||
| auto mapped = (int32_t*)MapViewOfFile(hMap, FILE_MAP_WRITE, 0, 0, size); | ||
| CHECK(mapped != nullptr); | ||
|
|
||
| for (size_t i = 0; i < elements; ++i) | ||
| mapped[i] = int32_t(i); | ||
|
|
||
| CHECK(FlushViewOfFile(mapped, size)); | ||
| CHECK(FlushFileBuffers(hFile)); | ||
| CHECK(UnmapViewOfFile(mapped)); | ||
| CloseHandle(hMap); | ||
| CloseHandle(hFile); | ||
| }) |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Add error handling for Windows API calls.
While the code checks for errors after most Windows API calls using CHECK, some calls like CloseHandle aren't checked, and the error handling could be more descriptive for debugging purposes.
Consider enhancing the error handling for Windows API calls. For example:
- CloseHandle(hMap);
- CloseHandle(hFile);
+ CHECK(CloseHandle(hMap));
+ CHECK(CloseHandle(hFile));Additionally, consider adding more descriptive error messages with GetLastError() for better debuggability:
if (!FlushViewOfFile(mapped, size)) {
DWORD error = GetLastError();
std::cerr << "FlushViewOfFile failed with error: " << error << std::endl;
throw std::runtime_error("FlushViewOfFile failed");
}🤖 Prompt for AI Agents (early access)
In benchmark.cpp around lines 102 to 130, improve error handling by checking the
return values of all Windows API calls, including CloseHandle, and replace
generic CHECK macros with explicit if-statements that retrieve and log detailed
error information using GetLastError(). For each API call, if it fails, capture
the error code, output a descriptive error message to standard error, and throw
an exception or handle the error appropriately to aid debugging.
| #define _CRT_SECURE_NO_DEPRECATE | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Make security deprecation suppression Windows-specific.
The _CRT_SECURE_NO_DEPRECATE define should be conditionally included only for Windows builds to avoid affecting other platforms unnecessarily.
-#define _CRT_SECURE_NO_DEPRECATE
+#ifdef _WIN32
+#define _CRT_SECURE_NO_DEPRECATE
+#endif🤖 Prompt for AI Agents (early access)
In benchmark.cpp around lines 3 to 4, the _CRT_SECURE_NO_DEPRECATE macro is
defined unconditionally, which can affect non-Windows platforms. Wrap the
#define _CRT_SECURE_NO_DEPRECATE directive inside a preprocessor check that
ensures it is only defined when compiling on Windows, such as using #ifdef
_WIN32 or a similar Windows-specific macro.
| // ?? | ||
| #else |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Implement proper file flushing for Windows.
The "??" comment indicates uncertainty about the equivalent Windows operation for sync(). For Windows, when using std::ofstream, you should call _flushall() or get the underlying file handle and use FlushFileBuffers().
#ifdef _WIN32
- // ??
+ // Flush file buffers to disk
+ f.close();
+ _flushall(); // Flush all open buffers
#else
sync();
#endifCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents (early access)
In benchmark.cpp around lines 96 to 97, replace the placeholder "??" comment
with proper file flushing code for Windows. Use either the _flushall() function
or obtain the underlying file handle from the std::ofstream and call
FlushFileBuffers() to ensure data is flushed to disk correctly on Windows
systems.
There was a problem hiding this comment.
The intent is not a flush, but to prevent OS write/disk caching. I.e. https://stackoverflow.com/questions/7575313/windows-equivalent-to-sync
There was a problem hiding this comment.
@pknowles Thank you for the clarification. You're right - sync() is about forcing OS disk cache writes, not just flushing buffers. Windows indeed doesn't have a direct equivalent to the system-wide sync() function.
For the std::ofstream case, you could:
#ifdef _WIN32
// Windows has no direct equivalent to sync()
// f.close() will flush the stream, but OS might still cache the write
f.close();
#else
sync();
#endifFor truly equivalent behavior (forcing disk writes), you'd need low-level Windows API calls that aren't always reliable across different Windows versions. Since this is benchmark code, it might be worth documenting this platform difference.
(｡･ω･｡) considering the technical details
✏️ Learnings added
Learnt from: pknowles
PR: decodeless/benchmark#2
File: benchmark.cpp:96-97
Timestamp: 2025-05-09T05:42:31.813Z
Learning: In the benchmark code, `sync()` is used on Linux to prevent OS write/disk caching and ensure accurate timing, but Windows doesn't have a direct equivalent function. The closest alternatives are using `FlushFileBuffers()` on specific file handles or accepting that Windows may use disk caching differently.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| void* reserved = mmap(nullptr, reservedAddressSpace, PROT_NONE, MAP_PRIVATE | MAP_ANONYMOUS | MAP_NORESERVE, -1, 0); | ||
| int f = open(mappedFile.path.c_str(), O_RDWR | O_CREAT | O_TRUNC, (mode_t)0600); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Same MAP_NORESERVE issue in another location.
The MAP_NORESERVE flag used here has the same cross-platform compatibility issue.
- void* reserved = mmap(nullptr, reservedAddressSpace, PROT_NONE, MAP_PRIVATE | MAP_ANONYMOUS | MAP_NORESERVE, -1, 0);
+ void* reserved = mmap(nullptr, reservedAddressSpace, PROT_NONE, MAP_PRIVATE | MAP_ANONYMOUS
+#ifdef MAP_NORESERVE
+ | MAP_NORESERVE
+#endif
+ , -1, 0);🤖 Prompt for AI Agents (early access)
In benchmark.cpp around lines 678 to 679, the use of the MAP_NORESERVE flag in
the mmap call causes cross-platform compatibility issues. Remove the
MAP_NORESERVE flag from the mmap flags argument and adjust the code to handle
memory reservation without relying on this flag, ensuring compatibility across
different operating systems.
| .run("mmap", | ||
| [&] { | ||
| size_t pageSize = sysconf(_SC_PAGESIZE); | ||
| void* reserved = mmap(nullptr, reservedAddressSpace, PROT_NONE, MAP_PRIVATE | MAP_ANONYMOUS | MAP_NORESERVE, -1, 0); | ||
| int f = open(mappedFile.path.c_str(), O_RDWR | O_CREAT | O_TRUNC, (mode_t)0600); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Fix mmap flag MAP_NORESERVE for cross-platform compatibility.
The MAP_NORESERVE flag is not universally available across all Unix-like platforms. Some systems might not support this flag, which could cause compilation errors.
- void* reserved = mmap(nullptr, reservedAddressSpace, PROT_NONE, MAP_PRIVATE | MAP_ANONYMOUS | MAP_NORESERVE, -1, 0);
+ void* reserved = mmap(nullptr, reservedAddressSpace, PROT_NONE, MAP_PRIVATE | MAP_ANONYMOUS
+#ifdef MAP_NORESERVE
+ | MAP_NORESERVE
+#endif
+ , -1, 0);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| .run("mmap", | |
| [&] { | |
| size_t pageSize = sysconf(_SC_PAGESIZE); | |
| void* reserved = mmap(nullptr, reservedAddressSpace, PROT_NONE, MAP_PRIVATE | MAP_ANONYMOUS | MAP_NORESERVE, -1, 0); | |
| int f = open(mappedFile.path.c_str(), O_RDWR | O_CREAT | O_TRUNC, (mode_t)0600); | |
| .run("mmap", | |
| [&] { | |
| size_t pageSize = sysconf(_SC_PAGESIZE); | |
| void* reserved = mmap(nullptr, reservedAddressSpace, PROT_NONE, MAP_PRIVATE | MAP_ANONYMOUS | |
| #ifdef MAP_NORESERVE | |
| | MAP_NORESERVE | |
| #endif | |
| , -1, 0); | |
| int f = open(mappedFile.path.c_str(), O_RDWR | O_CREAT | O_TRUNC, (mode_t)0600); |
🤖 Prompt for AI Agents (early access)
In benchmark.cpp around lines 553 to 557, the use of the MAP_NORESERVE flag in
the mmap call may cause compilation errors on platforms where this flag is not
supported. To fix this, add conditional compilation directives to include
MAP_NORESERVE only if it is defined on the target platform. Use #ifdef or
similar preprocessor checks to ensure cross-platform compatibility by excluding
MAP_NORESERVE where it is unavailable.
Motivated by decodeless/mappedfile#7, tracked by #1.
Summary by CodeRabbit
New Features
Chores
Tests