Implement 128bit integer conversion for limited API #1310

kngwyu · 2020-12-12T08:05:49Z

Fix #1309
They would be slow since they use eval... but could there be any alternative?

birkenfeld · 2020-12-12T08:09:42Z

Why use eval when you can call the method directly with call_method?

birkenfeld · 2020-12-12T08:12:17Z

This is the snippet I tested:

        let bytes = obj.call_method("to_bytes", (16, "little"), None)?;
        let res = u128::from_le_bytes(bytes.extract()?);

Or do you think this is slower due to conversion of the arguments from Rust? Hm, you need to construct a whole dictionary for the eval...

Of course, both are missing the PyNumber_Index call that the other conversion is doing.

birkenfeld · 2020-12-12T08:13:29Z

Alternative, as I said in #1309, is to use PyNumber_ APIs to shift and mask the number to get two values that fit into u64's.

kngwyu · 2020-12-12T08:21:39Z

Why use eval when you can call the method directly with call_method?

Thanks, I just failed to come up with it. We can call PyNumber_Index manually.

to use PyNumber_ APIs to shift and mask the number to get two values that fit into u64's.

I tried that first but I found it too complex, especially for i128, though it is faster...

birkenfeld · 2020-12-12T08:29:12Z

So in my tests (FromPyObject only) it was slightly faster to call with call_method, but then use your method of downcast instead of extract.

kngwyu · 2020-12-12T08:59:07Z

src/types/num.rs

@@ -183,11 +173,12 @@ mod int128_conversion {
            impl IntoPy<PyObject> for $rust_type {
                fn into_py(self, py: Python) -> PyObject {
                    unsafe {
-                        let bytes = self.to_ne_bytes();
+                        // Always use little endian
+                        let bytes = self.to_le_bytes();


Though it's not related to this PR, I noticed that to_le_bytes is added in Rust 1.32.
Using always little-endian simplified the code a lot.

kngwyu · 2020-12-12T08:59:32Z

src/types/num.rs

                        let obj = ffi::_PyLong_FromByteArray(
-                            bytes.as_ptr() as *const c_uchar,
-                            $byte_size,


I don't know why 16 is not hard-coded...

maybe bytes.len() is most correct? I would think that the compiler will still optimize this at compile time.

davidhewitt

Nice!

I think it's better to use std::mem::size_of{_val} instead of hard-coding 16, as it's easier to understand what that means for a future reader rather than just seeing a magic number 16.

src/types/num.rs

davidhewitt · 2020-12-12T09:04:49Z

src/types/num.rs

                        let obj = ffi::_PyLong_FromByteArray(
-                            bytes.as_ptr() as *const c_uchar,
-                            $byte_size,


maybe bytes.len() is most correct? I would think that the compiler will still optimize this at compile time.

davidhewitt · 2020-12-12T09:08:15Z

src/types/num.rs

@@ -202,83 +193,131 @@ mod int128_conversion {
                        if num.is_null() {
                            return Err(PyErr::fetch(ob.py()));
                        }
-                        let mut buffer = [0; $byte_size];
+                        let mut buffer = [0; 16];


Suggested change

let mut buffer = [0; 16];

let mut buffer = [0; std::mem::size_of::<$rust_type>()];

davidhewitt · 2020-12-12T09:09:33Z

src/types/num.rs

                        let ok = ffi::_PyLong_AsByteArray(
                            num as *mut ffi::PyLongObject,
                            buffer.as_mut_ptr(),
-                            $byte_size,
-                            IS_LITTLE_ENDIAN,
+                            16,


Suggested change

16,

buffer.len(),

davidhewitt · 2020-12-12T09:13:09Z

src/types/num.rs

+                    let num = unsafe { ffi::PyNumber_Index(ob.as_ptr()) };
+                    let num: &PyLong =
+                        unsafe { crate::FromPyPointer::from_owned_ptr_or_err(py, num)? };
+                    let args = (16, "little".to_object(py));


Suggested change

let args = (16, "little".to_object(py));

let args = (std::mem::size_of::<$rust_type>(), "little".to_object(py));

or could define a const SIZE: usize = std::mem::size_of::<$rust_type>() and use here and on line 258.

birkenfeld · 2020-12-12T10:02:48Z

src/types/num.rs

+                            ffi::PyNumber_Index(ob.as_ptr()),
+                        )
+                    }?;
+                    let kwargs = [("signed", $is_signed)].into_py_dict(py);


ah damn, it requires a dict here too then. Can you at least make this

let kwargs = if $is_signed { Some(...) } else { None };

?

CHANGELOG.md

Co-authored-by: David Hewitt <1939362+davidhewitt@users.noreply.github.com>

kngwyu · 2020-12-12T10:39:22Z

Thank you for your reviews!

davidhewitt

💯

kngwyu mentioned this pull request Dec 12, 2020

abi3 causes compile failures where u128s are involved #1309

Closed

kngwyu force-pushed the abi3-128bit-integer branch from 09423d0 to 7f814f7 Compare December 12, 2020 08:57

kngwyu commented Dec 12, 2020

View reviewed changes

davidhewitt reviewed Dec 12, 2020

View reviewed changes

kngwyu force-pushed the abi3-128bit-integer branch from 7f814f7 to 0cafe3d Compare December 12, 2020 09:54

birkenfeld reviewed Dec 12, 2020

View reviewed changes

Implement 128bit integer conversion for limited API

8c4cba2

kngwyu force-pushed the abi3-128bit-integer branch from 0cafe3d to 8c4cba2 Compare December 12, 2020 10:14

davidhewitt reviewed Dec 12, 2020

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

Update CHANGELOG.md

7a0542d

Co-authored-by: David Hewitt <1939362+davidhewitt@users.noreply.github.com>

davidhewitt approved these changes Dec 12, 2020

View reviewed changes

kngwyu merged commit 22359e9 into master Dec 12, 2020

programmerjake mentioned this pull request Dec 13, 2020

Use shifting and masking to convert between 128-bit Rust integers and Python integers #1314

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement 128bit integer conversion for limited API #1310

Implement 128bit integer conversion for limited API #1310

kngwyu commented Dec 12, 2020

birkenfeld commented Dec 12, 2020

birkenfeld commented Dec 12, 2020 •

edited

birkenfeld commented Dec 12, 2020

kngwyu commented Dec 12, 2020 •

edited

birkenfeld commented Dec 12, 2020

kngwyu Dec 12, 2020

kngwyu Dec 12, 2020

davidhewitt Dec 12, 2020

davidhewitt left a comment

davidhewitt Dec 12, 2020

davidhewitt Dec 12, 2020

davidhewitt Dec 12, 2020

davidhewitt Dec 12, 2020

birkenfeld Dec 12, 2020

kngwyu Dec 12, 2020

kngwyu commented Dec 12, 2020

davidhewitt left a comment

	let mut buffer = [0; 16];
	let mut buffer = [0; std::mem::size_of::<$rust_type>()];

	let args = (16, "little".to_object(py));
	let args = (std::mem::size_of::<$rust_type>(), "little".to_object(py));

Implement 128bit integer conversion for limited API #1310

Implement 128bit integer conversion for limited API #1310

Conversation

kngwyu commented Dec 12, 2020

birkenfeld commented Dec 12, 2020

birkenfeld commented Dec 12, 2020 • edited

birkenfeld commented Dec 12, 2020

kngwyu commented Dec 12, 2020 • edited

birkenfeld commented Dec 12, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidhewitt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kngwyu commented Dec 12, 2020

davidhewitt left a comment

Choose a reason for hiding this comment

birkenfeld commented Dec 12, 2020 •

edited

kngwyu commented Dec 12, 2020 •

edited