Faster conversion from u128/i128 to PyLong with abi3 #1315

kngwyu · 2020-12-13T08:33:04Z

Fixes #1314
I hope it's correct...

Also, adding 30 lines is acceptable for such a minor feature?

kngwyu · 2020-12-13T08:47:01Z

Now I understand why I thought this approach is too complex: I was thinking of using lshift and add, but lshift and or are better.

birkenfeld · 2020-12-13T08:56:39Z

src/types/num.rs

                    let pybytes: &PyBytes = num
-                        .call_method("to_bytes", (bytes.len(), "little"), kwargs(py, $is_signed))?
+                        .call_method("to_bytes", (bytes.len(), "little"), kwargs)?


this seems to revert a refactoring, or did you want to change the FromPy part as well?

Since now the kwargs function is called once, I moved it in the function.

davidhewitt

Thanks. It's 14 more lines of code total for a more efficient solution so I think this is a desirable change. I expect abi3 builds to become quite common once we have them working.

I think it'd be quite good to have a test here to be sure that we've got the right solution without any edge cases.

proptest can be pretty nice for tests like this where data is passed in a roundtrip. I think we can have a test like this, for example:

use proptest::prelude::*;

proptest! {
    #[test]
    fn test_i128_roundtrip(x: i128) {
        Python::with_gil(|py| {
            let x_py = x.into_py(py);
            py_run!(py, x_py, format!("assert x_py == {}", x));
            let roundtripped: i128 = x_py.extract().unwrap();
            assert_eq!(x, roundtripped);
        })
    }
}

and similar for u128.

davidhewitt · 2020-12-13T09:30:27Z

src/types/num.rs

-                        .call_method("from_bytes", (bytes, "little"), kwargs(py, $is_signed))
-                        .expect("Integer conversion (u128/i128 to PyLong) failed")
-                        .into_py(py)
+                    let (first, last) = split_128int(self.to_le_bytes());


Can I suggest we use the terminology lower / upper instead of first / last? For me it's easier to understand which bits are most significant / least significant with that terminology.

Suggested change

let (first, last) = split_128int(self.to_le_bytes());

let (lower, upper) = split_128int(self.to_le_bytes());

Thanks, I wanted better names too 😉

kngwyu · 2020-12-13T11:35:59Z

Just a status update: renamed some variables and re-implemented PyLong to i128/u128 conversion using PyLong_AsUnsignedLongLongMask and shift.
Proptest looks good but I'm still reading the document.

davidhewitt

Nice! Couple more suggestions...

davidhewitt · 2020-12-13T11:58:41Z

src/types/num.rs

+                    let le_bytes = self.to_le_bytes();
+                    let lower = u64::from_le_bytes(slice_to_bytearr(&le_bytes[..BYTE_SIZE / 2]));
+                    let upper =
+                        <$half_type>::from_le_bytes(slice_to_bytearr(&le_bytes[BYTE_SIZE / 2..]));


A couple of ideas:

might want to use split_at ?

using TryInto + unwrap() is an option here, the compiler can statically verify the slice size so will optimize the panic away:

Suggested change

let le_bytes = self.to_le_bytes();

let lower = u64::from_le_bytes(slice_to_bytearr(&le_bytes[..BYTE_SIZE / 2]));

let upper =

<$half_type>::from_le_bytes(slice_to_bytearr(&le_bytes[BYTE_SIZE / 2..]));

use std::convert::TryInto;

let le_bytes = self.to_le_bytes();

let (lower, upper) = le_bytes.split_at(BYTE_SIZE / 2);

let lower = u64::from_le_bytes(lower.try_into().unwrap());

let upper = <$half_type>::from_le_bytes(upper.try_into().unwrap());

Looks nice 👍🏽

davidhewitt · 2020-12-13T12:01:50Z

src/types/num.rs

                            py,
-                            ffi::PyNumber_Index(ob.as_ptr()),


I think the call to PyNumber_Index is still needed before calling PyLong_AsUnsignedLongLongMask, so that types which implement __index__ are converted to PyLong?

No, PyLong_AsUnsigned... calls it internally.

Ah perfect, thanks for checking!

kngwyu · 2020-12-13T12:59:34Z

Added an overflow test for i128.
And for proptest, I think in this case max/min is sufficiently strong and we don't strongly need randomized tests.

birkenfeld · 2020-12-13T13:19:51Z

There should also be a test with values in the i/u64 range, to ensure that the two halves are not mixed up.

davidhewitt · 2020-12-13T13:51:14Z

Agreed; if we're not proptesting we should make sure the tests we do have cover for mistakes like halves being mixed up. We know it's correct now but who knows what will happen next time I try to refactor this code 👀

programmerjake · 2020-12-13T13:56:54Z

Why are you using all the le bytes methods when you can just use:

let v: u128 = ...;
let low_half = v as u64;
let high_half = (v >> 64) as u64;

and

let v: i128 = ...;
let low_half = v as u64; // note unsigned
let high_half = (v >> 64) as i64;

and then on the python side:

(high_half << 64) | low_half

programmerjake · 2020-12-13T14:09:03Z

Good test values are:

let u128_test_value = 0xFF0102030405060788090A0B0C0D0E0Fu128;
let i128_test_value = u128_test_value as i128;

since it tests the halves are in the right order with the right signedness.

davidhewitt · 2020-12-13T14:10:00Z

Why are you using all the le bytes methods when you can just use:

😅 great suggestion, thanks! I think when this implementation started with to_bytes we got stuck on one idea too much!

kngwyu · 2020-12-13T14:46:55Z

Why are you using all the le bytes methods when you can just use:

Wow, thanks. It seems like I wrote too many TeX documents to forget some bit hacks 🙄

kngwyu · 2020-12-13T14:54:30Z

Added a basic proptest but is this sufficient?

programmerjake · 2020-12-13T15:02:31Z

src/types/num.rs

-                    bytes.copy_from_slice(pybytes.as_bytes());
-                    Ok(<$rust_type>::from_le_bytes(bytes))
+                            -1 as _,
+                            ffi::PyLong_AsUnsignedLongLongMask(ob.as_ptr()),


I think you'd probably want an assertion of some sort that PyLong_UnsignedLongLongMask returns the expected type, since C/C++ don't guarantee that it's u64 -- it could be u128 or some other type on an unusual OS.

Hmm 🤔 , but even the Rust stdlib does not support it https://doc.rust-lang.org/nightly/std/os/raw/type.c_ulonglong.html.

The docs are just showing the type used on the system they used to build the docs (x86_64-unknown-linux-gnu iirc), it doesn't mean it will always be u64.

For an example, see https://doc.rust-lang.org/nightly/std/os/raw/type.c_long.html

I meant the description:

The C standard technically only requires that this type be an unsigned integer with the size of a long long, although in practice, no system would have a long long that is not a u64, as most systems do not have a standardised u128 type.

Also, it has no #[cfg block:

So I think we don't need to consider the system where ull is u128 for now

ok, yeah. I did check both the AS/400 and RISC-V 128-bit compilers (or at least their ABI proposals/docs) and they both have a 64-bit ull -- AS/400 just doesn't have a 128-bit int type (no intptr_t at all), and RV128 proposes long long long for 128-bit types.

programmerjake · 2020-12-13T15:07:16Z

You accidentally changed src/lib.rs to an executable file.

programmerjake · 2020-12-13T15:07:51Z

Btw, sorry to sound all negative, thanks for all your work!

programmerjake · 2020-12-13T15:13:03Z

other than all that, looks good to me!

kngwyu · 2020-12-13T15:25:50Z

Btw, sorry to sound all negative, thanks for all your work!

No worry, thank you for your comments 👍🏼

davidhewitt

Looks good! Just need to chmod -x src/lib.rs before merging. Not sure how that happened 🤔

kngwyu · 2020-12-18T04:27:52Z

Thanks!

Not sure how that happened

Maybe my emacs setting does something wrong, but I'm also not sure 😓

kngwyu force-pushed the abi3-128bit-integer branch 2 times, most recently from 71e4eb8 to 07bade4 Compare December 13, 2020 08:45

Faster conversion from u128/i128 to PyLong with abi3

a93d97b

kngwyu force-pushed the abi3-128bit-integer branch from 07bade4 to a93d97b Compare December 13, 2020 08:52

birkenfeld reviewed Dec 13, 2020

View reviewed changes

davidhewitt reviewed Dec 13, 2020

View reviewed changes

kngwyu force-pushed the abi3-128bit-integer branch from 1a1ba74 to 5c1a2f2 Compare December 13, 2020 12:49

Faster conversion from PyLong to u128/i128 with LIMITED_API

3ebf526

kngwyu force-pushed the abi3-128bit-integer branch from 5c1a2f2 to 445d932 Compare December 13, 2020 14:51

programmerjake reviewed Dec 13, 2020

View reviewed changes

davidhewitt mentioned this pull request Dec 14, 2020

0.13 Release #1306

Closed

davidhewitt approved these changes Dec 16, 2020

View reviewed changes

kngwyu added 2 commits December 18, 2020 13:26

Fix py_run macro so that we can use it internally

c274b60

Use proptest for testing 128bit intger conversion

cd7348f

kngwyu force-pushed the abi3-128bit-integer branch from 445d932 to cd7348f Compare December 18, 2020 04:26

kngwyu merged commit e64dc12 into master Dec 19, 2020

messense deleted the abi3-128bit-integer branch March 18, 2021 02:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Faster conversion from u128/i128 to PyLong with abi3 #1315

Faster conversion from u128/i128 to PyLong with abi3 #1315

kngwyu commented Dec 13, 2020

kngwyu commented Dec 13, 2020 •

edited

Loading

birkenfeld Dec 13, 2020

kngwyu Dec 13, 2020

davidhewitt left a comment

davidhewitt Dec 13, 2020

kngwyu Dec 13, 2020

kngwyu commented Dec 13, 2020 •

edited

Loading

davidhewitt left a comment

davidhewitt Dec 13, 2020

kngwyu Dec 13, 2020

davidhewitt Dec 13, 2020

kngwyu Dec 13, 2020

davidhewitt Dec 13, 2020

kngwyu commented Dec 13, 2020

birkenfeld commented Dec 13, 2020

davidhewitt commented Dec 13, 2020

programmerjake commented Dec 13, 2020 •

edited

Loading

programmerjake commented Dec 13, 2020

davidhewitt commented Dec 13, 2020

kngwyu commented Dec 13, 2020

kngwyu commented Dec 13, 2020

programmerjake Dec 13, 2020

kngwyu Dec 13, 2020

programmerjake Dec 13, 2020

programmerjake Dec 13, 2020

kngwyu Dec 14, 2020

programmerjake Dec 14, 2020

programmerjake commented Dec 13, 2020

programmerjake commented Dec 13, 2020

programmerjake commented Dec 13, 2020

kngwyu commented Dec 13, 2020

davidhewitt left a comment

kngwyu commented Dec 18, 2020

	let (first, last) = split_128int(self.to_le_bytes());
	let (lower, upper) = split_128int(self.to_le_bytes());

Faster conversion from u128/i128 to PyLong with abi3 #1315

Faster conversion from u128/i128 to PyLong with abi3 #1315

Conversation

kngwyu commented Dec 13, 2020

kngwyu commented Dec 13, 2020 • edited Loading

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

kngwyu commented Dec 13, 2020 • edited Loading

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

kngwyu commented Dec 13, 2020

birkenfeld commented Dec 13, 2020

davidhewitt commented Dec 13, 2020

programmerjake commented Dec 13, 2020 • edited Loading

programmerjake commented Dec 13, 2020

davidhewitt commented Dec 13, 2020

kngwyu commented Dec 13, 2020

kngwyu commented Dec 13, 2020

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

programmerjake commented Dec 13, 2020

programmerjake commented Dec 13, 2020

programmerjake commented Dec 13, 2020

kngwyu commented Dec 13, 2020

davidhewitt left a comment

Choose a reason for hiding this comment

kngwyu commented Dec 18, 2020

kngwyu commented Dec 13, 2020 •

edited

Loading

kngwyu commented Dec 13, 2020 •

edited

Loading

programmerjake commented Dec 13, 2020 •

edited

Loading