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

Jump to bottom

gh-103650: Fix perf maps address format #103651

Merged
merged 2 commits into from May 7, 2023
Merged

gh-103650: Fix perf maps address format #103651

merged 2 commits into from May 7, 2023

Conversation

art049
Copy link
Contributor

@art049 art049 commented Apr 20, 2023 •
edited by arhadthedev

@bedevere-bot
Copy link

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.

@bedevere-bot bedevere-bot mentioned this pull request Apr 20, 2023
Closed
@cpython-cla-bot
Copy link

cpython-cla-bot bot commented Apr 20, 2023 •
edited

All commit authors signed the Contributor License Agreement.
CLA signed

@bedevere-bot
Copy link

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.

sunmy2019
sunmy2019 requested changes Apr 20, 2023
View reviewed changes
Python/perf_trampoline.c Outdated Show resolved Hide resolved
@arhadthedev arhadthedev added interpreter-core (Objects, Python, Grammar, and Parser dirs) 3.11 bug and security fixes 3.12 new features, bugs and security fixes labels Apr 20, 2023
@terryjreedy
Copy link
Member

I think that this should wait until Pablo verifies that it is indeed the implementation that should be changed.

@carljm
Copy link
Contributor

carljm commented May 5, 2023

@art049 did you verify that the new output after this PR works the same with perf?

@terryjreedy since the relevant docs here are the external perf map specification, not the CPython docs, it seems clear that the implementation in CPython should match the specification. There is no option to change the specification, we don't own it.

@art049
Copy link
Contributor Author

art049 commented May 5, 2023

@art049 did you verify that the new output after this PR works the same with perf?

Yes, also there are existing tests checking this:

cpython/Lib/test/test_perf_profiler.py

Lines 304 to 327 in 0ba49e2

def test_python_calls_appear_in_the_stack_if_perf_activated(self):
with temp_dir() as script_dir:
code = """if 1:
def foo(n):
x = 0
for i in range(n):
x += i
def bar(n):
foo(n)
def baz(n):
bar(n)
baz(10000000)
"""
script = make_script(script_dir, "perftest", code)
stdout, stderr = run_perf(script_dir, sys.executable, "-Xperf", script)
self.assertEqual(stderr, "")
self.assertIn(f"py::foo:{script}", stdout)
self.assertIn(f"py::bar:{script}", stdout)
self.assertIn(f"py::baz:{script}", stdout)

@pablogsal pablogsal added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label May 7, 2023
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @pablogsal for commit 0ba49e2 🤖

If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label May 7, 2023
@pablogsal
Copy link
Member

Running this PR with the buildbots as the perf stuff is not checked on regular CI.

@pablogsal pablogsal merged commit 8d95012 into python:main May 7, 2023
97 of 99 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sunmy2019 sunmy2019 sunmy2019 requested changes

@pablogsal pablogsal pablogsal approved these changes

Assignees
No one assigned
Labels
3.11 bug and security fixes 3.12 new features, bugs and security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@art049 @bedevere-bot @terryjreedy @carljm @pablogsal @sunmy2019 @arhadthedev