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-91052: Add C API for watching dictionaries #31787

Merged
merged 9 commits into from Oct 7, 2022
Merged

gh-91052: Add C API for watching dictionaries #31787

merged 9 commits into from Oct 7, 2022

Conversation

carljm
Copy link
Member

@carljm carljm commented Mar 9, 2022 •
edited

Notes on the PR for reviewers:

  • Since this API is intended for use by extensions, and _Py* APIs are internal-only, I added the API as public (Py*). But I think it should perhaps be CPython-specific, so I put it in Include/cpython/dictobject.h, not Include/dictobject.h. Not sure about any of this, though, so happy to change as requested.

itamaro
itamaro reviewed Mar 9, 2022
View reviewed changes
Objects/dictobject.c Outdated Show resolved Hide resolved
Objects/dictobject.c Outdated Show resolved Hide resolved
@carljm carljm force-pushed the dictwatch branch 3 times, most recently from eed27b4 to d22463b Compare March 14, 2022 23:14
@MaxwellDupre
Copy link
Contributor

Do you have a test harness/or something I can use (without too much effort :)? I could just run the Python test suite but that may be missing the point.

@carljm
Copy link
Member Author

carljm commented May 2, 2022

Do you have a test harness/or something I can use (without too much effort :)? I could just run the Python test suite but that may be missing the point.

Hi! I'm not sure what you are trying to achieve -- can you clarify your goals and how they relate to this PR?

@carljm carljm mentioned this pull request May 3, 2022
Closed
@cpython-cla-bot
Copy link

cpython-cla-bot bot commented Oct 3, 2022 •
edited

All commit authors signed the Contributor License Agreement.
CLA signed

@carljm carljm changed the title bpo-46896: Add C API for watching dictionaries gh-91052: Add C API for watching dictionaries Oct 3, 2022
@carljm
Copy link
Member Author

carljm commented Oct 4, 2022

@markshannon I've updated this PR to the design we discussed, would be glad for your review. I'll try to get some updated pyperformance numbers tomorrow.

@carljm
Copy link
Member Author

carljm commented Oct 4, 2022

Shows neutral PyPerformance results: https://gist.github.com/carljm/fa125fe5147146cd62ae10485b3fc31b

markshannon
markshannon reviewed Oct 5, 2022
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.

Generally looks good. A few comments.

Include/internal/pycore_dict.h Outdated Show resolved Hide resolved
Include/internal/pycore_interp.h Outdated Show resolved Hide resolved
Modules/_testcapimodule.c Outdated Show resolved Hide resolved
Modules/_testcapimodule.c Outdated Show resolved Hide resolved
Modules/_testcapimodule.c Show resolved Hide resolved
@carljm
Merge branch 'main' into dictwatch
4bb5d9d 
* main: (66 commits)
  pythongh-65961: Raise `DeprecationWarning` when `__package__` differs from `__spec__.parent` (python#97879)
  docs(typing): add "see PEP 675" to LiteralString (python#97926)
  pythongh-97850: Remove all known instances of module_repr() (python#97876)
  I changed my surname early this year (python#96671)
  pythongh-93738: Documentation C syntax (:c:type:<C type> -> :c:expr:<C type>) (python#97768)
  pythongh-91539: improve performance of get_proxies_environment  (python#91566)
  build(deps): bump actions/stale from 5 to 6 (python#97701)
  pythonGH-95172 Make the same version `versionadded` oneline (python#95172)
  pythongh-88050: Fix asyncio subprocess to kill process cleanly when process is blocked (python#32073)
  pythongh-93738: Documentation C syntax (Function glob patterns -> literal markup) (python#97774)
  pythongh-93357: Port test cases to IsolatedAsyncioTestCase, part 2 (python#97896)
  pythongh-95196: Disable incorrect pickling of the C implemented classmethod descriptors (pythonGH-96383)
  pythongh-97758: Fix a crash in getpath_joinpath() called without arguments (pythonGH-97759)
  pythongh-74696: Pass root_dir to custom archivers which support it (pythonGH-94251)
  pythongh-97661: Improve accuracy of sqlite3.Cursor.fetchone docs (python#97662)
  pythongh-87092: bring compiler code closer to a preprocessing-opt-assembler organisation (pythonGH-97644)
  pythonGH-96704: Add {Task,Handle}.get_context(), use it in call_exception_handler() (python#96756)
  pythongh-93738: Documentation C syntax (:c:type:`PyTypeObject*` -> :c:expr:`PyTypeObject*`) (python#97778)
  pythongh-97825: fix AttributeError when calling subprocess.check_output(input=None) with encoding or errors args (python#97826)
  Add re.VERBOSE flag documentation example (python#97678)
  ...
@carljm
Address review comments
7dcf9b0
@carljm
Merge branch 'main' into dictwatch
7938d6f 
* main:
  pythonGH-88050: fix race in closing subprocess pipe in asyncio  (python#97951)
  pythongh-93738: Disallow pre-v3 syntax in the C domain (python#97962)
  pythongh-95986: Fix the example using match keyword (python#95989)
  pythongh-97897: Prevent os.mkfifo and os.mknod segfaults with macOS 13 SDK (pythonGH-97944)
  pythongh-94808: Cover `PyUnicode_Count` in CAPI (python#96929)
  pythongh-94808: Cover `PyObject_PyBytes` case with custom `__bytes__` method (python#96610)
  pythongh-95691: Doc BufferedWriter and BufferedReader (python#95703)
  pythonGH-88968: Add notes about socket ownership transfers (python#97936)
  pythongh-96865: [Enum] fix Flag to use CONFORM boundary (pythonGH-97528)
@carljm
Minor cleanups
fb19617
@carljm carljm mentioned this pull request Oct 6, 2022
Merged
markshannon
markshannon reviewed Oct 6, 2022
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.

A few more comments, but looking good.

Doc/c-api/dict.rst Show resolved Hide resolved
Doc/c-api/dict.rst Outdated Show resolved Hide resolved
Doc/c-api/dict.rst Outdated Show resolved Hide resolved
Include/cpython/dictobject.h Outdated Show resolved Hide resolved
@carljm
Merge branch 'main' into dictwatch
e5d2169 
* main:
  fixes pythongh-96078: os.sched_yield release the GIL while calling sched_yield(2). (pythongh-97965)
  pythongh-65961: Do not rely solely on `__cached__` (pythonGH-97990)
  pythongh-97850: Remove the open issues section from the import reference (python#97935)
  Docs: pin sphinx-lint (pythonGH-97992)
  pythongh-94590: add signatures to operator itemgetter, attrgetter, methodcaller (python#94591)
  Add Pynche's move to the What's new in 3.11 (python#97974)
  pythongh-97781: Apply changes from importlib_metadata 5. (pythonGH-97785)
  pythongh-86482: Document assignment expression need for ()s (python#23291)
  pythongh-97943: PyFunction_GetAnnotations should return a borrowed reference. (python#97949)
  pythongh-94808: Coverage: Test that maximum indentation level is handled (python#95926)
@markshannon
Copy link
Member

LGTM, thanks.

@markshannon markshannon merged commit a4b7794 into python:main Oct 7, 2022
@carljm carljm deleted the dictwatch branch October 7, 2022 00:14
@sweeneyde
Copy link
Member

I think this may have introduced some refleaks into test_capi.

@carljm
Copy link
Member Author

carljm commented Oct 7, 2022

Thanks for the heads up, I'll take a look.

@carljm carljm mentioned this pull request Oct 7, 2022
Merged
iritkatriel
iritkatriel reviewed Oct 7, 2022
View reviewed changes
Lib/test/test_capi.py
self.assertIs(unraisable.object, d)
self.assertEqual(str(unraisable.exc_value), "boom!")

def test_two_watchers(self):
Copy link
Member

Choose a reason for hiding this comment

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

This test is ref leaking on my PR: #98001

Copy link
Member

Choose a reason for hiding this comment

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

Ah probably fixed in #98017 ?

@vstinner
Copy link
Member

vstinner commented Oct 7, 2022 •
edited

For the C API documentation changes, you should add .. versionadded:: 3.12 to new functions, and document them in Doc/whatsnew/3.12.rst:
https://docs.python.org/dev/whatsnew/3.12.html#id2

@carljm carljm mentioned this pull request Oct 7, 2022
Merged
@carljm
Copy link
Member Author

carljm commented Oct 7, 2022

Thanks @vstinner , I included these changes in #98055.

carljm added a commit to carljm/cpython that referenced this pull request Oct 8, 2022
@carljm
Merge branch 'main' into typewatch
29e38df 
* main:
  bpo-35540 dataclasses.asdict now supports defaultdict fields (pythongh-32056)
  pythonGH-91052: Add C API for watching dictionaries (pythonGH-31787)
  bpo-38693: Use f-strings instead of str.format() within importlib (python#17058)
mpage pushed a commit to mpage/cpython that referenced this pull request Oct 11, 2022
@carljm @mpage
pythonGH-91052: Add C API for watching dictionaries (pythonGH-31787)
4f9e610
@erlend-aasland erlend-aasland mentioned this pull request Nov 10, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@itamaro itamaro itamaro left review comments

@iritkatriel iritkatriel iritkatriel left review comments

@markshannon markshannon markshannon left review comments

@Fidget-Spinner Fidget-Spinner Fidget-Spinner left review comments

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

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

Successfully merging this pull request may close these issues.

None yet