HeapTypeExt and Protocol Methods Pointer by qingshi163 · Pull Request #3819 · RustPython/RustPython

qingshi163 · 2022-06-25T07:44:41Z

No description provided.

qingshi163 · 2022-06-25T20:10:45Z

@youknowone can you run the performance check? I think we may use week order read after replace AtomicCell to AtomicPtr

youknowone · 2022-07-10T22:37:32Z

I am running tests. I used benchmark tests from https://github.com/python/pyperformance/tree/main/pyperformance/data-files/benchmarks
It has pyperf dependency. I manually installed it to my RUSTPYTHONPATH.

running command was:

./rustpython_main fannkuch.py -o reports/fannkuch_main.json

comparing command was:

python -m pyperf compare_to --table reports/*nbody*

$ python3 -m pyperf compare_to --table reports/*nbody*
+-----------+-------------------+----------------------+-------------------------+-------------------------+
| Benchmark | nbody_cpython3.10 | nbody_cpython3.9     | nbody_rustpython_3819   | nbody_rustpython_main   |
+===========+===================+======================+=========================+=========================+
| nbody     | 104 ms            | 101 ms: 1.03x faster | 1.78 sec: 17.16x slower | 1.80 sec: 17.36x slower |
+-----------+-------------------+----------------------+-------------------------+-------------------------+
$ python3 -m pyperf compare_to --table reports/*fannkuch*
+-----------+----------------------+----------------------+--------------------------+--------------------------+
| Benchmark | fannkuch_cpython3.10 | fannkuch_cpython3.9  | fannkuch_rustpython_3819 | fannkuch_rustpython_main |
+===========+======================+======================+==========================+==========================+
| fannkuch  | 352 ms               | 354 ms: 1.01x slower | 3.63 sec: 10.31x slower  | 3.66 sec: 10.41x slower  |
+-----------+----------------------+----------------------+--------------------------+--------------------------+
$ python3 -m pyperf compare_to --table reports/*scimark* 
+-------------------------+---------------------+-----------------------+-------------------------+-------------------------+
| Benchmark               | scimark_cpython3.10 | scimark_cpython3.9    | scimark_rustpython_3819 | scimark_rustpython_main |
+=========================+=====================+=======================+=========================+=========================+
| scimark_fft             | 291 ms              | 241 ms: 1.21x faster  | 3.82 sec: 13.11x slower | 3.87 sec: 13.28x slower |
+-------------------------+---------------------+-----------------------+-------------------------+-------------------------+
| scimark_lu              | 120 ms              | 115 ms: 1.05x faster  | 1.14 sec: 9.45x slower  | 1.14 sec: 9.49x slower  |
+-------------------------+---------------------+-----------------------+-------------------------+-------------------------+
| scimark_monte_carlo     | 75.0 ms             | 86.9 ms: 1.16x slower | 802 ms: 10.69x slower   | 799 ms: 10.65x slower   |
+-------------------------+---------------------+-----------------------+-------------------------+-------------------------+
| scimark_sor             | 143 ms              | 165 ms: 1.15x slower  | 1.69 sec: 11.83x slower | 1.71 sec: 11.92x slower |
+-------------------------+---------------------+-----------------------+-------------------------+-------------------------+
| scimark_sparse_mat_mult | 4.06 ms             | 3.47 ms: 1.17x faster | 44.0 ms: 10.84x slower  | 44.0 ms: 10.84x slower  |
+-------------------------+---------------------+-----------------------+-------------------------+-------------------------+
| Geometric mean          | (ref)               | 1.02x faster          | 11.12x slower           | 11.17x slower           |
+-------------------------+---------------------+-----------------------+-------------------------+-------------------------+

youknowone · 2022-07-11T01:23:28Z

-            Self::number_general_op(number, other, inner_div, vm)
-        }),
-        ..PyNumberMethods::NOT_IMPLEMENTED
+macro_rules! atomic_func {


putting in type/slots.rs will be better to share the macro over all protocol implementations

youknowone · 2022-07-11T01:26:27Z

-    const AS_NUMBER: PyNumberMethods;
-
-    #[inline]
    #[pyslot]
-    fn as_number(_zelf: &PyObject, _vm: &VirtualMachine) -> &'static PyNumberMethods {
-        &Self::AS_NUMBER
-    }


is this need to be changed?

we need static variable, const variable is not globally static, it creates instance wherever is used.

return static reference from a const variable gives me a warning with compiler.

youknowone · 2022-07-11T01:27:45Z

+    }
+}
+
+static AS_NUMBER: PyNumberMethods = PyNumberMethods {


even if we make this out of trait, it still can be defined in fn as_number() method unless we need it as a pub

youknowone · 2022-07-11T01:28:37Z

 };
+use crate::{


Suggested change

};

use crate::{

youknowone · 2022-07-11T01:35:03Z

@@ -1,189 +1,155 @@
+use crossbeam_utils::atomic::AtomicCell;
+use once_cell::sync::OnceCell;
+


Suggested change

youknowone · 2022-07-11T01:43:20Z

+    pub number_methods: PyNumberMethods,
+}
+
+pub struct PointerSlot<T>(NonNull<T>);


What's the benefit of this type comparing to &'static?
They seem to guarantee same things.

It just hint the reference should either be static or from type, I don't know is it neccesary.

youknowone · 2022-07-11T01:54:13Z

+impl<'a> From<&'a PyObject> for PyNumber<'a> {
+    fn from(obj: &'a PyObject) -> Self {


This change means having a PyNumber object doesn't guarantee it is an actual number object.
Do you intend to change it?
In that case, we'd better to carefully comment about it because it is counter-intutitive to rust sense.

I still think OnceCell for methods is an overkill because it can be safely managed only with properly designed types.

I think PyNumber shouldn't garrentee anything as it could be int, float, subclass, complex or class with _int_, _float_ etc, so what the caller expect is depend on what method been called.

The reason I use OnceCell is because we always use the type id first to avoid method call and I think that is more common case like get int from PyInt

We will obtain an PyNumber object only when we need PyNumber operations, isn't it? The method can be obtained when we create PyNumber, not when retrieving methods from it. PyNumber itself is a Once. OnceCell in PyNumber is duplicated encoding of data.

so maybe a function that will create PyNumber only if it is not exact type could be better?

I am sorry. I didn't understand well what you meant by "create PyNumber only if it is not exact type". could you help me to understand it?

I think we need to create a PyNumber type object only when we need to call methods() of it. In that case, we don't need OnceCell for PyNumber - because we will create a PyNumber object always with methods. If we don't need method, we can handle PyObjectRef instead of PyNumber.

because recent patches are adding number protocol implementations, quickly merging this one will be less painful in future.
I want to know how do you think about this OnceCell problem for now. You know about it the best.

@qingshi163 do you mind if I try to edit this PR to work this way? I want to listen your decision because you are the best professional for PyNumber.

I am thinking we should remove OnceCell from PyNumber but keep From<&PyObject> trait.
Than we modify or create function under VirtualMachine(to_int, to_index, to_float, etc) for all the function that could short cut with type id and do the check there. so we will always call number methods if we created PyNumber.

We should always use functions under VirtualMachine instead PyNumber if we do not sure the type.

I'd like to keep From<&PyObject> is because PyNumber is not garuentee anything about the type. we should not check the type when creating it.

Sure you can modify the pr, I am quite busy and I want to fix the sre-engine first.

youknowone · 2022-07-13T11:08:03Z

oh, I learned about those performance tools from @corona10, thank you!

youknowone · 2022-07-24T06:10:11Z

$ python3 -m pyperf compare_to --table reports/*nbody*
+-----------+-------------------+----------------------+-------------------------+-------------------------+
| Benchmark | nbody_cpython3.10 | nbody_cpython3.9     | nbody_rustpython_3819   | nbody_rustpython_main   |
+===========+===================+======================+=========================+=========================+
| nbody     | 103 ms            | 101 ms: 1.02x faster | 1.78 sec: 17.27x slower | 1.77 sec: 17.17x slower |
+-----------+-------------------+----------------------+-------------------------+-------------------------+

$ python3 -m pyperf compare_to --table reports/*fannkuch*
+-----------+----------------------+----------------------+--------------------------+--------------------------+
| Benchmark | fannkuch_cpython3.10 | fannkuch_cpython3.9  | fannkuch_rustpython_3819 | fannkuch_rustpython_main |
+===========+======================+======================+==========================+==========================+
| fannkuch  | 349 ms               | 348 ms: 1.00x faster | 3.59 sec: 10.31x slower  | 3.66 sec: 10.48x slower  |
+-----------+----------------------+----------------------+--------------------------+--------------------------+

$ python3 -m pyperf compare_to --table reports/*scimark* 
+-------------------------+---------------------+-----------------------+-------------------------+-------------------------+
| Benchmark               | scimark_cpython3.10 | scimark_cpython3.9    | scimark_rustpython_3819 | scimark_rustpython_main |
+=========================+=====================+=======================+=========================+=========================+
| scimark_fft             | 290 ms              | 239 ms: 1.21x faster  | 3.82 sec: 13.17x slower | 3.84 sec: 13.24x slower |
+-------------------------+---------------------+-----------------------+-------------------------+-------------------------+
| scimark_lu              | 120 ms              | 114 ms: 1.06x faster  | 1.13 sec: 9.41x slower  | 1.14 sec: 9.46x slower  |
+-------------------------+---------------------+-----------------------+-------------------------+-------------------------+
| scimark_monte_carlo     | 75.6 ms             | 86.1 ms: 1.14x slower | 790 ms: 10.45x slower   | 803 ms: 10.62x slower   |
+-------------------------+---------------------+-----------------------+-------------------------+-------------------------+
| scimark_sor             | 142 ms              | 164 ms: 1.16x slower  | 1.69 sec: 11.89x slower | 1.68 sec: 11.87x slower |
+-------------------------+---------------------+-----------------------+-------------------------+-------------------------+
| scimark_sparse_mat_mult | 4.04 ms             | 3.43 ms: 1.18x faster | 44.1 ms: 10.91x slower  | 44.2 ms: 10.95x slower  |
+-------------------------+---------------------+-----------------------+-------------------------+-------------------------+
| Geometric mean          | (ref)               | 1.03x faster          | 11.10x slower           | 11.16x slower           |
+-------------------------+---------------------+-----------------------+-------------------------+-------------------------+

$ python3 -m pyperf compare_to --table reports/*int*    
+-----------+-----------------+-----------------------+----------------------+----------------------+
| Benchmark | int_cpython3.10 | int_cpython3.9        | int_rustpython_3819  | int_rustpython_main  |
+===========+=================+=======================+======================+======================+
| int       | 47.8 ns         | 41.8 ns: 1.14x faster | 389 ns: 8.13x slower | 392 ns: 8.19x slower |
+-----------+-----------------+-----------------------+----------------------+----------------------+

$ cat int.py
import pyperf

def bench_int(loops):
    range_it = range(loops)
    t0 = pyperf.perf_counter()

    for _ in range_it:
        int(1.1)

    return pyperf.perf_counter() - t0

if __name__ == '__main__':
    DEFAULT_ITERATIONS=1000
    runner = pyperf.Runner()
    runner.metadata['description'] = "n-body benchmark"

    args = runner.parse_args()
    runner.bench_time_func('int', bench_int)

youknowone · 2022-07-24T06:11:36Z

@qingshi163 could you review changes?

qingshi163

Great!

qingshi163 changed the title ~~[WIP] HeapTypeExt and Protocol Methods Pointer~~ HeapTypeExt and Protocol Methods Pointer Jun 25, 2022

qingshi163 requested a review from youknowone June 25, 2022 20:08

youknowone mentioned this pull request Jun 27, 2022

Fix itertools.count to take PyNumber instead of PyInt #3822

Merged

qingshi163 marked this pull request as ready for review July 8, 2022 08:30

youknowone reviewed Jul 11, 2022

View reviewed changes

youknowone force-pushed the heaptypeext branch 2 times, most recently from 3884481 to e8fe8ea Compare July 23, 2022 08:19

qingshi163 added 5 commits July 24, 2022 07:36

refactor number methods as atomic function

0ebd4e0

proto heaptypeext

c244575

fix static lifetime

44daeef

use heaptypeext for number protocol

2895c81

refactor replace nonnull with pointerslot

069c6c9

youknowone force-pushed the heaptypeext branch from e8fe8ea to 96935a8 Compare July 23, 2022 22:47

resolve conflict

e871a53

youknowone force-pushed the heaptypeext branch 2 times, most recently from d345c31 to 09c4fa3 Compare July 24, 2022 05:57

youknowone added 5 commits July 24, 2022 15:05

no once cell

a05712f

remove vm.try_index

2d1158b

PyObject::try_index

7a43a2e

PyObject::try_int

b96dddf

PyObject::try_float

370c4f7

youknowone force-pushed the heaptypeext branch from 09c4fa3 to 370c4f7 Compare July 24, 2022 06:07

qingshi163 commented Jul 24, 2022

View reviewed changes

youknowone approved these changes Jul 24, 2022

View reviewed changes

youknowone merged commit 2e7ec1d into RustPython:main Jul 24, 2022

qingshi163 deleted the heaptypeext branch July 24, 2022 19:48

youknowone mentioned this pull request Aug 7, 2022

[WIP] refactor number protocol store in pytype #3781

Closed

youknowone mentioned this pull request Aug 15, 2022

Unable to install RustPython from source with cargo install. #3982

Closed

		@@ -1,189 +1,155 @@
		use crossbeam_utils::atomic::AtomicCell;
		use once_cell::sync::OnceCell;

		impl<'a> From<&'a PyObject> for PyNumber<'a> {
		fn from(obj: &'a PyObject) -> Self {

Conversation

qingshi163 commented Jun 25, 2022

Uh oh!

qingshi163 commented Jun 25, 2022

Uh oh!

youknowone commented Jul 10, 2022

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qingshi163 Jul 11, 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!

youknowone commented Jul 13, 2022

Uh oh!

youknowone commented Jul 24, 2022

Uh oh!

youknowone commented Jul 24, 2022

Uh oh!

qingshi163 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

qingshi163 Jul 11, 2022 •

edited

Loading