GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | ||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | ||||||||||||||||||||
GeezerVison is showing github.com — gh-126220: Adapt `_lsprof` to Argument Clinic by sobolevn · Pull Request #126233 · python/cpython Conversation
|
|
What's the perf impact here? |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
@gaogaotiantian I've never had experience with profiling |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Just do a quick fib() with cprofile and compare it with the previous implementation would do. Maybe also with one without cprofile. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
@gaogaotiantian maybe I did something wrong, but looks like we also got a speedup from my numbers 😅 (I have little idea about how # Test code: ex.py def test(): def fib(): start, nextn = 0, 1 while True: yield start start, nextn = nextn, start + nextn gen = fib() for _ in range(100000): next(gen) import cProfile cProfile.run(test.__code__) Running it on Running on this PR: Am I missing something? |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
Sorry I meant a recursive import time def fib(n): if n <= 1: return 1 return fib(n - 1) + fib(n - 2) start = time.time() # enable profiler fib(24) # disable profiler print(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 |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
It still gives me a perf boost for some reason 🤔 import time import cProfile profiler = cProfile.Profile() def fib(n): if n <= 1: return 1 return fib(n - 1) + fib(n - 2) start = time.perf_counter() profiler.enable() fib(30) # 100th profiler.disable() print(time.perf_counter() - start) # Old: 0.9349823750017094 # New: 0.9253051670020795 # 1% faster |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Is the boost stable(reproducible)? |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
Yes, it is pretty much the same on m2 macos: |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
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. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
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.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
@gaogaotiantian sorry, I forgot to highlight it initially. Please, double check params names that I introduced. Are they correct? |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
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 As for the argument name, the |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Can we split this PR into 2? One with the changes to only the 4 callbacks, and the other with all the other methods? |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Fair enough, I would split this PR later! 👍 |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
@erlend-aasland maybe you would be interested in double checking the AC part? :) |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
_lsprof.Profiler methods with 0 args_lsprof to AC
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
_lsprof to AC_lsprof to Argument Clinic
Co-authored-by: Erlend E. Aasland <[email protected]>
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
Done! |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Thanks @sobolevn for the PR 🌮🎉.. I'm working now to backport this PR to: 3.12, 3.13. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Thanks everyone! |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
(cherry picked from commit c806cd5) Co-authored-by: sobolevn <[email protected]> Co-authored-by: Erlend E. Aasland <[email protected]>
|
Sorry, @sobolevn, I could not cleanly backport this to |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
GH-126402 is a backport of this pull request to the 3.13 branch. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Oups, backports are not needed anymore. Closing them. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
Co-authored-by: Erlend E. Aasland <[email protected]>
Co-authored-by: Erlend E. Aasland <[email protected]>
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.