Skip to content

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

Merged
encukou merged 18 commits into
python:mainfrom
aisk:ctypes-heap-types
Jan 18, 2024
Merged

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

Conversation

@aisk

@aisk aisk commented Jan 1, 2024

Copy link
Copy Markdown
Member

Make the metatypes in _ctypes into heap types.

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

@erlend-aasland erlend-aasland left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

Comment thread Modules/_ctypes/_ctypes.c Outdated
Comment thread Modules/_ctypes/_ctypes.c Outdated
Comment thread Modules/_ctypes/_ctypes.c Outdated
Comment thread Modules/_ctypes/_ctypes.c Outdated
Comment thread Modules/_ctypes/_ctypes.c Outdated
Comment thread Modules/_ctypes/_ctypes.c Outdated
Comment thread Modules/_ctypes/_ctypes.c Outdated
Comment thread Modules/_ctypes/ctypes.h
@erlend-aasland 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
Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>
Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>
Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>
Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>
Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>
Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>
Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>
@erlend-aasland

Copy link
Copy Markdown
Contributor
$ ./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!

@erlend-aasland

Copy link
Copy Markdown
Contributor

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

@aisk

aisk commented Jan 4, 2024

Copy link
Copy Markdown
Member Author

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

@aisk

aisk commented Jan 4, 2024

Copy link
Copy Markdown
Member Author

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?

@erlend-aasland

Copy link
Copy Markdown
Contributor

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.

@aisk

aisk commented Jan 4, 2024

Copy link
Copy Markdown
Member Author

Got it, working on this.

Comment thread Modules/_ctypes/_ctypes.c Outdated
Comment on lines +1529 to +1532
PyCArrayType_traverse(PyTypeObject *self, visitproc visit, void *arg)
{
Py_VISIT(Py_TYPE(self));
return PyType_Type.tp_traverse((PyObject *)self, visit, arg);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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?

@aisk aisk Jan 12, 2024

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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.

@neonene neonene Jan 12, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

@neonene neonene Jan 13, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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!

@neonene

neonene commented Jan 13, 2024

Copy link
Copy Markdown
Contributor

Looks good to me.

@encukou encukou merged commit 8e31cdc into python:main Jan 18, 2024
@erlend-aasland

Copy link
Copy Markdown
Contributor

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

@bedevere-bot

Copy link
Copy Markdown

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

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

What do you need to do:

  1. Don't panic.
  2. Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
  3. 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.
  4. Check if the failure is related to this commit (8e31cdc) or if it is a false positive.
  5. 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)

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

encukou commented Jan 18, 2024

Copy link
Copy Markdown
Member

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

kulikjak pushed a commit to kulikjak/cpython that referenced this pull request Jan 22, 2024
…onGH-113620)


Co-authored-by: Erlend E. Aasland <erlend@python.org>
aisk added a commit to aisk/cpython that referenced this pull request Feb 11, 2024
…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
…onGH-113620)


Co-authored-by: Erlend E. Aasland <erlend@python.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants