Introduce PyAtomicRef and impl for PyObject.typ by qingshi163 · Pull Request #3828 · RustPython/RustPython

qingshi163 · 2022-06-29T15:11:58Z

@youknowone correctness and performance check?

fanninpm · 2022-06-29T16:34:33Z

Feel free to ignore those Clippy failures. @youknowone likely missed them in #3817.

youknowone · 2022-06-29T20:26:31Z

+    type>

            
+
+    fn deref(&self) -> &Self::Target {
+        unsafe { self.ptr.load().as_ref() }


This is looking unsafe.

While thread 1 called deref() to obtain &Py<T>, inner can be swapped out in thread 2.
When the swapped out object is dropped, thread 1 will have a dangling pointer.

qingshi163 · 2022-08-07T14:24:25Z

#3970

qingshi163 · 2022-08-08T10:22:26Z

Is this approach safe as we manaully require the reference to the pytype cannot across between the frames. Most of the case we use the pytype is either using pytyperef or using the reference immediately.
@youknowone

youknowone · 2022-08-22T08:37:34Z

Oh, I missed your last push. I will check the changes soon.

youknowone · 2022-08-28T16:43:20Z

 }

-impl PyTypeRef {
+impl Py<PyType> {


This is a great idea.

youknowone

Logically looking good to me.

youknowone · 2022-08-28T16:46:23Z

    /// tracer function for this frame (usually is None)
    pub trace: PyMutex<PyObjectRef>,
    state: PyMutex<FrameState>,
+    pub temperary_refs: PyMutex<Vec<PyObjectRef>>,


Maybe we can reuse this for del.
I didn't know many fields in Frame are mutex. I thought they are single-thread accessible.

typo

Suggested change

pub temperary_refs: PyMutex<Vec<PyObjectRef>>,

pub temporary_refs: PyMutex<Vec<PyObjectRef>>,

If frame itself is executed single-threaded, we could replace mutex with an atomic increasing vec, do we have any crates doing that?

I don't think we have those crate. When we create safety by human, UnsafeCell is the common tool

youknowone · 2022-08-28T16:53:20Z

+    #[must_use]
+    pub unsafe fn swap(&self, pyref: PyRef<T>) -> PyRef<T> {
+        let py = PyRef::leak(pyref);
+        let old = self.0.swap(NonNull::from(py));


Suggested change

let old = self.0.swap(NonNull::from(py));

let old = self.0.swap(NonNull::new_unchecked(py));

we already know py is not a null.

I am not sure possible but retaining &'static mut T will be good when available.

these two are equal. no check will be made as NonNull::from only accept valid value and it is calling new_unchecked inside the function

what do you mean about &'static mut T

oh, got it. py is a reference, not a pointer.

I was thinking if this is possible.

pub struct PyAtomicRef<T: PyObjectPayload>(AtomicCell<&'static mut Py<T>>);

I think I've found over the past while, mainly from looking at crossbeam's issue list, that atomics aren't actually able to be hidden away enough such that AtomicCell would be a good abstraction. I'd honestly prefer a PyAtomic<*mut Py<T>>, which is a type alias that resolves to std::sync::atomic::AtomicPtr when threading is enabled, and Cell<*mut _> when it's not. I meant to go and replace all the AtomicCell uses with PyAtomic after I added PyAtomic, but I never got around to it.

youknowone · 2022-08-29T04:03:11Z

    }
 }

+pub struct PyAtomicRef<T: PyObjectPayload>(AtomicCell<NonNull<Py<T>>>);


Will using AtomicCell<NonNull> spend same cost as AtomicPtr?

AtomicCell::::is_lock_free() should be true for the platforms that support AtomicPtr. But I am not sure about any overhead that AtomicCell does.

coolreader18 · 2022-08-29T05:43:08Z

+    pub fn swap_to_temporary_refs(&self, pyref: PyRef<T>, vm: &VirtualMachine) {
+        let old = unsafe { self.swap(pyref) };
+        if let Some(frame) = vm.current_frame() {
+            frame.temporary_refs.lock().push(old.into());


Does temporary_refs ever get cleared? Like I guess it doesn't need to be, necessarily, but I don't like the idea of

class A: pass class B: pass B().__class__ = A

meaning that A will never ever ever be deallocated.

What if instead class() returned a guard.... no that wouldn't work. I'll try and think of something by the morning, maybe.

I guess when the frame is popped

Ohhh, I missed that its per-frame, yeah I guess that does make sense. Nevermind!

I guess when the frame is popped

Yes, we can clear the refs for each instruction, but I don't think it is worth.

The errors were not rebase problem

coolreader18 · 2022-08-29T17:35:11Z

(using PyAtomic instead of AtomicCell would fix the provenance errors in miri)

qingshi163 · 2022-08-29T17:50:36Z

(using PyAtomic instead of AtomicCell would fix the provenance errors in miri)

let me try

coolreader18 · 2022-08-29T18:35:18Z

Also, vm.frames should definitely be made private now, since it's now a safety thing that a frame cannot be popped off the frame stack + dropped before it's fully done running.

qingshi163 · 2022-08-29T19:21:19Z

(using PyAtomic instead of AtomicCell would fix the provenance errors in miri)

let me try

Don't know why but seem like failing the compile when threading is off, any mistake about how to use PyAtomic?
@coolreader18

qingshi163 · 2022-08-31T18:54:33Z

(using PyAtomic instead of AtomicCell would fix the provenance errors in miri)

let me try

Don't know why but seem like failing the compile when threading is off, any mistake about how to use PyAtomic? @coolreader18

nevermind, it just the name conflict

qingshi163 · 2022-10-02T18:34:36Z

@coolreader18 @youknowone review?

youknowone

@qingshi163 looks good to me.
If @coolreader18 is too busy to review this PR, please feel free to merge it yourself.

Thank you for the awesome work!

* Remove unused PyLease PyLease wrapped a read guard over the object's type when `typ` was a `PyRwLock<PyTypeRef>`. Its last users, `class()` and `lease_class()`, went away when `typ` became a `PyAtomicRef<PyType>` (#3828). Its field is private and it has no constructor, so it cannot be built anywhere. Assisted-by: Claude Code:claude-opus-5-5 * Skip test_large_content_length_truncated on macOS The test sets the CGI handler timeout to 1ms. That timeout also bounds reading the request line, so when the client has not sent the request within 1ms the server logs "Request timed out" and closes the connection. The client then gets ConnectionResetError or BrokenPipeError. This reproduces on macOS with both RustPython and CPython 3.14.6. The skip was removed in #8759 after Linux-only repeats. Assisted-by: Claude Code:claude-opus-5-5

qingshi163 force-pushed the main branch from cc9cec1 to 6206aae Compare June 29, 2022 15:37

qingshi163 requested a review from youknowone June 29, 2022 15:38

qingshi163 marked this pull request as ready for review June 29, 2022 15:38

youknowone reviewed Jun 29, 2022

View reviewed changes

qingshi163 force-pushed the main branch from 6206aae to c42e170 Compare August 6, 2022 20:26

qingshi163 requested a review from youknowone August 7, 2022 14:22

youknowone requested a review from coolreader18 August 28, 2022 16:39

youknowone reviewed Aug 28, 2022

View reviewed changes

Comment thread vm/src/builtins/type.rs

}

impl PyTypeRef {

impl Py<PyType> {

youknowone Aug 28, 2022

Copy link
Copy Markdown

Member

Choose a reason for hiding this comment

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

This is a great idea.

youknowone previously approved these changes Aug 28, 2022

View reviewed changes

qingshi163 force-pushed the main branch from c42e170 to 3421c31 Compare August 28, 2022 18:08

youknowone reviewed Aug 29, 2022

View reviewed changes

coolreader18 reviewed Aug 29, 2022

View reviewed changes

qingshi163 force-pushed the main branch from c37d2af to 4fe77b7 Compare August 29, 2022 19:38

qingshi163 requested review from coolreader18 and youknowone August 31, 2022 18:54

youknowone approved these changes Oct 15, 2022

View reviewed changes

qingshi163 added 3 commits October 15, 2022 17:46

introduce PyAtomicRef

5e63320

refactor PyObject.typ use PyAtomicRef

6ca2c54

add documents

5b93594

qingshi163 added 5 commits October 15, 2022 17:47

add swap_to_temporary_refs

e8e783b

replace AtomicCell with PyAtomic

ec42fc8

fixup

b67e033

fix name conflict

65d5d59

rebase and fixup

cda592f

qingshi163 force-pushed the main branch from ce67046 to cda592f Compare October 15, 2022 19:20

qingshi163 merged commit 30aeb8b into RustPython:main Oct 15, 2022

qingshi163 deleted the main branch October 15, 2022 19:53

qingshi163 restored the main branch October 15, 2022 19:53

youknowone mentioned this pull request Oct 23, 2022

bug: Double Free On BufferedReader&FileIO? #4238

Closed

qingshi163 deleted the main branch November 13, 2022 08:12

youknowone mentioned this pull request Sep 25, 2026

Remove unused PyLease; skip flaky httpservers test on macOS #8821

Merged

	pub temperary_refs: PyMutex<Vec<PyObjectRef>>,
	pub temporary_refs: PyMutex<Vec<PyObjectRef>>,

	let old = self.0.swap(NonNull::from(py));
	let old = self.0.swap(NonNull::new_unchecked(py));

Conversation

qingshi163 commented Jun 29, 2022

Uh oh!

fanninpm commented Jun 29, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qingshi163 commented Aug 7, 2022

Uh oh!

qingshi163 commented Aug 8, 2022

Uh oh!

youknowone commented Aug 22, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

youknowone left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

youknowone Aug 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coolreader18 commented Aug 29, 2022

Uh oh!

qingshi163 commented Aug 29, 2022

Uh oh!

coolreader18 commented Aug 29, 2022

Uh oh!

qingshi163 commented Aug 29, 2022

Uh oh!

qingshi163 commented Aug 31, 2022

Uh oh!

qingshi163 commented Oct 2, 2022

Uh oh!

youknowone left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

youknowone Aug 29, 2022 •

edited

Loading