gh-103092: Convert some `_ctypes` metatypes to heap types by aisk · Pull Request #113620 · python/cpython · GitHub

Make the metatypes in _ctypes into heap types.

Issue: Isolate Stdlib Extension Modules #103092

Thanks, this is a good start. First, we should add some tests to Lib/test/test_ctypes/, though, to make sure we're not breaking anything as we proceed:

test the inheritance hierarchy for _ctypes types
test immutability for _ctypes types
test disallow instantiation for relevant _ctypes types

Also, this PR introduces reference leaks; check the traverse/clear/free slots for each new heap type:

$ ./python.exe -m test -R : test_ctypes                                                                                                                                        (clinic/real-warnings *$%)
Using random seed: 4031286472
Raised RLIMIT_NOFILE: 256 -> 1024
0:00:00 load avg: 3.13 Run 1 test sequentially
0:00:00 load avg: 3.13 [1/1] test_ctypes
beginning 9 repetitions
123456789
.........
test_ctypes leaked [494, 494, 494, 494] references, sum=1976
test_ctypes leaked [314, 313, 313, 313] memory blocks, sum=1253
test_ctypes failed (reference leak)

== Tests result: FAILURE ==

1 test failed:
    test_ctypes

Total duration: 6.4 sec
Total tests: run=432 skipped=29
Total test files: run=1/1 failed=1
Result: FAILURE

Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>

$ ./python.exe -m test -R : test_ctypes                                                                                                                                        (clinic/real-warnings *$%)
Using random seed: 4031286472
Raised RLIMIT_NOFILE: 256 -> 1024
0:00:00 load avg: 3.13 Run 1 test sequentially
0:00:00 load avg: 3.13 [1/1] test_ctypes
beginning 9 repetitions
123456789
.........
test_ctypes leaked [494, 494, 494, 494] references, sum=1976
test_ctypes leaked [314, 313, 313, 313] memory blocks, sum=1253
test_ctypes failed (reference leak)

== Tests result: FAILURE ==

1 test failed:
    test_ctypes

Total duration: 6.4 sec
Total tests: run=432 skipped=29
Total test files: run=1/1 failed=1
Result: FAILURE

Fixed by 1877f7b; thanks, @aisk!

I'd really like if we could add tests before proceeding with this; we should do this in a separate PR.

Hi @erlend-aasland, thanks for the review, I'm currently adding the tests.

Sorry, I might not have been clear. Do you mean to add tests in another pull request, and then process this and subsequent pull requests to see if the changes maintain compatibility as before?

Sorry, I might not have been clear. Do you mean to add tests in another pull request, and then process this and subsequent pull requests to see if the changes maintain compatibility as before?

Yes, I would prefer to add the tests in a separate PR first. Then we can proceed with this.

Got it, working on this.

PyCArrayType_Type and PyCSimpleType_Type can use CDataType_clear/traverse, as they now have the GC protocol. If we care about the overhead of unnecessary visits to StgDict->proto (non-container), a comment would be nice?

I checked the CDataType_clear/traverse, and found that the only difference to current implementation, is that the CDataType version will try to convert self to StgDictObject and clear / visit StgDictObject's own fields.

Since PyCArrayType_Type and PyCSimpleType_Type are not StgDictObject, so I think re-use it is not nessasry, and may leads confusion.

Putting your comments into those functions will be helpful when converting the PyCStgDict type. Please check me (a bit different):

1) PyCStructType_Type
2) UnionType_Type
3) PyCPointerType_Type
4) PyCArrayType_Type
5) PyCSimpleType_Type

They are not StgDictObject, but their dict objects in the tp_dict slot are replaced with StgDictObject by using Py_SETREF(). Previously, (4)(5) did not have Py_TPFLAGS_HAVE_GC, since the stgdict->proto member is a non-container type (e.g. str type: "h","i").

Do you think (1)(2)(3) are StgDictObject? They use CDataType's traverse and clear functions.

Off-topic: StgDictObject itself (self->tp_dict) is handled by PyType_Type.tp_*(self). However, current PyCStgDict_Type does not have Py_TPFLAGS_HAVE_GC, so at least a container proto needs to be visited in CDataType_traverse(), taking account for the cache?/clone? of StgDictObject as well?

Ah, sorry, I made a mistake in my first comment. I've understood your idea, and the codes have been updated. Thank you for pointing it out!

Looks good to me.

Thanks for the PR, @aisk. Thanks for the reviews @neonene and @encukou.

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot AMD64 Ubuntu Shared 3.x has failed when building commit 8e31cdc.

What do you need to do:

Don't panic.
Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
Go to the page of the buildbot that failed (https://buildbot.python.org/all/#builders/506/builds/6880) and take a look at the build logs.
Check if the failure is related to this commit (8e31cdc) or if it is a false positive.
If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/all/#builders/506/builds/6880

Summary of the results of the build (if available):

Click to see traceback logs

remote: Enumerating objects: 11, done.        
remote: Counting objects:   9% (1/11)        
remote: Counting objects:  18% (2/11)        
remote: Counting objects:  27% (3/11)        
remote: Counting objects:  36% (4/11)        
remote: Counting objects:  45% (5/11)        
remote: Counting objects:  54% (6/11)        
remote: Counting objects:  63% (7/11)        
remote: Counting objects:  72% (8/11)        
remote: Counting objects:  81% (9/11)        
remote: Counting objects:  90% (10/11)        
remote: Counting objects: 100% (11/11)        
remote: Counting objects: 100% (11/11), done.        
remote: Compressing objects:  16% (1/6)        
remote: Compressing objects:  33% (2/6)        
remote: Compressing objects:  50% (3/6)        
remote: Compressing objects:  66% (4/6)        
remote: Compressing objects:  83% (5/6)        
remote: Compressing objects: 100% (6/6)        
remote: Compressing objects: 100% (6/6), done.        
remote: Total 6 (delta 5), reused 0 (delta 0), pack-reused 0        
From https://github.com/python/cpython
 * branch                  main       -> FETCH_HEAD
Note: switching to '8e31cdc9450b3e644d48954865568e162edad514'.

You are in 'detached HEAD' state. You can look around, make experimental
changes and commit them, and you can discard any commits you make in this
state without impacting any branches by switching back to a branch.

If you want to create a new branch to retain commits you create, you may
do so (now or later) by using -c with the switch command. Example:

  git switch -c <new-branch-name>

Or undo this operation with:

  git switch -

Turn off this advice by setting config variable advice.detachedHead to false

HEAD is now at 8e31cdc945 gh-103092: Convert some `_ctypes` metatypes to heap types (GH-113620)
Switched to and reset branch 'main'

configure: WARNING: pkg-config is missing. Some dependencies may not be detected correctly.

/tmp/ccvaW277.s: Assembler messages:
/tmp/ccvaW277.s: Fatal error: Objects/genobject.o: No space left on device
make: *** [Makefile:2786: Objects/genobject.o] Error 1

find: ‘build’: No such file or directory
find: ‘build’: No such file or directory
find: ‘build’: No such file or directory
find: ‘build’: No such file or directory
make: [Makefile:2876: clean-retain-profile] Error 1 (ignored)

The buildbot fails with "No space left on device" while building.
(It's the second one to do so today...)

…onGH-113620) Co-authored-by: Erlend E. Aasland <erlend@python.org>

aisk added 5 commits December 22, 2023 19:10

convert PyCSimpleType_Type to heap type

d5a196d

turn all meta type to heap type

78acadf

simplify the slots and spec names

799494d

update the traverse function

ad3e1ec

fix codes ident

55d9fc4

bedevere-app Bot added the awaiting review label Jan 1, 2024

bedevere-app Bot mentioned this pull request Jan 1, 2024

Isolate Stdlib Extension Modules #103092

Closed

25 tasks

remove unused CDataType_as_sequence

fc91804

Eclips4 requested a review from erlend-aasland January 1, 2024 15:52

erlend-aasland reviewed Jan 3, 2024

View reviewed changes

erlend-aasland changed the title gh-103092: make metatypes in ctypes into heap types gh-103092: Convert some _ctypes metatypes to heap types Jan 3, 2024

aisk and others added 9 commits January 3, 2024 17:28

Update Modules/_ctypes/_ctypes.c

d1b9652

Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>

Update Modules/_ctypes/_ctypes.c

5bfea09

Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>

Update Modules/_ctypes/_ctypes.c

3b661b7

Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>

Update Modules/_ctypes/_ctypes.c

748d18c

Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>

Update Modules/_ctypes/_ctypes.c

7744075

Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>

Update Modules/_ctypes/_ctypes.c

abe4c3f

Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>

Update Modules/_ctypes/_ctypes.c

04cf63f

Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>

re-order the metatypes in ctypes.h

96194a1

fix memory leak

1877f7b

erlend-aasland added the skip news label Jan 4, 2024

aisk mentioned this pull request Jan 5, 2024

gh-103092: Test _ctypes type hierarchy and features #113727

Merged

erlend-aasland added 2 commits January 9, 2024 18:29

Merge branch 'main' into ctypes-heap-types

c0b333b

Merge branch 'main' into ctypes-heap-types

bfa771b

erlend-aasland mentioned this pull request Jan 9, 2024

gh-103092: Ensure _ctypes.c static types are accessed via global state #113857

Merged

erlend-aasland requested review from encukou and kumaraditya303 January 9, 2024 21:05

aisk mentioned this pull request Jan 10, 2024

gh-103092: Isolate ctypes #103612

Closed

17 tasks

neonene reviewed Jan 11, 2024

View reviewed changes

Using traverse / clear slot function from CDataType

1c09a0c

erlend-aasland approved these changes Jan 17, 2024

View reviewed changes

bedevere-app Bot added awaiting merge and removed awaiting review labels Jan 17, 2024

encukou approved these changes Jan 18, 2024

View reviewed changes

encukou merged commit 8e31cdc into python:main Jan 18, 2024

bedevere-app Bot removed the awaiting merge label Jan 18, 2024

aisk deleted the ctypes-heap-types branch January 18, 2024 15:38

kulikjak pushed a commit to kulikjak/cpython that referenced this pull request Jan 22, 2024

pythongh-103092 : Convert some _ctypes metatypes to heap types (pyth…

a47260e

…onGH-113620) Co-authored-by: Erlend E. Aasland <erlend@python.org>

vstinner mentioned this pull request Jan 22, 2024

Convert _ctypes static types (and the "meta class") to heap types #114314

Closed

aisk added a commit to aisk/cpython that referenced this pull request Feb 11, 2024

pythongh-103092 : Convert some _ctypes metatypes to heap types (pyth…

075a16c

…onGH-113620) Co-authored-by: Erlend E. Aasland <erlend@python.org>

Glyphack pushed a commit to Glyphack/cpython that referenced this pull request Sep 2, 2024

pythongh-103092 : Convert some _ctypes metatypes to heap types (pyth…

8b48938

…onGH-113620) Co-authored-by: Erlend E. Aasland <erlend@python.org>

+              PyCArrayType_traverse(PyTypeObject *self, visitproc visit, void *arg)
+              {
+                  Py_VISIT(Py_TYPE(self));
+                  return PyType_Type.tp_traverse((PyObject *)self, visit, arg);

gh-103092: Convert some `_ctypes` metatypes to heap types by aisk · Pull Request #113620 · python/cpython · GitHub

Uh oh!

Uh oh!

gh-103092: Convert some _ctypes metatypes to heap types - #113620

gh-103092: Convert some _ctypes metatypes to heap types#113620encukou merged 18 commits intopython:mainpython/cpython:mainfrom aisk:ctypes-heap-typesaisk/cpython:ctypes-heap-typesCopy head branch name to clipboard

Conversation

aisk commented Jan 1, 2024 • edited by bedevere-app Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

erlend-aasland left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

erlend-aasland commented Jan 4, 2024

Uh oh!

erlend-aasland commented Jan 4, 2024

Uh oh!

aisk commented Jan 4, 2024

Uh oh!

aisk commented Jan 4, 2024

Uh oh!

erlend-aasland commented Jan 4, 2024

Uh oh!

aisk commented Jan 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

neonene Jan 11, 2024

Choose a reason for hiding this comment

Uh oh!

aisk Jan 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

neonene Jan 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

neonene Jan 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aisk Jan 13, 2024

Choose a reason for hiding this comment

Uh oh!

neonene commented Jan 13, 2024

Uh oh!

erlend-aasland commented Jan 18, 2024

Uh oh!

bedevere-bot commented Jan 18, 2024

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Uh oh!

encukou commented Jan 18, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

gh-103092: Convert some _ctypes metatypes to heap types#113620
encukou merged 18 commits into
python:mainfrom
aisk:ctypes-heap-types

aisk commented Jan 1, 2024 •
edited by bedevere-app Bot
Loading

aisk commented Jan 4, 2024 •
edited
Loading

aisk Jan 12, 2024 •
edited
Loading

neonene Jan 12, 2024 •
edited
Loading

neonene Jan 13, 2024 •
edited
Loading