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

Unexpectedly slow class method calls #226

Closed
denik opened this issue Oct 28, 2023 · 2 comments · Fixed by #230
Closed

Unexpectedly slow class method calls #226

denik opened this issue Oct 28, 2023 · 2 comments · Fixed by #230

Comments

@denik
Copy link

denik commented Oct 28, 2023

Calling a class method is unexpectedly much slower (10-20x) than calling a function. This is not observed with comparable Cython code, so not an issue with Python class in C extensions.

pydust: calling function is fast (compiled with "zig build -Doptimize=ReleaseFast")

$ python -m timeit -s 'from fibonacci._lib import get1' 'get1()'
5000000 loops, best of 5: 37.7 nsec per loop

$ python -m timeit -s 'from fibonacci._lib import get1' 'get1()'
5000000 loops, best of 5: 46.1 nsec per loop

Calling class method is slow:

$ python -m timeit -s 'from fibonacci._lib import Class; s = Class(10)' 's.get()'
200000 loops, best of 5: 981 nsec per loop

$ python -m timeit -s 'from fibonacci._lib import Class; s = Class(10)' 's.get()'
200000 loops, best of 5: 922 nsec per loop

Code: diff against fresh checkout of ziggy-pydust-template:

$ git diff
diff --git a/src/fib.zig b/src/fib.zig
index 3fde4f7..c32e91c 100644
--- a/src/fib.zig
+++ b/src/fib.zig
@@ -80,6 +80,28 @@ pub const FibonacciIterator = py.class(struct {
     }
 });
 
+pub const Class = py.class(struct {
+    const Self = @This();
+
+    i: u64,
+
+    pub fn __new__(args: struct { i: u64 }) !Self {
+        return .{ .i = args.i };
+    }
+
+    pub fn get(self: *Self) u64 {
+        return self.i;
+    }
+});
+
+pub fn get1() u64 {
+    return 10;
+}

Same thing done with Cython - same performance for function and method call:

$ python -m timeit -s 'from testclass import get1' 'get1()'
5000000 loops, best of 5: 39.3 nsec per loop

$ python -m timeit -s 'from testclass import get1' 'get1()'
5000000 loops, best of 5: 44.1 nsec per loop

$ python -m timeit -s 'from testclass import Class; s = Class(10)' 's.get()'
5000000 loops, best of 5: 44.5 nsec per loop

$ python -m timeit -s 'from testclass import Class; s = Class(10)' 's.get()'
5000000 loops, best of 5: 47 nsec per loop

$ cat testclass.pyx 
cdef class Class:
    cdef int i

    def __init__(self, i):
        self.i = i

    cpdef get(self):
        return self.i

cpdef get1():
    return 10

$ cat setup.py 
from setuptools import setup
from Cython.Build import cythonize

setup(
    ext_modules = cythonize("testclass.pyx")
)

$ python setup.py build_ext --inplace
@gatesn
Copy link
Contributor

gatesn commented Oct 28, 2023

Thank you for reporting this, I'll take a look. My initial suspicion is that we have some naive code that asserts the type of the self argument - which is probably unnecessary given we know it's a class/instance method.

@gatesn
Copy link
Contributor

gatesn commented Oct 28, 2023

Yup, so a trace confirmed all the time is spent doing an import then isinstance check on the self parameter.

Screenshot 2023-10-28 at 22 37 43

I've put up a PR #228 that seems to fix it. With optimize=Debug:

(ziggy-pydust-py3.11) ➜  ziggy-pydust git:(ngates/class-method-perf) ✗ python -m timeit -s 'from example.fib import get1' 'get1()'
5000000 loops, best of 5: 85.2 nsec per loop
(ziggy-pydust-py3.11) ➜  ziggy-pydust git:(ngates/class-method-perf) ✗ python -m timeit -s 'from example.fib import Class; s = Class(10)' 's.get()'
5000000 loops, best of 5: 95.5 nsec per loop

I'll try to put up a proper fix to this (which I believe to be #227) tomorrow sometime.

gatesn added a commit that referenced this issue Oct 30, 2023
TODO(ngates): we should store PyType objects on module state and then
auto-traverse them. See #229

Fixes #226, #227, #228
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 a pull request may close this issue.

2 participants