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.

Sign up for GitHub

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-103295: expose API for writing perf map files #103546

Merged

pablogsal merged 60 commits into python:main from gsallam:main

May 21, 2023

Contributor

gsallam commented Apr 14, 2023 •

edited by bedevere-bot

Loading

#96123 added support for CPython to write /tmp/perf-.map files, associating instruction address ranges with a human-readable frame name for the Linux perf profiler.

Two external Python JIT compilers, Cinder and Pyston, both also independently write to perf map files.

Since perf map files are one-per-process, multiple separate libraries trying to write perf map entries independently can lead to file corruption from simultaneous writes.

It's unlikely for both Cinder and Pyston JITs to be used in the same process, but it's quite reasonable to use one of these JITs along with CPython's native perf trampoline support.

This PR add a C-API to write to the perf map file. It also update the perf trampoline to use the new API to write to the perf map file.

Issue: expose API for writing perf map files #103295

cpython-cla-bot bot commented Apr 14, 2023 •

edited

Loading

All commit authors signed the Contributor License Agreement.

bedevere-bot commented Apr 14, 2023

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 mentioned this pull request

expose API for writing perf map files #103295

Closed

bedevere-bot added the awaiting review label

bedevere-bot commented Apr 14, 2023

Most changes to Python require a NEWS entry.

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

4 similar comments

bedevere-bot commented Apr 14, 2023

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 commented Apr 14, 2023

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 commented Apr 14, 2023

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 commented Apr 14, 2023

Most changes to Python require a NEWS entry.

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

gsallam marked this pull request as ready for review

April 16, 2023 20:50

arhadthedev added the interpreter-core label

Contributor

czardoz commented Apr 19, 2023

Looks like the CI is having trouble installing dependencies, I will update this branch with rebase and try again in a bit

gsallam and others added 18 commits

April 19, 2023 15:27


          Update osmodule.h

f76d2c5


          Update posixmodule.c

de6b63e


          Update pycore_ceval_state.h

4f55b2d


          Update perf_trampoline.c

03fccfb


          Update pylifecycle.c

78d543a


          Update pycore_ceval.h

e8c321d


          Update _testinternalcapi.c

3a97933


          Create test_perfmaps.py

94441c4


          Update posixmodule.c

a221fd8


          Update osmodule.h

392e842


          Update osmodule.h


          Update osmodule.h

804153a


          Update posixmodule.c

ce54c6c


          📜🤖 Added by blurb_it.

6c3f233


          Update osmodule.h

71db361


          Update test_perfmaps.py

020c872


          Update ignored.tsv

e33b45d


          Fix trampoline APIs on windows, return int instead of void* from the …

04e9716

…init function

bedevere-bot added the awaiting merge label

sunmy2019 requested changes

View reviewed changes

Python/perf_trampoline.c Show resolved Hide resolved

facebook-github-bot pushed a commit to facebookincubator/cinder that referenced this pull request


          Backport perf trampoline

a88f497

Summary: Backport the perf-trampoline introduced in python/cpython#96123.  The perf trampoline doesn't work properly with the JIT, so we have submitted a PR to have a C-API to unify writing to the perf-map files python/cpython#103546.

Reviewed By: czardoz

Differential Revision: D45419843

fbshipit-source-id: 16bd13d7981e48c9eb7bc0e5eef1c1f4748965f6

facebook-github-bot pushed a commit to facebookincubator/cinder that referenced this pull request


          Perfmap C-API and JIT integration

c475629

Summary:
With the perf trampoline writing to the perf-map files, we want to have a C-API to unify writing to the perf-map files to avoid file corruption from simultaneous writes. We are trying to upstream the API here python/cpython#103546. More details about the motivation is in the PR.

In addition to introducing the C-API, we also change JIT to utilize the new C-API.

Reviewed By: czardoz

Differential Revision: D45421966

fbshipit-source-id: d270cc753a245f93cbfe3d723d0880595fef45f2

gsallam and others added 8 commits

May 19, 2023 17:37


          Return error codes instead of setting exceptions in PyUnstable_PerfMa…

d245fbf

…pState_Init


          Add a note that holding the GIL is not required for these APIs

2027de6


          Remove the init_state call since it is always NULL for the perf backend

0383b80


          trim extra whitespaces

59f3c15


          simplify link to perf map docs from perf profiling

8bc6150


          document return values, other doc tweaks

c7a49b4


          move headers to sysmodule.h

125513a


          pass through init return value from write-entry

10630e8

carljm requested a review from sunmy2019

May 20, 2023 03:41

carljm added the 🔨 test-with-buildbots label

bedevere-bot commented May 20, 2023

🤖 New build scheduled with the buildbot fleet by @carljm for commit 10630e8 🤖

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

bedevere-bot removed the 🔨 test-with-buildbots label

gpshead approved these changes

View reviewed changes

sunmy2019 approved these changes

View reviewed changes

Python/perf_trampoline.c Outdated Show resolved Hide resolved

Python/perf_trampoline.c Outdated Show resolved Hide resolved

carljm added 4 commits

May 19, 2023 21:41


          apparently :c:data:errno doesn't exist

386dd42


          use PyMem_Raw* instead of malloc/free

6585f32


          Merge branch 'main' into gsallam/main

ea5965a

* main: (30 commits)
  pythongh-103987: fix several crashes in mmap module (python#103990)
  docs: fix wrong indentation causing rendering error in dis page (python#104661)
  pythongh-94906: Support multiple steps in math.nextafter (python#103881)
  pythongh-104472: Skip `test_subprocess.ProcessTestCase.test_empty_env` if ASAN is enabled (python#104667)
  pythongh-103839: Allow building Tkinter against Tcl 8.7 without external libtommath (pythonGH-103842)
  pythongh-85984: New additions and improvements to the tty library. (python#101832)
  pythongh-104659: Consolidate python examples in enum documentation (python#104665)
  pythongh-92248: Deprecate `type`, `choices`, `metavar` parameters of `argparse.BooleanOptionalAction` (python#103678)
  pythongh-104645: fix error handling in marshal tests (python#104646)
  pythongh-104600: Make type.__type_params__ writable (python#104634)
  pythongh-104602: Add additional test for listcomp with lambda (python#104639)
  pythongh-104640: Disallow walrus in comprehension within type scopes (python#104641)
  pythongh-103921: Rename "type" header in argparse docs (python#104654)
  Improve readability of `typing._ProtocolMeta.__instancecheck__` (python#104649)
  pythongh-96522: Fix deadlock in pty.spawn (python#96639)
  pythonGH-102818: Do not call `PyTraceBack_Here` in sys.settrace trampoline.  (pythonGH-104579)
  pythonGH-103545: Add macOS specific constants for ``os.setpriority`` to ``os`` (python#104606)
  pythongh-104623: Update macOS installer to SQLite 3.42.0 (pythonGH-104624)
  pythongh-104619: never leak comprehension locals to outer locals() (python#104637)
  pythongh-104602: ensure all cellvars are known up front (python#104603)
  ...


          fix PyMem_Free to PyMem_RawFree

e6be2a7

carljm added the 🔨 test-with-buildbots label

bedevere-bot commented May 20, 2023

🤖 New build scheduled with the buildbot fleet by @carljm for commit e6be2a7 🤖

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

bedevere-bot removed the 🔨 test-with-buildbots label

Member

carljm commented May 20, 2023 •

edited

Loading

Refleak buildbot failures are unrelated, they are all failing in test_mmap, which is a known issue on main: #104698. Otherwise buildbots look happy.

I think this is ready for merge; @pablogsal I will plan to merge it tomorrow evening unless you have objections!

pablogsal merged commit be0c106 into python:main

bedevere-bot removed the awaiting merge label

Member

pablogsal commented May 21, 2023

I think this is ready for merge; @pablogsal I will plan to merge it tomorrow evening unless you have objections!

I went ahead and land it myself after another review pass 😉

Great job everyone!

facebook-github-bot pushed a commit to facebookincubator/cinder that referenced this pull request


          User error codes and fix 32-bit system issues

0a7cc0b

Summary:
Port the latest changes from the [perf-map C-API PR](python/cpython#103546). This includes using error codes instead of exceptions to avoid the need to hold the GIL.

Additionally, there was an issue with Fedora on a 32-bit system that was fixed in this [PR](python/cpython#104811), so I also ported the fix.

Reviewed By: carljm

Differential Revision: D46041553

fbshipit-source-id: 6502189f489ca6cf11949ecd93c044e5ebe4fd2c

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

pablogsal pablogsal left review comments

czardoz czardoz left review comments

carljm carljm left review comments

sunmy2019 sunmy2019 approved these changes

gpshead gpshead approved these changes

Labels

interpreter-core