gh-127295: ctypes: Switch field accessors to fixed-width integers #127297

encukou · 2024-11-26T15:02:07Z

This refactoring has a miniscule but consistent performance benefit (1.01x geometric mean; edit: 46 cases slower & 415 faster). See my bench script & results. Thanks @vstinner for pyperf and instructions for isolating CPU cores!

For the repetitive parts, this uses a combination of macros and Argument Clinic for code generation (inline, so one can easily inspect the results). This is the most readable/maintainable of various approaches I tried.
(No, I'm not a fan of the giant macros, but it beats both code generated from f-strings that lack syntax highlighting, and external multiple-include files. Oh, and the /////////// lines make it easy to see misplaced backslashes.)

Several related changes are included:

Pass a meaningful size argument, rather than zero, to non-bitfield accessors. (This would be great to have for size-generic functions, and it should be a private implementation detail. I want to do this now and get the asserts in the wild, to see if I missed someone who's relying on the detail.)
Use a switch in _ctypes_get_fielddesc, rather than a linear search. (This requires that there are now several boring chunks with a line for each of the format codes, sbBcdCEFgfhHiIlLqQPzuUZXvO, but Argument Clinic makes this bearable.)
Rearrange struct fielddesc to move all the accessors together, making code generation a bit easier
Motley consistency improvements, like changing BSTR in function names to its code char X, to match the other accessors

Issue: ctypes: Switch field accessors to fixed-width integers #127295

Replace formattable by a switch. Generate some repetitive parts of handling individual C types.

encukou · 2024-11-26T15:06:47Z

(Yes, this is wrong as-is; hopefully due to a silly mistake on my part.)

bedevere-bot · 2024-11-27T08:48:28Z

🤖 New build scheduled with the buildbot fleet by @encukou for commit fcae66b 🤖

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

encukou · 2024-12-06T10:19:34Z

@ZeroIntensity @picnixz @serhiy-storchaka, would one of you be interested in looking at these changes?

picnixz · 2024-12-06T10:40:17Z

I'll look into it but let me first clean-up my backlog (so probably in a few hours or tomorrow)

encukou · 2024-12-06T10:50:32Z

No rush :)

ZeroIntensity · 2024-12-06T12:51:13Z

I'll take a look later today :)

picnixz

A first round of review.

Modules/_ctypes/cfield.c

Co-authored-by: Bénédikt Tran <[email protected]>

ZeroIntensity

This mostly LGTM. My main concern is that this adds some new thread safety issues with global variables, but knowing ctypes, it probably didn't work before anyway.

cc @skirpichev, you might be interested in looking at the integer-related parts of this PR.

Modules/_ctypes/cfield.c

ZeroIntensity · 2024-12-10T13:38:08Z

Modules/_ctypes/cfield.c

 static PyObject *
 g_set(void *ptr, PyObject *value, Py_ssize_t size)
 {
+    assert(NUM_BITS(size) || (size == sizeof(long double)));


This is repeated a lot; is it worth making it its own macro?

I prefer this for now.
One advantage of writing this out is that it's much clearer if you land on this line in a debugger. With this one line, the trade-off isn't worth it.

(I'll be the first to admit that the macros this PR adds are a pain to debug, but, IMO they also remove enough duplication to be worth it.)

encukou · 2024-12-13T11:45:18Z

@serhiy-storchaka, should I wait for your review?

ZeroIntensity

I did one final pass and I'm happy to say that this LGTM.

encukou · 2024-12-16T14:57:43Z

Thank you!

If there are no objections, I plan to merge on Wednesday, and work on the next PR in this area.

skirpichev · 2024-12-17T04:03:29Z

Modules/_ctypes/cfield.c

+        if (PyLong_Check(value)                                               \
+            && PyUnstable_Long_IsCompact((PyLongObject *)value))              \
+        {                                                                     \
+            val = (CTYPE)PyUnstable_Long_CompactValue(                        \
+                      (PyLongObject *)value);                                 \
+        }                                                                     \
+        else {                                                                \
+            Py_ssize_t res = PyLong_AsNativeBytes(                            \


PyLong_AsNativeBytes() already has a quick path for compact values. Did you check performance impact without first if statement?

encukou · 2024-12-18T10:42:44Z

Yes, without the if it's slightly but measurably slower. With the benchmarks in the OP: Geometric mean: 1.02x slower; 526 slower cases, 87 faster, 635 not significant.

(Perhaps that would be worth the simplification, but that's for another PR & discussion.)

bedevere-bot · 2024-12-18T11:48:30Z

🤖 New build scheduled with the buildbot fleet by @encukou for commit 5155293 🤖

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

encukou · 2024-12-20T13:28:32Z

Thank you for the reviews!

…rs (pythonGH-127297) This should be a pure refactoring, without user-visible behaviour changes. Before this change, ctypes uses traditional native C types, usually identified by [`struct` format characters][struct-chars] when a short (and identifier-friendly) name is needed: - `signed char` (`b`) / `unsigned char` (`B`) - `short` (`h`) / `unsigned short` (`h`) - `int` (`i`) / `unsigned int` (`i`) - `long` (`l`) / `unsigned long` (`l`) - `long long` (`q`) / `unsigned long long` (`q`) These map to C99 fixed-width types, which this PR switches to: - - `int8_t`/`uint8_t` - `int16_t`/`uint16_t` - `int32_t`/`uint32_t` - `int64_t`/`uint64_t` The C standard doesn't guarantee that the “traditional” types must map to the fixints. But, [`ctypes` currently requires it][swapdefs], so the assumption won't break anything. By “map” I mean that the *size* of the types matches. The *alignment* requirements might not. This needs to be kept in mind but is not an issue in `ctypes` accessors, which [explicitly handle unaligned memory][memcpy] for the integer types. Note that there are 5 “traditional” C type sizes, but 4 fixed-width ones. Two of the former are functionally identical to one another; which ones they are is platform-specific (e.g. `int`==`long`==`int32_t`.) This means that one of the [current][current-impls-1] [implementations][current-impls-2] is redundant on any given platform. The fixint types are parametrized by the number of bytes/bits, and one bit for signedness. This makes it easier to autogenerate code for them or to write generic macros (though generic API like [`PyLong_AsNativeBytes`][PyLong_AsNativeBytes] is problematic for performance reasons -- especially compared to a `memcpy` with compile-time-constant size). When one has a *different* integer type, determining the corresponding fixint means a `sizeof` and signedness check. This is easier and more robust than the current implementations (see [`wchar_t`][sizeof-wchar_t] or [`_Bool`][sizeof-bool]). [swapdefs]: https://github.com/python/cpython/blob/v3.13.0/Modules/_ctypes/cfield.c#L420-L444 [struct-chars]: https://docs.python.org/3/library/struct.html#format-characters [current-impls-1]: https://github.com/python/cpython/blob/v3.13.0/Modules/_ctypes/cfield.c#L470-L653 [current-impls-2]: https://github.com/python/cpython/blob/v3.13.0/Modules/_ctypes/cfield.c#L703-L944 [memcpy]: https://github.com/python/cpython/blob/v3.13.0/Modules/_ctypes/cfield.c#L613 [PyLong_AsNativeBytes]: https://docs.python.org/3/c-api/long.html#c.PyLong_AsNativeBytes [sizeof-wchar_t]: https://github.com/python/cpython/blob/v3.13.0/Modules/_ctypes/cfield.c#L1547-L1555 [sizeof-bool]: https://github.com/python/cpython/blob/v3.13.0/Modules/_ctypes/cfield.c#L1562-L1572 Co-authored-by: Bénédikt Tran <[email protected]>

encukou added 3 commits November 26, 2024 14:18

Consistently pass the size to getfunc/setfunc

eeb0d4e

Switch to fixed-width integers

e33c929

Replace formattable by a switch. Generate some repetitive parts of handling individual C types.

Remove comment for obsolete idea

a11c0d9

bedevere-app bot mentioned this pull request Nov 26, 2024

ctypes: Switch field accessors to fixed-width integers #127295

Closed

bedevere-app bot added the awaiting core review label Nov 26, 2024

encukou added the skip news label Nov 26, 2024

encukou marked this pull request as draft November 26, 2024 15:03

bedevere-app bot removed the awaiting core review label Nov 26, 2024

encukou added 6 commits November 26, 2024 16:10

Fix the size argument

c69b65f

Re-run Clinic

eb8cf98

Regen Clinic

b9ed727

Specify signedness of the FFI types, don't match the C types

7cbe57c

Avoid compiler warnings

144a3b0

Silence the GCC warning in a more local way. It's a GCC bug.

fcae66b

encukou marked this pull request as ready for review November 27, 2024 08:42

bedevere-app bot added the awaiting core review label Nov 27, 2024

encukou added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Nov 27, 2024

bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Nov 27, 2024

serhiy-storchaka self-requested a review December 6, 2024 10:40

picnixz self-requested a review December 6, 2024 10:40

ZeroIntensity self-requested a review December 6, 2024 12:50

picnixz reviewed Dec 6, 2024

View reviewed changes

Modules/_ctypes/cfield.c Show resolved Hide resolved

Modules/_ctypes/cfield.c Outdated Show resolved Hide resolved

Modules/_ctypes/cfield.c Show resolved Hide resolved

Modules/_ctypes/cfield.c Outdated Show resolved Hide resolved

Modules/_ctypes/cfield.c Outdated Show resolved Hide resolved

encukou and others added 2 commits December 6, 2024 15:44

Apply suggestions from code review

05ba989

Co-authored-by: Bénédikt Tran <[email protected]>

Add parens around macro argument

7c08d60

encukou added 3 commits December 6, 2024 15:45

Fix refcounting for _CTYPES_DEBUG_KEEP builds

50c82be

Consistency in comments. Why not.

7375c09

Merge in the main branch

6e8a4de

ZeroIntensity reviewed Dec 7, 2024

View reviewed changes

Modules/_ctypes/cfield.c Show resolved Hide resolved

Modules/_ctypes/cfield.c Show resolved Hide resolved

Modules/_ctypes/cfield.c Show resolved Hide resolved

encukou added 2 commits December 9, 2024 12:27

Include <stdbool.h> & use bool rather than _Bool

870a038

Use a mutex around _ctypes_init_fielddesc

1b41ace

encukou mentioned this pull request Dec 9, 2024

gh-126937: ctypes: fix TypeError when a field's size is >65535 bytes #126938

Merged

skirpichev self-requested a review December 10, 2024 00:09

encukou mentioned this pull request Dec 11, 2024

[3.13] gh-126937: ctypes: add test for maximum size of a struct field (GH-126938) #127825

Merged

ZeroIntensity reviewed Dec 12, 2024

View reviewed changes

encukou added 2 commits December 13, 2024 11:55

Merge in the main branch

07d7ded

Nitpick: Use true/false for bool

34098c3

ZeroIntensity approved these changes Dec 13, 2024

View reviewed changes

skirpichev reviewed Dec 17, 2024

View reviewed changes

Avoid unaligned load

5155293

encukou added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Dec 18, 2024

bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Dec 18, 2024

Merge branch 'main' into ctypes-fixint

821a7b8

encukou merged commit 78ffba4 into python:main Dec 20, 2024
39 checks passed

bedevere-app bot removed the awaiting core review label Dec 20, 2024

encukou deleted the ctypes-fixint branch December 20, 2024 13:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gh-127295: ctypes: Switch field accessors to fixed-width integers #127297

gh-127295: ctypes: Switch field accessors to fixed-width integers #127297

encukou commented Nov 26, 2024 •

edited

Loading

encukou commented Nov 26, 2024 •

edited

Loading

bedevere-bot commented Nov 27, 2024

encukou commented Dec 6, 2024

picnixz commented Dec 6, 2024

encukou commented Dec 6, 2024

ZeroIntensity commented Dec 6, 2024

picnixz left a comment

ZeroIntensity left a comment

ZeroIntensity Dec 10, 2024

encukou Dec 13, 2024

encukou commented Dec 13, 2024

ZeroIntensity left a comment

encukou commented Dec 16, 2024

skirpichev Dec 17, 2024

encukou commented Dec 18, 2024

bedevere-bot commented Dec 18, 2024

encukou commented Dec 20, 2024

gh-127295: ctypes: Switch field accessors to fixed-width integers #127297

gh-127295: ctypes: Switch field accessors to fixed-width integers #127297

Conversation

encukou commented Nov 26, 2024 • edited Loading

encukou commented Nov 26, 2024 • edited Loading

bedevere-bot commented Nov 27, 2024

encukou commented Dec 6, 2024

picnixz commented Dec 6, 2024

encukou commented Dec 6, 2024

ZeroIntensity commented Dec 6, 2024

picnixz left a comment

Choose a reason for hiding this comment

ZeroIntensity left a comment

Choose a reason for hiding this comment

ZeroIntensity Dec 10, 2024

Choose a reason for hiding this comment

encukou Dec 13, 2024

Choose a reason for hiding this comment

encukou commented Dec 13, 2024

ZeroIntensity left a comment

Choose a reason for hiding this comment

encukou commented Dec 16, 2024

skirpichev Dec 17, 2024

Choose a reason for hiding this comment

encukou commented Dec 18, 2024

bedevere-bot commented Dec 18, 2024

encukou commented Dec 20, 2024

encukou commented Nov 26, 2024 •

edited

Loading

encukou commented Nov 26, 2024 •

edited

Loading