gh-126220: Adapt _lsprof to Argument Clinic

[sobolevn]

I decided to go with the AC, because:

1. It is supported in this module
2. It solves the problem
3. It fixes multiple signatures

But, I believe it will be slower. If we really want to preserve speed in this case, I can add manual size checks for args.

Refs #103534

• Issue: _lsprof.Profiler._creturn_callback() segfaults #126220

[ZeroIntensity]

LGTM. I'm always happy with fixes via AC, because they're much more scalable.

[gaogaotiantian]

What's the perf impact here?

[sobolevn]

@gaogaotiantian I've never had experience with profiling cProfile. Do you have any links / suggestions on how to do that?

[gaogaotiantian]

Just do a quick fib() with cprofile and compare it with the previous implementation would do. Maybe also with one without cprofile.

[sobolevn]

@gaogaotiantian maybe I did something wrong, but looks like we also got a speedup from my numbers 😅 (I have little idea about how cProfile works).

# Test code: ex.pydeftest():
deffib():
start, nextn=0, 1whileTrue:
yieldstartstart, nextn=nextn, start+nextngen=fib()
for_inrange(100000):
next(gen)
importcProfilecProfile.run(test.__code__)

Running it on main:

» ./python.exe ex.py
200003 function calls in 0.188 seconds
Ordered by: standard name
ncalls tottime percall cumtime percall filename:lineno(function)
1 0.017 0.017 0.188 0.188 ex.py:1(test)
100000 0.147 0.000 0.147 0.000 ex.py:2(fib)
1 0.000 0.000 0.188 0.188 {built-in method builtins.exec}
100000 0.024 0.000 0.171 0.000 {built-in method builtins.next}
1 0.000 0.000 0.000 0.000 {method 'disable' of '_lsprof.Profiler' objects}

Running on this PR:

» ./python.exe ex.py 200003 function calls in 0.185 seconds
Ordered by: standard name
ncalls tottime percall cumtime percall filename:lineno(function)
1 0.016 0.016 0.185 0.185 ex.py:1(test)
100000 0.145 0.000 0.145 0.000 ex.py:2(fib)
1 0.000 0.000 0.185 0.185 {built-in method builtins.exec}
100000 0.024 0.000 0.168 0.000 {built-in method builtins.next}
1 0.000 0.000 0.000 0.000 {method 'disable' of '_lsprof.Profiler' objects}

Am I missing something?

[gaogaotiantian]

Sorry I meant a recursive fib().

importtimedeffib(n):
ifn<=1:
return1returnfib(n-1) +fib(n-2)
start=time.time()
# enable profilerfib(24)
# disable profilerprint(time.time() -start)

We need to consider the worst case for cprofile, which is when the code has a lot of function calls. We need to know the overhead of the profiler, and how that is compared with before.

You can also refer to #103533, there's a profiling code listed. That might give some extra information.

Moving to sys.monitoring gave us a 20%+ improvement on overhead, just want to check what's the regression with clinic and whether it's acceptable. Profiler is a bit sensitive to performance because the more overhead there is, the less useful it is.

[sobolevn]

It still gives me a perf boost for some reason 🤔

importtimeimportcProfileprofiler=cProfile.Profile()
deffib(n):
ifn<=1:
return1returnfib(n-1) +fib(n-2)
start=time.perf_counter()
profiler.enable()
fib(30) # 100thprofiler.disable()
print(time.perf_counter() -start)
# Old: 0.9349823750017094# New: 0.9253051670020795# 1% faster 

[gaogaotiantian]

Is the boost stable(reproducible)?

[sobolevn]

Yes, it is pretty much the same on m2 macos:

(.venv) ~/Desktop/cpython2 main ✗ » ./python.exe ex.py 0.9349823750017094
(.venv) ~/Desktop/cpython2 main ✗ » ./python.exe ex.py
0.9357958749969839
(.venv) ~/Desktop/cpython2 main ✗ » ./python.exe ex.py
0.9352227920026053
(.venv) ~/Desktop/cpython2 main ✗ » ./python.exe ex.py
0.924946416002058
(.venv) ~/Desktop/cpython2 main ✗ » ./python.exe ex.py
0.926337541997782
(.venv) ~/Desktop/cpython2 main ✗ » ./python.exe ex.py
0.9347994170000311

(.venv) ~/Desktop/cpython2 issue-126220 ✗ » ./python.exe ex.py
0.9370929580036318
(.venv) ~/Desktop/cpython2 issue-126220 ✗ » ./python.exe ex.py
0.9262957079990883
(.venv) ~/Desktop/cpython2 issue-126220 ✗ » ./python.exe ex.py
0.9328266249940498
(.venv) ~/Desktop/cpython2 issue-126220 ✗ » ./python.exe ex.py
0.9382220000011148
(.venv) ~/Desktop/cpython2 issue-126220 ✗ » ./python.exe ex.py
0.9285237919984502
(.venv) ~/Desktop/cpython2 issue-126220 ✗ » ./python.exe ex.py
0.9296539580027456

[gaogaotiantian]

Yeah from the result I don't think there's an observable boost. On the other hand, that's good, because no observable regression either.

[gaogaotiantian]

Just a side note - this needs to be backported to 3.12 as well :) I added the tags so you should be able to just merge it.

[sobolevn]

@gaogaotiantian sorry, I forgot to highlight it initially. Please, double check params names that I introduced. Are they correct?

[gaogaotiantian]

Actually, I have some doubts about the changes for functions that are not crashing. This is a bug fix, which would be backported. I don't think we should mix in the code polish to the PR. Even though I think changing this in main would be fine, I would prefer having only the crash fix (could be with AC) in one PR, and the rest in the other. One thing that bothers me immediately is that I can't quickly convince myself the changes to __init__ is purely equivalent.

As for the argument name, the obj in the callbacks should be either unused or instruction_offset(which is what it is).

[gaogaotiantian]

Can we split this PR into 2? One with the changes to only the 4 callbacks, and the other with all the other methods?

[sobolevn]

Fair enough, I would split this PR later! 👍
Thank you!

[gaogaotiantian]

LGTM!

[sobolevn]

@erlend-aasland maybe you would be interested in double checking the AC part? :)

[sobolevn]

Done!

[miss-islington-app]

Thanks @sobolevn for the PR 🌮🎉.. I'm working now to backport this PR to: 3.12, 3.13.
🐍🍒⛏🤖

[sobolevn]

Thanks everyone!

[miss-islington-app]

Sorry, @sobolevn, I could not cleanly backport this to 3.12 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker c806cd5af677c385470001efc68da38a32919196 3.12

[bedevere-app]

GH-126402 is a backport of this pull request to the 3.13 branch.

[sobolevn]

Oups, backports are not needed anymore. Closing them.