Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Object protocols without specialization #961

Merged
merged 14 commits into from
Jun 18, 2020
Merged

Conversation

kngwyu
Copy link
Member

@kngwyu kngwyu commented Jun 6, 2020

For #210.
Now all pyclass has a method registory for protocols:

impl pyo3::class::proto_methods::HasProtoRegistry for MyClass {
    fn registory() -> &'static pyo3::class::proto_methods::PyProtoRegistry {
        static REGISTRY: PyProtoRegistry = PyProtoRegistry::new();
        &REGISTRY
    }
}

, where PyProtoRegistory is a set of method tables:

#[doc(hidden)]
pub struct PyProtoRegistry {
    async_methods: AtomicPtr<PyAsyncMethods>,
    basic_methods: AtomicPtr<PyObjectMethods>,
    ...
}

.

Using the ctor crate, we insert some initialization codes for this registry.
For example, when we write this code:

#[pyproto]
impl PyObjectProtocol for MyClass {
    fn __hash__(&self) -> PyResult<isize> {
        Ok(self.val as isize)
    }
}

, (roughly) the following code is generated:

#[ctor]
fn __init_Object_2817873() {
    let mut table = pyo3::class::PyObjectMethods::default();
    table.set_hash::<MyClass>();
    <MyClass as pyo3::class::proto_methods::HasProtoRegistry>::registory().set_basic_methods(table);
}

One disadvantage of this solution is it needs redundant Box::new for some protocols(basic, iter, and so on). However, to keep PyProtoRegistry simple and small, I chose this design.

Unfortunately, some protocols are not yet tested.

TODO

  • Add tests for async protocols
    • Maybe we need to change the trait definition to make it work?
  • Add tests for descr protocols
  • Maybe need some more doc comments?

@kngwyu
Copy link
Member Author

kngwyu commented Jun 6, 2020

I forgot to mention that this PR has a breaking change. nb_bool is now moved under PyNumberProtocol from PyObjectProtocol.

Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

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

Thanks so much for this - it's excellent to see that this code is shorter than what was there before and should unblock us for compiling pyo3 on stable! 🎉

There's a couple things I'd like to see changed:

I forgot to mention that this PR has a breaking change. nb_bool is now moved under PyNumberProtocol from PyObjectProtocol.

I understand why you moved this, but I am not sure if it is the right change from a user design.

The related C API is listed in the Object Protocol: https://docs.python.org/3/c-api/object.html?highlight=pyobject_istrue

Does it make the implementation much more complicated if we can keep this in the PyObjectProtocol?

pyo3-derive-backend/src/pyclass.rs Outdated Show resolved Hide resolved
src/class/proto_methods.rs Outdated Show resolved Hide resolved
src/class/proto_methods.rs Show resolved Hide resolved
Comment on lines 117 to 118
self.async_methods
.store(Box::into_raw(Box::new(methods)), Ordering::SeqCst)
Copy link
Member

Choose a reason for hiding this comment

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

Technically all of these could leak memory if called for a second time.

Copy link
Member Author

@kngwyu kngwyu Jun 7, 2020

Choose a reason for hiding this comment

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

Yes, but the current code does the same. See https://github.com/PyO3/pyo3/blob/v0.10.1/src/pyclass.rs#L151.
The difference is now we cause memory leak for all protocols(e.g., iter and basic).
We can avoid this by changing the pub struct PyProtoRegistry like the below:

pub struct PyProtoRegistry {
    /// Protocols that needs specific table types
    async_methods: AtomicPtr<ffi::PyAsyncMethods>,
    buffer_methods: AtomicPtr<ffi::PyBufferProcs>,
    mapping_methods: AtomicPtr<ffi::PyMappingMethods>,
    number_methods: AtomicPtr<ffi::PyNumberMethods>,
    sequence_methods: AtomicPtr<ffi::PySequenceMethods>,
    /// Other methods.
    /// AtomicUsize is used instead of `Atomic<Option<ffi::func>>` or so because
    /// we can't use function pointer in const function.
    tp_str: AtomicUsize,
    tp_repr: AtomicUsize.
    ....
}

.

I chose this design first but abandoned it due to its ugliness.

Copy link
Member

Choose a reason for hiding this comment

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

Cool, I think your design choice is fine. Can we maybe just add a docstring for now to warn calling these twice will leak?

src/class/pyasync.rs Show resolved Hide resolved
src/instance.rs Outdated Show resolved Hide resolved
src/pycell.rs Outdated Show resolved Hide resolved
@kngwyu
Copy link
Member Author

kngwyu commented Jun 7, 2020

Thank you for the review!

Let's fix the Send + Sync bounds in #948 rather than adding in this PR.

I accidentally pushed these changes when I was playing with them. I'll revert them.

Does it make the implementation much more complicated if we can keep this in the PyObjectProtocol?

I don't have any good solution now, unfortunately.

@davidhewitt
Copy link
Member

I don't have any good solution now, unfortunately.

👍 ok. I have a half-baked idea but it's hard to write down as I haven't properly thought about it yet. I'll try to find time tomorrow evening to experiment with it whether it can help and explain it better then.

@kngwyu
Copy link
Member Author

kngwyu commented Jun 8, 2020

I have a half-baked idea but it's hard to write down as I haven't properly thought about it yet.

Thank you but wait for a bit...
I don't think most users know that __bool__ is classified as a part of Object protocols.
Thus I don't think it's worth doing if we need more codes to do so.

EDITED:
I just tried to move nb_bool under ObjectProtocol again but the resulting code is not so smart 🤔

    // nb_bool is a part of PyObjectProtocol, but should be placed under tp_as_number
    let mut nb_bool = None;
    // basic methods
    if let Some(basic) = T::basic_methods() {
        unsafe { basic.as_ref() }.update_typeobj(type_object);
        nb_bool = unsafe { basic.as_ref() }.nb_bool;
    }

    // number methods
    type_object.tp_as_number = T::number_methods()
        .map(|mut p| {
            unsafe { p.as_mut() }.nb_bool = nb_bool;
            p.as_ptr()
        })
        .unwrap_or_else(|| {
            if nb_bool.is_some() {
                let mut number_methods = ffi::PyNumberMethods::default();
                number_methods.nb_bool = nb_bool;
                Box::into_raw(Box::new(number_methods))
            } else {
                ptr::null_mut()
            }
        });

Still not sure we should do this...

@kngwyu
Copy link
Member Author

kngwyu commented Jun 8, 2020

At last, I decided to move nb_bool under PyObjectProtocol again.
I don't think the code is very ugly but please check it.

Comment on lines 130 to 131
// E.g., if we have `__add__`, we need to skip `set_radd`.
let mut skipped_setters = HashSet::new();
Copy link
Member

Choose a reason for hiding this comment

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

Do you think that we still need skipped_setters once we implement a solution for #844?

pyo3-derive-backend/src/pyproto.rs Outdated Show resolved Hide resolved
src/class/pyasync.rs Show resolved Hide resolved
Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

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

At last, I decided to move nb_bool under PyObjectProtocol again.
I don't think the code is very ugly but please check it.

I think what you've done is nice. It's good to avoid the unnecessary breaking change 😄

This branch is looking good, I'm very excited by it! As well as your TODO for docs and tests I would be interested to hear what you think about how we could solve #844 with this new implementation.

src/class/macros.rs Show resolved Hide resolved
src/pyclass.rs Show resolved Hide resolved
@davidhewitt
Copy link
Member

It would also be good to add a test for #940 in this PR - if I read your implementation correctly I think that should already be solved by what you've done!

@konstin konstin mentioned this pull request Jun 9, 2020
@kngwyu kngwyu force-pushed the slot-provider branch 2 times, most recently from 1d6295c to ed31c70 Compare June 13, 2020 10:42
@kngwyu
Copy link
Member Author

kngwyu commented Jun 13, 2020

Added a test for __await__.
I was really confused by Python's async/await and noticed some things:

  • Returning Future from Rust is hard.
  • To return some value, we have to make StopIterException have the value.

But for now, I think this is sufficient for testing proc-macros.

@kngwyu
Copy link
Member Author

kngwyu commented Jun 13, 2020

OMG...
image
(Windows + Python3.8)

tests/test_dunder.rs Show resolved Hide resolved
tests/test_dunder.rs Outdated Show resolved Hide resolved
tests/test_dunder.rs Outdated Show resolved Hide resolved
@madhavajay
Copy link

🍿🤓

@kngwyu
Copy link
Member Author

kngwyu commented Jun 15, 2020

I would be interested to hear what you think about how we could solve #844 with this new implementation.

I think we can reuse the same trick as __set/delitem__ for that, but maybe it should be done in another PR 🤔

@davidhewitt
Copy link
Member

I think we can reuse the same trick as set/delitem for that, but maybe it should be done in another PR 🤔

I'm happy with that - this PR is already quite large 😄

@kngwyu
Copy link
Member Author

kngwyu commented Jun 17, 2020

Now I think this PR is ready.
Though I added more comments than usual, please point out what you think is unclear.

Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

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

This is looking fantastic to me! +1,373 / -2,245 - nice job 🚀

@@ -76,7 +82,7 @@ fn impl_proto_impl(
} else {
quote!(0)
};
// TODO(kngwyu): doc
// TODO(kngwyu): Set ml_doc
Copy link
Member

Choose a reason for hiding this comment

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

Let's create an issue for this so that we remember to do it?

@konstin
Copy link
Member

konstin commented Jun 17, 2020

The test_await was failing for me locally (python3.6 from apt in virtualenv), this is the workaround I used to make them pass.

Traceback (most recent call last):
  File "<string>", line 12, in <module>
  File "/usr/lib/python3.6/asyncio/events.py", line 694, in get_event_loop
    return get_event_loop_policy().get_event_loop()
  File "/usr/lib/python3.6/asyncio/events.py", line 602, in get_event_loop
    % threading.current_thread().name)
RuntimeError: There is no current event loop in thread 'Dummy-1'.
test test_await ... FAILED
diff --git a/tests/test_dunder.rs b/tests/test_dunder.rs
index 67a66fb49..47f22fc91 100644
--- a/tests/test_dunder.rs
+++ b/tests/test_dunder.rs
@@ -612,7 +612,11 @@ async def main():
 # but we see still errors on Github actions...
 if sys.platform == "win32" and sys.version_info >= (3, 8, 0):
     asyncio.set_event_loop_policy(asyncio.WindowsSelectorEventLoopPolicy())
-loop = asyncio.get_event_loop()
+try:
+    loop = asyncio.get_event_loop()
+except RuntimeError: # RuntimeError: There is no current event loop in thread 'Dummy-1'.
+    loop = asyncio.new_event_loop()
+    asyncio.set_event_loop(loop)
 assert loop.run_until_complete(main()) is None
 loop.close()
 "#

@kngwyu
Copy link
Member Author

kngwyu commented Jun 17, 2020

@konstin
Thanks, I pushed the change based on your diff.

@kngwyu kngwyu merged commit 390ff5f into PyO3:master Jun 18, 2020
@kngwyu kngwyu deleted the slot-provider branch June 18, 2020 08:19
@davidhewitt
Copy link
Member

🎉

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.

4 participants