GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-103092: Convert some `_ctypes` metatypes to heap types by aisk · Pull Request #113620 · python/cpython

Conversation

@aisk

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

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

Make the metatypes in _ctypes into heap types.

  • Issue: Isolate Stdlib Extension Modules #103092

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
aisk added 5 commits December 22, 2023 19:10
@aisk
convert PyCSimpleType_Type to heap type
d5a196d
@aisk
turn all meta type to heap type
78acadf
@aisk
simplify the slots and spec names
799494d
@aisk
update the traverse function
ad3e1ec
@aisk
fix codes ident
55d9fc4
@bedevere-app bedevere-app Bot added the awaiting review label Jan 1, 2024
@bedevere-app bedevere-app Bot mentioned this pull request Jan 1, 2024
Isolate Stdlib Extension Modules #103092
Closed
24 of 25 tasks
@aisk
remove unused CDataType_as_sequence
fc91804
@Eclips4
Eclips4 requested a review from erlend-aasland January 1, 2024 15:52
erlend-aasland
erlend-aasland reviewed Jan 3, 2024
View reviewed changes

@erlend-aasland erlend-aasland left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Modules/_ctypes/_ctypes.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Modules/_ctypes/_ctypes.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Modules/_ctypes/_ctypes.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Modules/_ctypes/_ctypes.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Modules/_ctypes/_ctypes.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Modules/_ctypes/_ctypes.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Modules/_ctypes/_ctypes.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Modules/_ctypes/ctypes.h Resolved

Uh oh!

There was an error while loading. Please reload this page.
@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
@aisk @erlend-aasland
Update Modules/_ctypes/_ctypes.c
d1b9652 
Co-authored-by: Erlend E. Aasland <[email protected]>
@aisk @erlend-aasland
Update Modules/_ctypes/_ctypes.c
5bfea09 
Co-authored-by: Erlend E. Aasland <[email protected]>
@aisk @erlend-aasland
Update Modules/_ctypes/_ctypes.c
3b661b7 
Co-authored-by: Erlend E. Aasland <[email protected]>
@aisk @erlend-aasland
Update Modules/_ctypes/_ctypes.c
748d18c 
Co-authored-by: Erlend E. Aasland <[email protected]>
@aisk @erlend-aasland
Update Modules/_ctypes/_ctypes.c
7744075 
Co-authored-by: Erlend E. Aasland <[email protected]>
@aisk @erlend-aasland
Update Modules/_ctypes/_ctypes.c
abe4c3f 
Co-authored-by: Erlend E. Aasland <[email protected]>
@aisk @erlend-aasland
Update Modules/_ctypes/_ctypes.c
04cf63f 
Co-authored-by: Erlend E. Aasland <[email protected]>
@aisk
re-order the metatypes in ctypes.h
96194a1
@aisk
fix memory leak
1877f7b
@erlend-aasland erlend-aasland added the skip news label Jan 4, 2024
@erlend-aasland

erlend-aasland commented Jan 4, 2024

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!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@erlend-aasland

erlend-aasland commented Jan 4, 2024

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@erlend-aasland

erlend-aasland commented Jan 4, 2024

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@aisk

aisk commented Jan 4, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

Got it, working on this.
erlend-aasland reacted with heart emoji erlend-aasland reacted with rocket emoji
All reactions
  • ❤️ 1 reaction
  • 🚀 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@aisk 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
@erlend-aasland
Merge branch 'main' into ctypes-heap-types
c0b333b
@erlend-aasland
Merge branch 'main' into ctypes-heap-types
bfa771b
@erlend-aasland 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
erlend-aasland requested review from encukou and kumaraditya303 January 9, 2024 21:05
@aisk aisk mentioned this pull request Jan 10, 2024
gh-103092: Isolate ctypes #103612
Closed
5 of 17 tasks
neonene
neonene reviewed Jan 11, 2024
View reviewed changes
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);

@neonene neonene Jan 11, 2024

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@aisk aisk Jan 12, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
neonene reacted with thumbs up emoji neonene reacted with eyes emoji
All reactions
  • 👍 1 reaction
  • 👀 1 reaction

@neonene neonene Jan 12, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@neonene neonene Jan 13, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@aisk aisk Jan 13, 2024

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

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!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@aisk
Using traverse / clear slot function from CDataType
1c09a0c
@neonene

neonene commented Jan 13, 2024

Copy link
Copy Markdown
Contributor

Looks good to me.
aisk reacted with heart emoji
All reactions
  • ❤️ 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
erlend-aasland
erlend-aasland approved these changes Jan 17, 2024
View reviewed changes
@bedevere-app bedevere-app Bot added awaiting merge and removed awaiting review labels Jan 17, 2024
encukou
encukou approved these changes Jan 18, 2024
View reviewed changes
@encukou
encukou merged commit 8e31cdc into python:main Jan 18, 2024
@bedevere-app bedevere-app Bot removed the awaiting merge label Jan 18, 2024
@erlend-aasland

erlend-aasland commented Jan 18, 2024

Copy link
Copy Markdown
Contributor

Thanks for the PR, @aisk. Thanks for the reviews @neonene and @encukou.
aisk reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot

bedevere-bot commented Jan 18, 2024

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)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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...)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
kulikjak pushed a commit to kulikjak/cpython that referenced this pull request Jan 22, 2024
@aisk @erlend-aasland @kulikjak
pythongh-103092: Convert some _ctypes metatypes to heap types (pyth…
a47260e 
…onGH-113620)


Co-authored-by: Erlend E. Aasland <[email protected]>
@vstinner 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
@aisk @erlend-aasland
pythongh-103092: Convert some _ctypes metatypes to heap types (pyth…
075a16c 
…onGH-113620)


Co-authored-by: Erlend E. Aasland <[email protected]>
Glyphack pushed a commit to Glyphack/cpython that referenced this pull request Sep 2, 2024
@aisk @erlend-aasland @Glyphack
pythongh-103092: Convert some _ctypes metatypes to heap types (pyth…
8b48938 
…onGH-113620)


Co-authored-by: Erlend E. Aasland <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@encukou encukou encukou approved these changes
@erlend-aasland erlend-aasland erlend-aasland approved these changes
@kumaraditya303 kumaraditya303 Awaiting requested review from kumaraditya303
+1 more reviewer
@neonene neonene neonene left review comments
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

@aisk @erlend-aasland @neonene @bedevere-bot @encukou
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.