Patchwork [01,of,10] rust-cpython: move py_shared_state to PySharedRefCell object

login
register
mail settings
Submitter Yuya Nishihara
Date Sept. 22, 2019, 6:41 a.m.
Message ID <e7f643679476b013e86b.1569134498@mimosa>
Download mbox | patch
Permalink /patch/41720/
State Accepted
Headers show

Comments

Yuya Nishihara - Sept. 22, 2019, 6:41 a.m.
# HG changeset patch
# User Yuya Nishihara <yuya@tcha.org>
# Date 1568469711 -32400
#      Sat Sep 14 23:01:51 2019 +0900
# Node ID e7f643679476b013e86b9a2b75f704d64eec4293
# Parent  7a01778bc7b717ae12ae6b73e9047a4d06eeba61
rust-cpython: move py_shared_state to PySharedRefCell object

The goal of this series is to encapsulate more "py_shared" thingy and
reduce the size of the macro, which is hard to debug.

Since py_shared_state manages the borrowing state of the object owned by
PySharedRefCell, this change makes more sense. If a PyObject has more than
one data to be leaked into Python world, each PySharedState should incref
the parent PyObject, and keep track of the corresponding borrowing state.
Pulkit Goyal - Oct. 11, 2019, 1:13 p.m.
On Sun, Sep 22, 2019 at 9:51 AM Yuya Nishihara <yuya@tcha.org> wrote:
>
> # HG changeset patch
> # User Yuya Nishihara <yuya@tcha.org>
> # Date 1568469711 -32400
> #      Sat Sep 14 23:01:51 2019 +0900
> # Node ID e7f643679476b013e86b9a2b75f704d64eec4293
> # Parent  7a01778bc7b717ae12ae6b73e9047a4d06eeba61
> rust-cpython: move py_shared_state to PySharedRefCell object
>
> The goal of this series is to encapsulate more "py_shared" thingy and
> reduce the size of the macro, which is hard to debug.
>
> Since py_shared_state manages the borrowing state of the object owned by
> PySharedRefCell, this change makes more sense. If a PyObject has more than
> one data to be leaked into Python world, each PySharedState should incref
> the parent PyObject, and keep track of the corresponding borrowing state.

Queued the series as per Raphaël review. Many thanks!
Raphaël Gomès - Oct. 11, 2019, 1:24 p.m.
Just for the record, "the series" means patches 1-8, as 9 and 10 are unsafe.

On 10/11/19 3:13 PM, Pulkit Goyal wrote:
> On Sun, Sep 22, 2019 at 9:51 AM Yuya Nishihara <yuya@tcha.org> wrote:
>> # HG changeset patch
>> # User Yuya Nishihara <yuya@tcha.org>
>> # Date 1568469711 -32400
>> #      Sat Sep 14 23:01:51 2019 +0900
>> # Node ID e7f643679476b013e86b9a2b75f704d64eec4293
>> # Parent  7a01778bc7b717ae12ae6b73e9047a4d06eeba61
>> rust-cpython: move py_shared_state to PySharedRefCell object
>>
>> The goal of this series is to encapsulate more "py_shared" thingy and
>> reduce the size of the macro, which is hard to debug.
>>
>> Since py_shared_state manages the borrowing state of the object owned by
>> PySharedRefCell, this change makes more sense. If a PyObject has more than
>> one data to be leaked into Python world, each PySharedState should incref
>> the parent PyObject, and keep track of the corresponding borrowing state.
> Queued the series as per Raphaël review. Many thanks!
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

Patch

diff --git a/rust/hg-cpython/src/dirstate/dirs_multiset.rs b/rust/hg-cpython/src/dirstate/dirs_multiset.rs
--- a/rust/hg-cpython/src/dirstate/dirs_multiset.rs
+++ b/rust/hg-cpython/src/dirstate/dirs_multiset.rs
@@ -16,10 +16,7 @@  use cpython::{
     Python,
 };
 
-use crate::{
-    dirstate::extract_dirstate,
-    ref_sharing::{PySharedRefCell, PySharedState},
-};
+use crate::{dirstate::extract_dirstate, ref_sharing::PySharedRefCell};
 use hg::{
     utils::hg_path::{HgPath, HgPathBuf},
     DirsMultiset, DirsMultisetIter, DirstateMapError, DirstateParseError,
@@ -28,7 +25,6 @@  use hg::{
 
 py_class!(pub class Dirs |py| {
     data inner: PySharedRefCell<DirsMultiset>;
-    data py_shared_state: PySharedState;
 
     // `map` is either a `dict` or a flat iterator (usually a `set`, sometimes
     // a `list`)
@@ -65,7 +61,6 @@  py_class!(pub class Dirs |py| {
         Self::create_instance(
             py,
             PySharedRefCell::new(inner),
-            PySharedState::default()
         )
     }
 
@@ -115,11 +110,7 @@  py_shared_ref!(Dirs, DirsMultiset, inner
 
 impl Dirs {
     pub fn from_inner(py: Python, d: DirsMultiset) -> PyResult<Self> {
-        Self::create_instance(
-            py,
-            PySharedRefCell::new(d),
-            PySharedState::default(),
-        )
+        Self::create_instance(py, PySharedRefCell::new(d))
     }
 
     fn translate_key(
diff --git a/rust/hg-cpython/src/dirstate/dirstate_map.rs b/rust/hg-cpython/src/dirstate/dirstate_map.rs
--- a/rust/hg-cpython/src/dirstate/dirstate_map.rs
+++ b/rust/hg-cpython/src/dirstate/dirstate_map.rs
@@ -21,7 +21,7 @@  use libc::c_char;
 use crate::{
     dirstate::copymap::{CopyMap, CopyMapItemsIterator, CopyMapKeysIterator},
     dirstate::{decapsule_make_dirstate_tuple, dirs_multiset::Dirs},
-    ref_sharing::{PySharedRefCell, PySharedState},
+    ref_sharing::PySharedRefCell,
 };
 use hg::{
     utils::hg_path::{HgPath, HgPathBuf},
@@ -44,14 +44,12 @@  use hg::{
 //     leaks, with all methods that go along for reference sharing.
 py_class!(pub class DirstateMap |py| {
     data inner: PySharedRefCell<RustDirstateMap>;
-    data py_shared_state: PySharedState;
 
     def __new__(_cls, _root: PyObject) -> PyResult<Self> {
         let inner = RustDirstateMap::default();
         Self::create_instance(
             py,
             PySharedRefCell::new(inner),
-            PySharedState::default()
         )
     }
 
diff --git a/rust/hg-cpython/src/ref_sharing.rs b/rust/hg-cpython/src/ref_sharing.rs
--- a/rust/hg-cpython/src/ref_sharing.rs
+++ b/rust/hg-cpython/src/ref_sharing.rs
@@ -12,7 +12,7 @@  use cpython::{PyResult, Python};
 use std::cell::{Cell, Ref, RefCell, RefMut};
 
 /// Manages the shared state between Python and Rust
-#[derive(Default)]
+#[derive(Debug, Default)]
 pub struct PySharedState {
     leak_count: Cell<usize>,
     mutably_borrowed: Cell<bool>,
@@ -103,12 +103,14 @@  impl PySharedState {
 #[derive(Debug)]
 pub struct PySharedRefCell<T> {
     inner: RefCell<T>,
+    pub py_shared_state: PySharedState, // TODO: remove pub
 }
 
 impl<T> PySharedRefCell<T> {
-    pub const fn new(value: T) -> PySharedRefCell<T> {
+    pub fn new(value: T) -> PySharedRefCell<T> {
         Self {
             inner: RefCell::new(value),
+            py_shared_state: PySharedState::default(),
         }
     }
 
@@ -178,12 +180,6 @@  impl<'a, T> Drop for PyRefMut<'a, T> {
 ///
 /// # Warning
 ///
-/// The targeted `py_class!` needs to have the
-/// `data py_shared_state: PySharedState;` data attribute to compile.
-/// A better, more complicated macro is needed to automatically insert it,
-/// but this one is not yet really battle tested (what happens when
-/// multiple references are needed?). See the example below.
-///
 /// TODO allow Python container types: for now, integration with the garbage
 ///     collector does not extend to Rust structs holding references to Python
 ///     objects. Should the need surface, `__traverse__` and `__clear__` will
@@ -208,7 +204,6 @@  impl<'a, T> Drop for PyRefMut<'a, T> {
 ///
 /// py_class!(pub class MyType |py| {
 ///     data inner: PySharedRefCell<MyStruct>;
-///     data py_shared_state: PySharedState;
 /// });
 ///
 /// py_shared_ref!(MyType, MyStruct, inner, MyTypeLeakedRef);
@@ -229,7 +224,7 @@  macro_rules! py_shared_ref {
                 // assert $data_member type
                 use crate::ref_sharing::PySharedRefCell;
                 let data: &PySharedRefCell<_> = self.$data_member(py);
-                self.py_shared_state(py)
+                data.py_shared_state
                     .borrow_mut(py, unsafe { data.borrow_mut() })
             }
 
@@ -248,7 +243,7 @@  macro_rules! py_shared_ref {
                 use crate::ref_sharing::PySharedRefCell;
                 let data: &PySharedRefCell<_> = self.$data_member(py);
                 let static_ref =
-                    self.py_shared_state(py).leak_immutable(py, data)?;
+                    data.py_shared_state.leak_immutable(py, data)?;
                 let leak_handle = $leaked::new(py, self);
                 Ok((leak_handle, static_ref))
             }
@@ -277,7 +272,7 @@  macro_rules! py_shared_ref {
             fn drop(&mut self) {
                 let gil = Python::acquire_gil();
                 let py = gil.python();
-                let state = self.inner.py_shared_state(py);
+                let state = &self.inner.$data_member(py).py_shared_state;
                 unsafe {
                     state.decrease_leak_count(py, false);
                 }
@@ -309,7 +304,6 @@  macro_rules! py_shared_ref {
 ///
 /// py_class!(pub class MyType |py| {
 ///     data inner: PySharedRefCell<MyStruct>;
-///     data py_shared_state: PySharedState;
 ///
 ///     def __iter__(&self) -> PyResult<MyTypeItemsIterator> {
 ///         let (leak_handle, leaked_ref) = unsafe { self.leak_immutable(py)? };