Skip to content

Conversation

@pknowles
Copy link
Contributor

@pknowles pknowles commented Oct 17, 2025

MAP_PRIVATE is backed by swap, which limits the maximum size

Summary by CodeRabbit

  • Refactor
    • Updated memory mapping behavior to ensure consistent file access patterns across all usage scenarios. No changes to the public API.

MAP_PRIVATE is backed by swap, which limits the maximum size
@coderabbitai
Copy link

coderabbitai bot commented Oct 17, 2025

Walkthrough

Modified the MappedFile constructor in the Linux implementation to consistently use MAP_SHARED for memory mapping, replacing the previous conditional behavior that selected MAP_PRIVATE for non-writable mappings and MAP_SHARED for writable ones.

Changes

Cohort / File(s) Summary
MappedFile Linux Mapping Behavior
include/decodeless/detail/mappedfile_linux.hpp
Changed constructor memory mapping flags from conditional (MAP_PRIVATE for read-only, MAP_SHARED for writable) to always use MAP_SHARED

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Possibly related PRs

Poem

🐰 A rabbit hops through memory lanes,
Where MAP_SHARED now reigns,
No more split decisions, hooray!
All mappings share the same way. 🗺️

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The pull request title "MAP_SHARED even for read-only" directly and accurately describes the primary change in the changeset. The title clearly communicates that the modified code now uses MAP_SHARED memory mapping flags universally, including for read-only file mappings, which were previously handled with MAP_PRIVATE. The title is concise, specific, and would allow a developer reviewing the repository history to immediately understand the technical nature and intent of the change. It avoids vague terminology and noise while capturing the essence of the modification.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch dev-shared-readonly

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d2b9b43 and a214250.

📒 Files selected for processing (1)
  • include/decodeless/detail/mappedfile_linux.hpp (1 hunks)
⏰ 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). (2)
  • GitHub Check: build (windows-latest, Release, cl)
  • GitHub Check: build (windows-latest, Release, cl)
🔇 Additional comments (1)
include/decodeless/detail/mappedfile_linux.hpp (1)

199-199: Semantic change noted, but verify file immutability assumptions are documented.

Changing from MAP_PRIVATE to MAP_SHARED for read-only mappings does alter semantics—the application will now see live file changes rather than a stable snapshot. However, this is unlikely to be problematic in practice:

  • Read-only mode (opened with O_RDONLY, protected PROT_READ) prevents the application from writing changes.
  • The codebase shows no evidence of concurrent file modification scenarios; usage is static and single-threaded.
  • MAP_SHARED for read-only is a standard approach to avoid swap-backed limitations.

Action: Document the assumption that mapped files remain immutable (or at least don't change unexpectedly during the application's use of the mapping). Add a brief note in the file type documentation clarifying this contract, especially if this is a public API.


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.

@pknowles pknowles merged commit a214250 into main Oct 17, 2025
12 checks passed
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