GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-126220: Adapt `_lsprof` to Argument Clinic by sobolevn · Pull Request #126233 · python/cpython

Conversation

@sobolevn

@sobolevn sobolevn commented Oct 31, 2024 •
edited by bedevere-app Bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@sobolevn
pythongh-126220: Fix crash on calls to _lsprof.Profiler methods wit…
299f63f
…h 0 args
@sobolevn
sobolevn requested review from gaogaotiantian and markshannon October 31, 2024 11:08
@bedevere-app bedevere-app Bot added the awaiting core review label Oct 31, 2024
@bedevere-app bedevere-app Bot mentioned this pull request Oct 31, 2024
_lsprof.Profiler._creturn_callback() segfaults #126220
Closed
@sobolevn
Fix CI
af78745
ZeroIntensity
ZeroIntensity approved these changes Oct 31, 2024
View reviewed changes

@ZeroIntensity ZeroIntensity left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@gaogaotiantian

gaogaotiantian commented Oct 31, 2024

Copy link
Copy Markdown
Member

What's the perf impact here?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@sobolevn

sobolevn commented Oct 31, 2024

Copy link
Copy Markdown
Member Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@gaogaotiantian

gaogaotiantian commented Oct 31, 2024

Copy link
Copy Markdown
Member

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@sobolevn

sobolevn commented Oct 31, 2024

Copy link
Copy Markdown
Member Author

@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.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 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?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@gaogaotiantian

gaogaotiantian commented Oct 31, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

Sorry I meant a recursive fib().

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 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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@sobolevn

sobolevn commented Oct 31, 2024

Copy link
Copy Markdown
Member Author

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 
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@gaogaotiantian

gaogaotiantian commented Oct 31, 2024

Copy link
Copy Markdown
Member

Is the boost stable(reproducible)?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@sobolevn

sobolevn commented Oct 31, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@gaogaotiantian

gaogaotiantian commented Oct 31, 2024

Copy link
Copy Markdown
Member

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@gaogaotiantian gaogaotiantian added needs backport to 3.12 only security fixes needs backport to 3.13 only security fixes labels Oct 31, 2024
gaogaotiantian
gaogaotiantian approved these changes Oct 31, 2024
View reviewed changes

@gaogaotiantian gaogaotiantian left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for 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.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@bedevere-app bedevere-app Bot added awaiting merge and removed awaiting core review labels Oct 31, 2024
@sobolevn

sobolevn commented Oct 31, 2024

Copy link
Copy Markdown
Member Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@gaogaotiantian

gaogaotiantian commented Oct 31, 2024

Copy link
Copy Markdown
Member

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).

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@gaogaotiantian

gaogaotiantian commented Oct 31, 2024

Copy link
Copy Markdown
Member

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

erlend-aasland reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@sobolevn

sobolevn commented Oct 31, 2024

Copy link
Copy Markdown
Member Author

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

gaogaotiantian reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

sobolevn added 2 commits October 31, 2024 22:06
@sobolevn
Only fix 4 crashers
3fddc30
@sobolevn
Use instruction_offset arg name
4ffecdd
@sobolevn
sobolevn requested a review from gaogaotiantian October 31, 2024 19:08
gaogaotiantian
gaogaotiantian approved these changes Oct 31, 2024
View reviewed changes

@gaogaotiantian gaogaotiantian left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

sobolevn reacted with heart emoji
All reactions
  • ❤️ 1 reaction
@sobolevn

sobolevn commented Oct 31, 2024

Copy link
Copy Markdown
Member Author

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

erlend-aasland reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@sobolevn
Convert all methods
e3160e3
@sobolevn sobolevn changed the title gh-126220: Fix crash on calls to _lsprof.Profiler methods with 0 args gh-126220: Convert _lsprof to AC Nov 1, 2024
@sobolevn sobolevn added the skip news label Nov 1, 2024
@sobolevn
sobolevn requested a review from gaogaotiantian November 1, 2024 22:09
erlend-aasland
erlend-aasland reviewed Nov 1, 2024
View reviewed changes
Comment thread Modules/_lsprof.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Modules/_lsprof.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Modules/_lsprof.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Modules/_lsprof.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

@erlend-aasland erlend-aasland changed the title gh-126220: Convert _lsprof to AC gh-126220: Adapt _lsprof to Argument Clinic Nov 1, 2024
sobolevn and others added 3 commits November 2, 2024 08:30
@sobolevn @erlend-aasland
Apply suggestions from code review
3cea189
Co-authored-by: Erlend E. Aasland <[email protected]>
@sobolevn
Address review
b84dde3
@sobolevn
Typo
bf3e120
@sobolevn
sobolevn requested a review from erlend-aasland November 3, 2024 07:09
gaogaotiantian
gaogaotiantian reviewed Nov 3, 2024
View reviewed changes
Comment thread Modules/_lsprof.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

erlend-aasland
erlend-aasland reviewed Nov 3, 2024
View reviewed changes
Comment thread Modules/_lsprof.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

@sobolevn
Address review
0d35f60
@sobolevn

sobolevn commented Nov 4, 2024

Copy link
Copy Markdown
Member Author

Done!

erlend-aasland reacted with hooray emoji erlend-aasland reacted with rocket emoji
All reactions
  • 🎉 1 reaction
  • 🚀 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

erlend-aasland
erlend-aasland approved these changes Nov 4, 2024
View reviewed changes
@sobolevn
sobolevn requested a review from gaogaotiantian November 4, 2024 08:48
gaogaotiantian
gaogaotiantian approved these changes Nov 4, 2024
View reviewed changes
@sobolevn
sobolevn merged commit c806cd5 into python:main Nov 4, 2024
@bedevere-app bedevere-app Bot removed the awaiting merge label Nov 4, 2024
@miss-islington-app

miss-islington-app Bot commented Nov 4, 2024

Copy link
Copy Markdown

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@sobolevn

sobolevn commented Nov 4, 2024

Copy link
Copy Markdown
Member Author

Thanks everyone!

erlend-aasland reacted with hooray emoji
All reactions
  • 🎉 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Nov 4, 2024
@sobolevn @erlend-aasland @miss-islington
pythongh-126220: Adapt _lsprof to Argument Clinic (pythonGH-126233)
9eaf3e6
(cherry picked from commit c806cd5)

Co-authored-by: sobolevn <[email protected]>
Co-authored-by: Erlend E. Aasland <[email protected]>
@miss-islington-app

miss-islington-app Bot commented Nov 4, 2024

Copy link
Copy Markdown

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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-app

bedevere-app Bot commented Nov 4, 2024

Copy link
Copy Markdown

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@miss-islington-app miss-islington-app Bot assigned sobolevn Nov 4, 2024
@bedevere-app bedevere-app Bot removed the needs backport to 3.13 only security fixes label Nov 4, 2024
@sobolevn

sobolevn commented Nov 4, 2024

Copy link
Copy Markdown
Member Author

Oups, backports are not needed anymore. Closing them.

erlend-aasland reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@sobolevn sobolevn removed the needs backport to 3.12 only security fixes label Nov 4, 2024
picnixz pushed a commit to picnixz/cpython that referenced this pull request Dec 8, 2024
@sobolevn @erlend-aasland @picnixz
pythongh-126220: Adapt _lsprof to Argument Clinic (python#126233)
c86f49b
Co-authored-by: Erlend E. Aasland <[email protected]>
ebonnal pushed a commit to ebonnal/cpython that referenced this pull request Jan 12, 2025
@sobolevn @erlend-aasland @ebonnal
pythongh-126220: Adapt _lsprof to Argument Clinic (python#126233)
2081eee
Co-authored-by: Erlend E. Aasland <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gaogaotiantian gaogaotiantian gaogaotiantian approved these changes
@erlend-aasland erlend-aasland erlend-aasland approved these changes
@ZeroIntensity ZeroIntensity ZeroIntensity approved these changes
@markshannon markshannon Awaiting requested review from markshannon

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@sobolevn sobolevn

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@sobolevn @gaogaotiantian @erlend-aasland @ZeroIntensity
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.