Skip to content

Conversation

@sepcnt
Copy link

@sepcnt sepcnt commented Nov 26, 2025

This pull request fix #961, adds initial Windows support to the project, focusing on enabling CI, memory management, thread affinity, and malloc functionality for Windows targets. The changes include updates to CI scripts and workflows to handle Windows-specific cases, platform-specific Rust code for memory and thread management, and a new implementation for malloc using Windows APIs.

Windows platform support

  • Added Windows-specific dependencies to Cargo.toml and implemented platform-specific code for thread affinity and CPU detection in src/scheduler/affinity.rs, using Windows APIs for processor count and thread affinity. [1] [2] [3] [4]
  • Implemented a Windows-specific malloc backend in src/util/malloc/library.rs, using HeapAlloc, HeapFree, and related APIs; updated allocation logic in src/util/malloc/malloc_ms_util.rs to support Windows. [1] [2] [3] [4] [5] [6]

CI and workflow updates

  • Modified .github/scripts/ci-common.sh and .github/workflows/minimal-tests-core.yml to add Windows jobs, set up bash shell defaults, and skip incompatible setup steps for Windows runners; also excluded malloc_jemalloc on Windows in feature initialization. [1] [2] [3] [4] [5] [6]

Memory management abstraction

  • Refactored src/util/memory.rs to abstract memory protection, mapping, and error handling for Windows, including platform-specific flag translation and error codes, and using Windows APIs for memory operations. [1] [2] [3] [4] [5] [6] [7] [8]

General codebase cleanup

  • Removed direct usage of libc::mprotect from src/policy/copyspace.rs, replacing it with platform-abstracted memory protection functions. [1] [2] [3]

These changes lay the foundation for building, testing, and running the project on Windows, with further improvements expected for full platform parity.

Copy link
Member

@qinsoon qinsoon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The implementation looks reasonable to me. The major issue is the implementation mmap_fixed (see details below).

@qinsoon
Copy link
Member

qinsoon commented Nov 27, 2025

@sepcnt Thanks for opening this PR. Just to better understand the context: do you plan to use MMTk on Windows for a specific project or runtime, or is this PR mainly to add general Windows support to mmtk-core?

Either way is fine -- I’d just like to understand your use case so we can make better long-term decisions around Windows support.

@sepcnt
Copy link
Author

sepcnt commented Nov 27, 2025

@sepcnt Thanks for opening this PR. Just to better understand the context: do you plan to use MMTk on Windows for a specific project or runtime, or is this PR mainly to add general Windows support to mmtk-core?

Either way is fine -- I’d just like to understand your use case so we can make better long-term decisions around Windows support.

While I am planning to build a runtime on the top of mmtk, this PR is mainly focus on making a 1:1 port.

@sepcnt sepcnt force-pushed the feat-windows-support branch from 8226c63 to b11ef21 Compare November 27, 2025 03:06
@qinsoon qinsoon added the PR-extended-testing Run extended tests for the pull request label Nov 27, 2025
@qinsoon
Copy link
Member

qinsoon commented Nov 27, 2025

The failed test for the Julia binding is unrelated with this PR.

@wks
Copy link
Collaborator

wks commented Nov 27, 2025

@sepcnt Thank you for your PR. While it is nice to support a popular platform, I think this change should be made after we refactor the mmtk-core code base and isolate OS-specific parts into dedicated modules, as suggested in #1420. This PR makes changes that are scattered over many modules, and often uses the pattern #[cfg(target_os = "windows")] and then #[cfg(not(target_os = "windows"))]. This makes the code hard to maintain.

Given that supporting Windows is not a priority of ours, it will be even better if it is possible to maintain Windows support outside our organization, as I mentioned in #1420 (comment). A refactoring is required to make this possible.

@qinsoon
Copy link
Member

qinsoon commented Nov 27, 2025

@sepcnt Thank you for your PR. While it is nice to support a popular platform, I think this change should be made after we refactor the mmtk-core code base and isolate OS-specific parts into dedicated modules, as suggested in #1420.

I feel it is a good idea to merge this PR before the refactoring. Supporting for an OS that is significantly different from Unix-like systems could actually help guide the refactoring process for an OS interface for us.

@wks
Copy link
Collaborator

wks commented Nov 27, 2025

@sepcnt Thank you for your PR. While it is nice to support a popular platform, I think this change should be made after we refactor the mmtk-core code base and isolate OS-specific parts into dedicated modules, as suggested in #1420.

I feel it is a good idea to merge this PR before the refactoring. Supporting for an OS that is significantly different from Unix-like systems could actually help guide the refactoring process for an OS interface for us.

A porting effort like this can definitely guide our progress of porting to another OS. But given that none of our core members can test this PR locally, and this PR is making a massive amount of changes to the code base using AI-assisted tools, I consider it a risk to merge it into the master branch directly. We may keep this pull request, and do our refactoring with this PR as a reference.

@sepcnt
Copy link
Author

sepcnt commented Nov 27, 2025

@sepcnt Thank you for your PR. While it is nice to support a popular platform, I think this change should be made after we refactor the mmtk-core code base and isolate OS-specific parts into dedicated modules, as suggested in #1420.

I feel it is a good idea to merge this PR before the refactoring. Supporting for an OS that is significantly different from Unix-like systems could actually help guide the refactoring process for an OS interface for us.

A porting effort like this can definitely guide our progress of porting to another OS. But given that none of our core members can test this PR locally, and this PR is making a massive amount of changes to the code base using AI-assisted tools, I consider it a risk to merge it into the master branch directly. We may keep this pull request, and do our refactoring with this PR as a reference.

Most of the changes are gated with cfg!. While this PR targets at Windows, I also tested on Ubuntu with below configurations:

sudo dpkg --add-architecture i386
sudo apt-get update && sudo apt-get install -y mingw-w64 wine64 wine32 xvfb
rustup target add x86_64-pc-windows-gnu

And in .cargo/config.toml:

[target.x86_64-pc-windows-gnu]
runner = "xvfb-run -a wine"
linker = "x86_64-w64-mingw32-gcc"

@qinsoon
Copy link
Member

qinsoon commented Nov 27, 2025

given that none of our core members can test this PR locally

Honestly, that does not seem to be a prerequisite for a PR to be merged. To me, for a PR to be merged, it needs to:

  1. pass all the tests/checks, and better have the changes tested.
  2. pass code review.
  3. align with the project's goal and design.

this PR is making a massive amount of changes to the code base using AI-assisted tools, I consider it a risk to merge it into the master branch directly.

We have code reviews and CI. Those are intended to mitigate the risk of merging to master. You can do code review and list the risky parts or code that may cause issues, and expect them to be addressed properly before merging.

As for AI/LLM coding, there is a lot of discussion recently. I don't want to go deep on this. To my own experience, human can write bad code, and AI (at this stage) under human supervision can write good code. So my general attitude is neutral -- I judge a PR by its content, rather than who wrote it (AI or human).

@wks
Copy link
Collaborator

wks commented Nov 27, 2025

Honestly, that does not seem to be a prerequisite for a PR to be merged. To me, for a PR to be merged, it needs to:

1. pass all the tests/checks, and better have the changes tested.

2. pass code review.

3. align with the project's goal and design.

If it's not yet a prerequisite, it should be.

Code reviewing and testing are ways to know if this version works. But if we accept this pull request, not only does it mean that we are accepting this change, but it also means we will have the responsibility to maintain it. It means for every change we make, we need to ensure we don't break Windows. And if some changes accidentally breaks Windows, we need to debug it. Given the result of this PR, i.e. #[cfg] scattered all over the place, it will be a nightmare for maintenance. So unless someone volunteers to do the architectural refactoring immediately, this is not going to be friendly to developers. However, all of our core members are currently quite occupied.

We have code reviews and CI. Those are intended to mitigate the risk of merging to master. You can do code review and list the risky parts or code that may cause issues, and expect them to be addressed properly before merging.

As for AI/LLM coding, there is a lot of discussion recently. I don't want to go deep on this. To my own experience, human can write bad code, and AI (at this stage) under human supervision can write good code. So my general attitude is neutral -- I judge a PR by its content, rather than who wrote it (AI or human).

There are also legal concerns about AI-generated code. AI-trained coding agents may be trained from and/or directly produce snippets that are incompatible with our license (Apache2 + MIT). Particularly, we cannot accept snippets that are GPL-licensed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

PR-extended-testing Run extended tests for the pull request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Windows support

3 participants