Skip to content

Use rustix/windows-sys for page size; drop maplit & page_size#7876

Open
joshuamegnauth54 wants to merge 1 commit into
RustPython:mainfrom
joshuamegnauth54:drop-some-unused-deps
Open

Use rustix/windows-sys for page size; drop maplit & page_size#7876
joshuamegnauth54 wants to merge 1 commit into
RustPython:mainfrom
joshuamegnauth54:drop-some-unused-deps

Conversation

@joshuamegnauth54
Copy link
Copy Markdown
Contributor

@joshuamegnauth54 joshuamegnauth54 commented May 16, 2026

The page_size crate is a simple libc wrapper for Unix and uses winapi-rs for Windows. windows-sys and windows-rs are the modern alternatives for winapi-rs. Both crates are maintained by Microsoft - they're official. Getting the page size for Unix is a simple call. Rustix can do it safely for us. Windows is effectively the same.

Besides Unix and Windows, I also added the page size for wasm32 which the page_size crate did not support. Rust has a built-in API for wasm32's page size, so I didn't add any new dependencies.

Finally, I dropped maplit which is seven years old and only used in one place. Using collect() with a single item iterator is better in this case because Rust can optimize it. maplist called HashMap::insert which would over allocate to amortize future allocs.

Summary by CodeRabbit

  • Chores
    • Updated project dependencies (removed unused crates, replaced page-size dependency, and adjusted feature flags).
  • Chores
    • Refactored memory-mapping and page-size handling; Python attributes PAGESIZE and ALLOCATIONGRANULARITY now use explicit, platform-specific implementations for Unix, Windows, and WebAssembly.

Review Change Stack

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 16, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Run ID: 55e86203-1000-4824-abbc-32cf502c291d

📥 Commits

Reviewing files that changed from the base of the PR and between 8be12df and 1ae5bc9.

⛔ Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (6)
  • Cargo.toml
  • crates/derive-impl/Cargo.toml
  • crates/derive-impl/src/compile_bytecode.rs
  • crates/derive-impl/src/lib.rs
  • crates/stdlib/Cargo.toml
  • crates/stdlib/src/mmap.rs
💤 Files with no reviewable changes (2)
  • crates/derive-impl/src/lib.rs
  • crates/derive-impl/Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (4)
  • crates/derive-impl/src/compile_bytecode.rs
  • crates/stdlib/Cargo.toml
  • crates/stdlib/src/mmap.rs
  • Cargo.toml

📝 Walkthrough

Walkthrough

Removes the maplit workspace and crate usages and refactors a single-entry HashMap construction; replaces page_size with memmap2/rustix::param and adds explicit Unix/wasm32/Windows page-size implementations and a Windows feature addition.

Changes

Dependency Simplification and Platform Abstraction

Layer / File(s) Summary
Remove maplit and refactor HashMap construction
Cargo.toml, crates/derive-impl/Cargo.toml, crates/derive-impl/src/lib.rs, crates/derive-impl/src/compile_bytecode.rs
Removes maplit from workspace and from crates/derive-impl, deletes the crate-root #[macro_use] extern crate maplit;, and rewrites a single-entry HashMap construction from hashmap! { ... } to core::iter::once(...).collect().
Replace page_size with rustix and platform-specific implementations
Cargo.toml, crates/stdlib/Cargo.toml, crates/stdlib/src/mmap.rs
Removes page_size from workspace deps, enables rustix param feature, adds memmap2 for non-wasm targets, extends windows-sys features, adjusts Windows imports, and implements PAGESIZE/ALLOCATIONGRANULARITY via rustix::param (Unix), memory_size(0) (wasm32), and GetSystemInfo (Windows).

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Suggested reviewers

  • ShaharNaveh
  • youknowone

Poem

🐰 I hopped through Cargo.toml, nibbling maplit away,
Rustix brought param magic to brighten the day.
Unix counts pages, wasm peeks memory's span,
Windows calls GetSystemInfo—such a clever plan.
Hop, patch, and compile — a carrot-coded play.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title directly and accurately summarizes the main changes: replacing page_size and maplit dependencies with rustix and windows-sys, and removing the unused maplit crate.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with 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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (1)
crates/stdlib/src/mmap.rs (1)

198-228: 💤 Low value

Consider extracting the common GetSystemInfo pattern.

Both Windows implementations create a SYSTEM_INFO, call GetSystemInfo, and return a field cast to usize. This pattern could be extracted to reduce duplication.

♻️ Optional refactor
+#[cfg(windows)]
+fn get_system_info() -> SYSTEM_INFO {
+    let mut info = SYSTEM_INFO::default();
+    unsafe {
+        GetSystemInfo(&mut info);
+    }
+    info
+}
+
 #[cfg(windows)]
 #[pyattr(name = "PAGESIZE", once)]
 fn page_size(_vm: &VirtualMachine) -> usize {
-    let mut info = SYSTEM_INFO::default();
-    unsafe {
-        GetSystemInfo(&mut info);
-    }
-    info.dwPageSize as _
+    get_system_info().dwPageSize as _
 }

 #[cfg(windows)]
 #[pyattr(name = "ALLOCATIONGRANULARITY", once)]
 fn granularity(_vm: &VirtualMachine) -> usize {
-    let mut info = SYSTEM_INFO::default();
-    unsafe {
-        GetSystemInfo(&mut info);
-    }
-    info.dwAllocationGranularity as _
+    get_system_info().dwAllocationGranularity as _
 }
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/stdlib/src/mmap.rs` around lines 198 - 228, Extract the duplicated
GetSystemInfo pattern into a small helper (e.g., a fn get_system_info() ->
SYSTEM_INFO) and use it from both Windows functions to avoid repeating
allocation and unsafe block: replace the body of page_size and the windows
granularity with calls to this helper and return the appropriate fields
(info.dwPageSize and info.dwAllocationGranularity) cast to usize; keep function
names page_size and granularity and ensure the helper encapsulates the unsafe
GetSystemInfo call and SYSTEM_INFO::default() initialization.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@crates/stdlib/src/mmap.rs`:
- Around line 192-196: The page_size implementation is wrong because
core::arch::wasm32::memory_size(0) returns the current memory page count, not
the fixed page size; change the wasm32 page_size (annotated with #[pyattr(name =
"PAGESIZE", once)] fn page_size) to return the constant WebAssembly page size
65_536 (64 KiB) instead of calling memory_size, and ensure any related symbol
that exposes ALLOCATIONGRANULARITY on wasm32 uses the same 65_536 constant (you
can introduce a WASM_PAGE_SIZE const and return that from page_size and
allocation_granularity).

---

Nitpick comments:
In `@crates/stdlib/src/mmap.rs`:
- Around line 198-228: Extract the duplicated GetSystemInfo pattern into a small
helper (e.g., a fn get_system_info() -> SYSTEM_INFO) and use it from both
Windows functions to avoid repeating allocation and unsafe block: replace the
body of page_size and the windows granularity with calls to this helper and
return the appropriate fields (info.dwPageSize and info.dwAllocationGranularity)
cast to usize; keep function names page_size and granularity and ensure the
helper encapsulates the unsafe GetSystemInfo call and SYSTEM_INFO::default()
initialization.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Run ID: 359f66fd-62d0-4fe6-9403-f86513e280b7

📥 Commits

Reviewing files that changed from the base of the PR and between ddfcb25 and 8be12df.

⛔ Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (6)
  • Cargo.toml
  • crates/derive-impl/Cargo.toml
  • crates/derive-impl/src/compile_bytecode.rs
  • crates/derive-impl/src/lib.rs
  • crates/stdlib/Cargo.toml
  • crates/stdlib/src/mmap.rs
💤 Files with no reviewable changes (2)
  • crates/derive-impl/Cargo.toml
  • crates/derive-impl/src/lib.rs

Comment thread crates/stdlib/src/mmap.rs
The `page_size` crate is a simple libc wrapper for Unix and uses
`winapi-rs` for Windows. `windows-sys` and `windows-rs` are the modern
alternatives for `winapi-rs`. Both crates are maintained by Microsoft -
they're official. Getting the page size for Unix is a simple call.
Rustix can do it safely for us. Windows is effectively the same.

Besides Unix and Windows, I also added the page size for wasm32 which
the `page_size` crate did not support. `wasm32`'s page size is defined
by the spec, so I hard coded it without adding new dependencies.

Finally, I dropped `maplit` which is seven years old and only used in
one place. Using `collect()` with a single item iterator is better in
this case because Rust can optimize it. `maplist` called
`HashMap::insert` which would over allocate to amortize future allocs.
@joshuamegnauth54 joshuamegnauth54 force-pushed the drop-some-unused-deps branch from 8be12df to 1ae5bc9 Compare May 16, 2026 01:25
Copy link
Copy Markdown
Contributor

@ShaharNaveh ShaharNaveh left a comment

Choose a reason for hiding this comment

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

Awesome!

ty for looking into it:)

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants