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.

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-103865: add monitoring support to LOAD_SUPER_ATTR #103866

Merged
merged 17 commits into from
May 16, 2023
Merged

gh-103865: add monitoring support to LOAD_SUPER_ATTR #103866

merged 17 commits into from
May 16, 2023

Conversation

carljm
Copy link
Member

@carljm carljm commented Apr 26, 2023 •
edited by bedevere-bot
Loading

@bedevere-bot bedevere-bot mentioned this pull request Apr 26, 2023
Closed
@carljm
Copy link
Member Author

carljm commented Apr 26, 2023

Skipping news, since this is a bugfix to a new feature that already has its own news entry.

@JelleZijlstra JelleZijlstra added the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Apr 26, 2023
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @JelleZijlstra for commit 862c3dc 🤖

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

@bedevere-bot bedevere-bot removed the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Apr 26, 2023
@JelleZijlstra JelleZijlstra added the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Apr 26, 2023
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @JelleZijlstra for commit 862c3dc 🤖

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

@bedevere-bot bedevere-bot removed the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Apr 26, 2023
@JelleZijlstra JelleZijlstra mentioned this pull request Apr 26, 2023
Merged
@JelleZijlstra
Copy link
Member

I triggered the refleak buildbots here because I suspect that some of the refleaks I saw in #103764 aren't related to my changes. That indeed seems to be the case.

@JelleZijlstra JelleZijlstra mentioned this pull request Apr 26, 2023
Closed
@carljm carljm added the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Apr 26, 2023
@bedevere-bot
Copy link

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

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

@bedevere-bot bedevere-bot removed the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Apr 26, 2023
@carljm carljm added the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Apr 26, 2023
@bedevere-bot
Copy link

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

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

@bedevere-bot bedevere-bot removed the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Apr 26, 2023
carljm added 2 commits May 1, 2023 07:59
@carljm
Merge branch 'main' into supermon
159b2aa 
* main: (26 commits)
  pythongh-104028: Reduce object creation while calling callback function from gc (pythongh-104030)
  pythongh-104036: Fix direct invocation of test_typing (python#104037)
  pythongh-102213: Optimize the performance of `__getattr__` (pythonGH-103761)
  pythongh-103895: Improve how invalid `Exception.__notes__` are displayed (python#103897)
  Adjust expression from `==` to `!=` in alignment with the meaning of the paragraph. (pythonGH-104021)
  pythongh-88496: Fix IDLE test hang on macOS (python#104025)
  Improve int test coverage (python#104024)
  pythongh-88773: Added teleport method to Turtle library (python#103974)
  pythongh-104015: Fix direct invocation of `test_dataclasses` (python#104017)
  pythongh-104012: Ensure test_calendar.CalendarTestCase.test_deprecation_warning consistently passes (python#104014)
  pythongh-103977: compile re expressions in platform.py only if required (python#103981)
  pythongh-98003: Inline call frames for CALL_FUNCTION_EX (pythonGH-98004)
  Replace Netlify with Read the Docs build previews (python#103843)
  Update name in acknowledgements and add mailmap (python#103696)
  pythongh-82054: allow test runner to split test_asyncio to execute in parallel by sharding. (python#103927)
  Remove non-existing tools from Sundry skiplist (python#103991)
  pythongh-103793: Defer formatting task name (python#103767)
  pythongh-87092: change assembler to use instruction sequence instead of CFG (python#103933)
  pythongh-103636: issue warning for deprecated calendar constants (python#103833)
  Various small fixes to dis docs (python#103923)
  ...
@carljm
fix handling error from instrumentation call
e32166d
@markshannon
Copy link
Member

What is the super = super assignment testing?
By setting the global variable super to the builtin super, nothing changes.
I think you need to set super to something that isn't super for it to be a useful test.

@carljm
Copy link
Member Author

carljm commented May 9, 2023

What is the super = super assignment testing?
By setting the global variable super to the builtin super, nothing changes.

The compiler checks for statically visible shadowing of the name super, and declines to emit LOAD_SUPER_ATTR at all if shadowing is found. So super = super does change something; it prevents LOAD_SUPER_ATTR from occurring in the bytecode.

This is useful for these tests because it allows me to verify that the monitoring events emitted are identical for LOAD_SUPER_ATTR as they would have been for the old-style LOAD_GLOBAL super; CALL; LOAD_ATTR form.

I think you need to set super to something that isn't super for it to be a useful test.

I don't currently have any tests here in test_monitoring.py that dynamically shadow super (such that we have LOAD_SUPER_ATTR but with a non-standard super). But I don't think such tests are really useful here, since whether super is dynamically shadowed or not doesn't actually change the (non-specialized) implementation of LOAD_SUPER_ATTR at all. (I do have such tests in test_super.py, for checking correctness.)

@carljm
Merge branch 'main' into supermon
c48eee3 
* main: (156 commits)
  pythongh-97696 Add documentation for get_coro() behavior with eager tasks (python#104304)
  pythongh-97933: (PEP 709) inline list/dict/set comprehensions (python#101441)
  pythongh-99889: Fix directory traversal security flaw in uu.decode() (python#104096)
  pythongh-104184: fix building --with-pydebug --enable-pystats (python#104217)
  pythongh-104139: Add itms-services to uses_netloc urllib.parse. (python#104312)
  pythongh-104240: return code unit metadata from codegen (python#104300)
  pythongh-104276: Make `_struct.unpack_iterator` type use type flag instead of custom constructor (python#104277)
  pythongh-97696: Move around and update the whatsnew entry for asyncio eager task factory (python#104298)
  pythongh-103193: Fix refleaks in `test_inspect` and `test_typing` (python#104320)
  require-pr-label.yml: Add missing "permissions:" (python#104309)
  pythongh-90656: Add platform triplets for 64-bit LoongArch (LA64) (python#30939)
  pythongh-104180: Read SOCKS proxies from macOS System Configuration (python#104181)
  pythongh-97696 Remove unnecessary check for eager_start kwarg (python#104188)
  pythonGH-104308: socket.getnameinfo should release the GIL (python#104307)
  pythongh-104310: Add importlib.util.allowing_all_extensions() (pythongh-104311)
  pythongh-99113: A Per-Interpreter GIL! (pythongh-104210)
  pythonGH-104284: Fix documentation gettext build (python#104296)
  pythongh-89550: Buffer GzipFile.write to reduce execution time by ~15% (python#101251)
  pythongh-104223: Fix issues with inheriting from buffer classes (python#104227)
  pythongh-99108: fix typo in Modules/Setup (python#104293)
  ...
@arhadthedev
Copy link
Member

The compiler checks for statically visible shadowing of the name super, and declines to emit LOAD_SUPER_ATTR at all if shadowing is found. So super = super does change something; it prevents LOAD_SUPER_ATTR from occurring in the bytecode.

This is useful for these tests because it allows me to verify that the monitoring events emitted are identical for LOAD_SUPER_ATTR as they would have been for the old-style LOAD_GLOBAL super; CALL; LOAD_ATTR form.

Could you add this as a comment for TestLoadSuperAttr, please (to not repeat it for three assignment = "x = 1" if optimized else "super = super")?

@carljm
Copy link
Member Author

carljm commented May 10, 2023

Could you add this as a comment

Done. I also extracted the "exec code either optimized or not" into a method, so there's now a clear single place to put the comment.

carljm added 3 commits May 10, 2023 10:43
@carljm
don't decref global_super before using it
d084043
@carljm
...and update generated cases
8fb9b52
@carljm
Merge branch 'main' into supermon
19b3d9e 
* main: (27 commits)
  pythongh-87849: fix SEND specialization family definition (pythonGH-104268)
  pythongh-101819: Adapt _io.IOBase.seek and _io.IOBase.truncate to Argument Clinic (python#104384)
  pythongh-101819: Adapt _io._Buffered* methods to Argument Clinic (python#104367)
  pythongh-101819: Refactor `_io` futher in preparation for module isolation (python#104369)
  pythongh-101819: Adapt _io.TextIOBase methods to Argument Clinic (python#104383)
  pythongh-101117: Improve accuracy of sqlite3.Cursor.rowcount docs (python#104287)
  pythonGH-92184: Convert os.altsep to '/' in filenames when creating ZipInfo objects (python#92185)
  pythongh-104357: fix inlined comprehensions that close over iteration var (python#104368)
  pythonGH-90208: Suppress OSError exceptions from `pathlib.Path.glob()` (pythonGH-104141)
  pythonGH-102181: Improve specialization stats for SEND (pythonGH-102182)
  pythongh-103000: Optimise `dataclasses.asdict` for the common case (python#104364)
  pythongh-103538: Remove unused TK_AQUA code (pythonGH-103539)
  pythonGH-87695: Fix OSError from `pathlib.Path.glob()` (pythonGH-104292)
  pythongh-104263: Rely on Py_NAN and introduce Py_INFINITY (pythonGH-104202)
  pythongh-104010: Separate and improve docs for `typing.get_origin` and `typing.get_args` (python#104013)
  pythongh-101819: Adapt _io._BufferedIOBase_Type methods to Argument Clinic (python#104355)
  pythongh-103960: Dark mode: invert image brightness (python#103983)
  pythongh-104252: Immortalize Py_EMPTY_KEYS (pythongh-104253)
  pythongh-101819: Clean up _io windows console io after pythongh-104197 (python#104354)
  pythongh-101819: Harden _io init (python#104352)
  ...
@markshannon
Copy link
Member

How does this interact with #104270?
I also got a fairly large to change monitoring in progress #104387.

I'm inclined to merge those two PRs first, then merge this last. If that's OK with you.

@carljm
Copy link
Member Author

carljm commented May 11, 2023

How does this interact with #104270?

It will certainly cause a merge conflict in generated_cases.c.h, since everything does, but otherwise it should be fine. This is all implemented in the unspecialized opcode, which doesn't change in #104270

I'm inclined to merge those two PRs first, then merge this last. If that's OK with you.

No problem.

@carljm
Merge branch 'main' into supermon
fb47955 
* main:
  pythongh-104057: Fix direct invocation of test_support (pythonGH-104069)
  pythongh-87729: improve hit rate of LOAD_SUPER_ATTR specialization (python#104270)
  pythongh-101819: Fix inverted debug preprocessor check in winconsoleio.c (python#104388)
@carljm carljm linked an issue May 11, 2023 that may be closed by this pull request
LOAD_SUPER_ATTR doesn't match monitoring behavior of the opcodes it replaces #103865
Closed
carljm added 3 commits May 12, 2023 08:04
@carljm
Merge branch 'main' into supermon
8efd7c6 
* main:
  pythongh-91896: Fixup some docs issues following ByteString deprecation (python#104422)
  pythonGH-104371: check return value of calling `mv.release` (python#104417)
  pythongh-104415: Fix refleak tests for `typing.ByteString` deprecation (python#104416)
  pythonGH-86275: Implementation of hypothesis stubs for property-based tests, with zoneinfo tests (python#22863)
  pythonGH-103082: Filter LINE events in VM, to simplify tool implementation. (pythonGH-104387)
  pythongh-93649: Split gc- and allocation tests from _testcapimodule.c (pythonGH-104403)
  pythongh-104389: Add 'unused' keyword to Argument Clinic C converters (python#104390)
  pythongh-101819: Prepare _io._IOBase for module state (python#104386)
  pythongh-104413: Fix refleak when super attribute throws AttributeError (python#104414)
  Fix refleak in `super_descr_get` (python#104408)
  pythongh-87526: Remove dead initialization from _zoneinfo parse_abbr() (python#24700)
  pythongh-91896: Improve visibility of `ByteString` deprecation warnings (python#104294)
  pythongh-104371: Fix calls to `__release_buffer__` while an exception is active (python#104378)
  pythongh-104377: fix cell in comprehension that is free in outer scope (python#104394)
  pythongh-104392: Remove _paramspec_tvars from typing (python#104393)
  pythongh-104396: uuid.py to skip platform check for emscripten and wasi (pythongh-104397)
  pythongh-99108: Refresh HACL* from upstream (python#104401)
  pythongh-104301: Allow leading whitespace in disambiguated pdb statements (python#104342)
@carljm
ensure that our assumptions about the compiler's behavior hold
9de351f
@carljm
Merge branch 'main' into supermon
343867a 
* main: (29 commits)
  pythongh-101819: Fix _io clinic input for unused base class method stubs (python#104418)
  pythongh-101819: Isolate `_io` (python#101948)
  Bump mypy from 1.2.0 to 1.3.0 in /Tools/clinic (python#104501)
  pythongh-104494: Update certain Tkinter pack/place tests for Tk 8.7 errors (python#104495)
  pythongh-104050: Run mypy on `clinic.py` in CI (python#104421)
  pythongh-104490: Consistently define phony make targets (python#104491)
  pythongh-67056: document that registering/unregistering an atexit func from within an atexit func is undefined (python#104473)
  pythongh-104487: PYTHON_FOR_REGEN must be minimum Python 3.10 (python#104488)
  pythongh-101282: move BOLT config after PGO (pythongh-104493)
  pythongh-104469 Convert _testcapi/float.c to use AC (pythongh-104470)
  pythongh-104456: Fix ref leak in _ctypes.COMError (python#104457)
  pythongh-98539: Make _SSLTransportProtocol.abort() safe to call when closed (python#104474)
  pythongh-104337: Clarify random.gammavariate doc entry  (python#104410)
  Minor improvements to typing docs (python#104465)
  pythongh-87092: avoid gcc warning on uninitialized struct field in assemble.c (python#104460)
  pythonGH-71383: IDLE - Document testing subsets of modules (python#104463)
  pythongh-104454: Fix refleak in AttributeError_reduce (python#104455)
  pythongh-75710: IDLE - add docstrings and comments to editor module (python#104446)
  pythongh-91896: Revert some very noisy DeprecationWarnings for `ByteString` (python#104424)
  Add a mention of PYTHONBREAKPOINT to breakpoint() docs (python#104430)
  ...
@carljm
Copy link
Member Author

carljm commented May 15, 2023

@markshannon the other two PRs are merged; shall we go ahead and get this in?

@markshannon
Copy link
Member

Yes.

@markshannon
Copy link
Member

From a monitoring point of view, super() and int() are the same. They are both calls to built in classes.
I don't think your tests cover that.

Can you add a test that the code eval(f'{cls}()') produces the same events for cls = "int" and cls = "super". Likewise for eval(f'{cls}().{meth}()').

carljm added 2 commits May 16, 2023 08:24
@carljm
Merge branch 'main' into supermon
a6dd626 
* main:
  pythonGH-104510: Fix refleaks in `_io` base types (python#104516)
  pythongh-104539: Fix indentation error in logging.config.rst (python#104545)
  pythongh-104050: Don't star-import 'types' in Argument Clinic (python#104543)
  pythongh-104050: Add basic typing to CConverter in clinic.py (python#104538)
  pythongh-64595: Fix write file logic in Argument Clinic (python#104507)
  pythongh-104523: Inline minimal PGO rules (python#104524)
  pythongh-103861: Fix Zip64 extensions not being properly applied in some cases (python#103863)
  pythongh-69152: add method get_proxy_response_headers to HTTPConnection class (python#104248)
  pythongh-103763: Implement PEP 695 (python#103764)
  pythongh-104461: Run tkinter test_configure_screen on X11 only (pythonGH-104462)
  pythongh-104469: Convert _testcapi/watchers.c to use Argument Clinic (python#104503)
  pythongh-104482: Fix error handling bugs in ast.c (python#104483)
  pythongh-104341: Adjust tstate_must_exit() to Respect Interpreter Finalization (pythongh-104437)
  pythonGH-102613: Fix recursion error from `pathlib.Path.glob()` (pythonGH-104373)
@carljm
add test verifying equivalence with int()
b1f064a
markshannon
markshannon approved these changes May 16, 2023
View reviewed changes
Copy link
Member

@markshannon markshannon left a comment

Choose a reason for hiding this comment

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

Looks good, thanks.

@carljm carljm merged commit f40890b into python:main May 16, 2023
@carljm carljm deleted the supermon branch May 16, 2023 16:29
carljm added a commit to carljm/cpython that referenced this pull request May 17, 2023
@carljm
Merge branch 'main' into 709funconly
b1bb3a7 
* main: (26 commits)
  pythonGH-101520: Move tracemalloc functionality into core, leaving interface in Modules. (python#104508)
  typing: Add more tests for TypeVar (python#104571)
  pythongh-104572: Improve error messages for invalid constructs in PEP 695 contexts (python#104573)
  typing: Use PEP 695 syntax in typing.py (python#104553)
  pythongh-102153: Start stripping C0 control and space chars in `urlsplit` (python#102508)
  pythongh-104469: Update README.txt for _testcapi (pythongh-104529)
  pythonGH-103092: isolate `_elementtree` (python#104561)
  pythongh-104050: Add typing to Argument Clinic converters (python#104547)
  pythonGH-103906: Remove immortal refcounting in the interpreter (pythonGH-103909)
  pythongh-87474: Fix file descriptor leaks in subprocess.Popen (python#96351)
  pythonGH-103092: isolate `pyexpat`  (python#104506)
  pythongh-75367: Fix data descriptor detection in inspect.getattr_static (python#104517)
  pythongh-104050: Add more annotations to `Tools/clinic.py` (python#104544)
  pythongh-104555: Fix isinstance() and issubclass() for runtime-checkable protocols that use PEP 695 (python#104556)
  pythongh-103865: add monitoring support to LOAD_SUPER_ATTR (python#103866)
  CODEOWNERS: Assign new PEP 695 files to myself (python#104551)
  pythonGH-104510: Fix refleaks in `_io` base types (python#104516)
  pythongh-104539: Fix indentation error in logging.config.rst (python#104545)
  pythongh-104050: Don't star-import 'types' in Argument Clinic (python#104543)
  pythongh-104050: Add basic typing to CConverter in clinic.py (python#104538)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@musashay23 musashay23 musashay23 approved these changes

@markshannon markshannon markshannon approved these changes

@iritkatriel iritkatriel Awaiting requested review from iritkatriel iritkatriel is a code owner

Assignees
No one assigned
Labels
skip news
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

LOAD_SUPER_ATTR doesn't match monitoring behavior of the opcodes it replaces
6 participants
@carljm @bedevere-bot @JelleZijlstra @markshannon @arhadthedev @musashay23