GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | |||||||||||||||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | |||||||||||||||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — Synchronize mmap buffer exports with close and resize by 1ndahous3 · Pull Request #9015 · RustPython/RustPython Conversation
|
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (1)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughThe mmap implementation now validates mappings during buffer creation and manages mapping guards during close and resize operations. Tests cover closed mappings, exported child views, and Windows resize behavior. Changesmmap guard lifecycle
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix Suggested reviewers: Merge Risk: ⚪ Minimal · up to This change makes mmap buffer exports synchronize with close and resize. No concrete merge-blocking risk was identified in the supplied context. The PR reports no test results, so the usual pre-merge checks still apply. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Here are some automated review suggestions for this pull request.
Reviewed commit: 7a6b4352d2
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
|
||
| #[pymethod] | ||
| fn close(zelf: &Py<Self>, vm: &VirtualMachine) -> PyResult<()> { | ||
| let mut mmap = zelf.mmap.lock(); |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Reject exported close calls before taking the data lock
When one thread passes this mmap to _SSLSocket.write, crates/stdlib/src/openssl.rs:3073-3104 holds the mmap guard via data.borrow_buf() across a potentially unbounded allow_threads call. A concurrent mapped.close() now blocks here instead of observing the already-nonzero export count and raising BufferError; because this mutex wait remains attached to the interpreter, it can also participate in a stop-the-world deadlock. Keep an unlocked export-count fast path before this acquisition and recheck under the lock to preserve the race fix; check_resizeable needs the same ordering.
Useful? React with 👍 / 👎.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Thanks, you're right. Fixed in aa7ac19. close() and check_resizeable() now reject existing exports before acquiring the mapping mutex, while retaining the check under the lock to prevent invalidating a concurrent export.
Both new regressions hold an actual borrowed mmap buffer across the operation. They failed before this change and now pass on Linux and Windows.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
Assisted-by: Codex:gpt-6
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Here are some automated review suggestions for this pull request.
Reviewed commit: aa7ac19eb5
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| with assert_raises(ValueError): | ||
| memoryview(mapped) |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Remove new assertions disallowed by the test policy
This block adds new test setup, assertions, and data to a test file, but the repository's critical test policy permits test-file edits only for adding or removing expected-failure markers (and their associated TODO comments). The added snippet therefore needs to be removed or the repository policy changed before landing.
AGENTS.md reference: AGENTS.md:L273-L279
Useful? React with 👍 / 👎.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
Merging this PR will improve performance by 15.38%
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ⚡ | rustpython[frozenset.py] |
411.6 µs | 356.8 µs | +15.38% |
Tip
Curious why performance improved? Comment @codspeedbot explain why performance improved on this PR, or directly use the CodSpeed MCP with your agent.
Comparing 1ndahous3:mmap_buffer_lifetime (aa7ac19) with main (54e47cd)
4 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
👍
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.