[Windows Build] Implement MMAP for mmap_data_loader.cpp #5164

jsidewhite · 2024-09-07T22:14:35Z

There is no sys/mman.h or posix-compatible mmap() implementation on Windows. The extension data loaders use it to map in data files, so adding an implementation.

Test-run,

& .\cmake-out\extension\data_loader\test\Debug\extension_data_loader_test.exe --gtest_brief=1 --gtest_filter=MmapDataLoader*
Running main() from ...\executorch\third-party\googletest\googletest\src\gtest_main.cc
[==========] 8 tests from 1 test suite ran. (50 ms total)
[ PASSED ] 8 tests.

For issue #4661

There is no sys/mman.h or posix-compatible mmap() implementation on Windows. The extension data loaders use it to map in data files, so adding an implementation. Test-run, & .\cmake-out\extension\data_loader\test\Debug\extension_data_loader_test.exe --gtest_brief=1 --gtest_filter=MmapDataLoader* Running main() from ...\executorch\third-party\googletest\googletest\src\gtest_main.cc [==========] 8 tests from 1 test suite ran. (50 ms total) [ PASSED ] 8 tests.

pytorch-bot · 2024-09-07T22:14:38Z

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/5164

📄 Preview Python docs built from this PR

Note: Links to docs will display an error until the docs builds have been completed.

This comment was automatically generated by Dr. CI and updates every 15 minutes.

facebook-github-bot · 2024-09-07T22:14:41Z

Hi @jsidewhite!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at [email protected]. Thanks!

dbort

I'm so sorry for the delay! I totally missed this PR. This looks like a good approach.

dbort · 2025-01-09T22:32:48Z

extension/data_loader/mman_windows.cpp

+
+#include <errno.h>
+#include <io.h>
+#include <windows.h>
+
+#include "mman_windows.h"
+


Please use angle brackets and the full path to the header

.cpp files should include their .h file first to demonstrate that the .h files don't need earlier includes to work cleanly

Suggested change

#include <errno.h>

#include <io.h>

#include <windows.h>

#include "mman_windows.h"

#include <executorch/extension/data_loader/mman_windows.h>

#include <errno.h>

#include <io.h>

#include <windows.h>

dbort · 2025-01-09T22:37:10Z

extension/data_loader/test/mmap_data_loader_test.cpp

@@ -6,12 +6,11 @@
 * LICENSE file in the root directory of this source tree.
 */

+#include <executorch/extension/data_loader/mman.h>


Please move this include down to the group on line 17. Tests should always include their header-under-test first to demonstrate that the header can be included cleanly without needing other includes.

dbort · 2025-01-09T22:38:36Z

extension/data_loader/mman_windows.h

+#ifdef __cplusplus
+extern "C" {
+#endif


ExecuTorch is always C++, so no need for this or the matching one below

dbort · 2025-01-09T22:38:58Z

extension/data_loader/mman_windows.h

+#ifndef _SYS_MMAN_H_
+#define _SYS_MMAN_H_


Please use #pragma once

dbort · 2025-01-09T22:40:25Z

extension/data_loader/mman.h

+#include <sys/mman.h>
+#include <unistd.h>
+
+ET_INLINE long get_os_page_size(){return sysconf(_SC_PAGESIZE)}


Not sure if this will compile without a semicolon. And the formatting is off

Suggested change

ET_INLINE long get_os_page_size(){return sysconf(_SC_PAGESIZE)}

ET_INLINE long get_os_page_size() {

return sysconf(_SC_PAGESIZE);

}

dbort · 2025-01-09T22:42:23Z

extension/data_loader/mman.h

+#include <sys/mman.h>
+#include <unistd.h>
+
+ET_INLINE long get_os_page_size(){return sysconf(_SC_PAGESIZE)}


Now that we're wrapping this, I'd like to return size_t instead of long to be more consistent with the types that ET uses.

dbort · 2025-01-09T22:45:05Z

extension/data_loader/mmap_data_loader.cpp

@@ -6,17 +6,16 @@
 * LICENSE file in the root directory of this source tree.
 */

+#include <executorch/extension/data_loader/mman.h>


Please move this include down to the group on line 21 so that this .cpp file includes its .h file first.

dbort · 2025-01-09T22:49:48Z

extension/data_loader/mmap_data_loader.cpp

+  size_t map_size = range.size;
+#ifdef _WIN32
+  // On Windows, don't mmap-in memory past end of on-disk file.
+  //
+  // The Windows implementation of mmap uses CreateFileMapping which returns
+  // error STATUS_SECTION_TOO_BIG (0xc0000040) if we try to map past the end
+  // of the last page of a file mapped in as read-only.
+  if (range.start + range.size > file_size_) {
+    map_size = file_size_ - range.start;
+  }
+#endif


Seems reasonable to always do this, if it works on unix systems; we're trying to minimize the number of ifdefs in common code.

Suggested change

size_t map_size = range.size;

#ifdef _WIN32

// On Windows, don't mmap-in memory past end of on-disk file.

//

// The Windows implementation of mmap uses CreateFileMapping which returns

// error STATUS_SECTION_TOO_BIG (0xc0000040) if we try to map past the end

// of the last page of a file mapped in as read-only.

if (range.start + range.size > file_size_) {

map_size = file_size_ - range.start;

}

#endif

size_t map_size = range.size;

if (range.start + map_size > file_size_) {

// Clamp to the end of the file.

//

// The Windows implementation of mmap uses CreateFileMapping which returns

// error STATUS_SECTION_TOO_BIG (0xc0000040) if we try to map past the end

// of the last page of a file mapped in as read-only.

map_size = file_size_ - range.start;

}

dbort · 2025-01-09T22:52:26Z

extension/data_loader/targets.bzl

+        srcs = [
+            "mmap_data_loader.cpp",
+            "mman_windows.cpp"
+        ] if host_info().os.is_windows else [
+            "mmap_data_loader.cpp"
+        ],


Something like this would avoid repeating the common part.

Suggested change

srcs = [

"mmap_data_loader.cpp",

"mman_windows.cpp"

] if host_info().os.is_windows else [

"mmap_data_loader.cpp"

],

srcs = ["mmap_data_loader.cpp"] +

["mman_windows.cpp"] if host_info().os.is_windows else [],

dbort · 2025-01-09T22:52:40Z

extension/data_loader/targets.bzl

+        ],
+        headers = [
+            "mman.h",
+            "mman_windows.h"


Please also make this header conditional on is_windows

ykhrustalev · 2025-01-31T20:33:22Z

@jsidewhite can I offer help on adding changes here?

swolchok requested a review from dbort November 25, 2024 21:20

vortex-captain mentioned this pull request Dec 4, 2024

Windows x64 Build with support for xnnpack and llama example #6979

Closed

chinazhangchao mentioned this pull request Dec 6, 2024

Support build on native Windows x64 #7217

Closed

dbort requested changes Jan 9, 2025

View reviewed changes

dbort mentioned this pull request Jan 31, 2025

Support build on native Windows #4661

Open

8 tasks

dbort requested a review from tarun292 March 1, 2025 01:25

SamGondelman mentioned this pull request Mar 4, 2025

[Windows] [mmap_data_loader.cpp] mmap equivalent for Windows #8916

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Windows Build] Implement MMAP for mmap_data_loader.cpp #5164

[Windows Build] Implement MMAP for mmap_data_loader.cpp #5164

jsidewhite commented Sep 7, 2024

pytorch-bot bot commented Sep 7, 2024

facebook-github-bot commented Sep 7, 2024

dbort left a comment

dbort Jan 9, 2025

dbort Jan 9, 2025

dbort Jan 9, 2025

dbort Jan 9, 2025

dbort Jan 9, 2025

dbort Jan 9, 2025

dbort Jan 9, 2025

dbort Jan 9, 2025

dbort Jan 9, 2025

dbort Jan 9, 2025

ykhrustalev commented Jan 31, 2025

-ET_INLINE long get_os_page_size(){return sysconf(_SC_PAGESIZE)}
+ET_INLINE long get_os_page_size() {
+  return sysconf(_SC_PAGESIZE);
+}

		#ifndef _SYS_MMAN_H_
		#define _SYS_MMAN_H_

[Windows Build] Implement MMAP for mmap_data_loader.cpp #5164

Are you sure you want to change the base?

[Windows Build] Implement MMAP for mmap_data_loader.cpp #5164

Conversation

jsidewhite commented Sep 7, 2024

pytorch-bot bot commented Sep 7, 2024

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/5164

facebook-github-bot commented Sep 7, 2024

Action Required

Process

dbort left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ykhrustalev commented Jan 31, 2025