gh-90667: Add specializations of Py_DECREF when types are known by sweeneyde · Pull Request #30872 · python/cpython

sweeneyde · 2022-01-25T05:27:23Z

#90667

sweeneyde · 2022-01-25T05:42:13Z

I tried to only modify hot-ish (not error path) code where DECREF is likely to take the deallocation branch

sweeneyde · 2022-01-25T05:42:48Z

Slower (7):

json_dumps: 12.8 ms +- 0.2 ms -> 13.1 ms +- 0.3 ms: 1.02x slower
meteor_contest: 106 ms +- 2 ms -> 109 ms +- 3 ms: 1.02x slower
telco: 6.50 ms +- 0.17 ms -> 6.58 ms +- 0.18 ms: 1.01x slower
fannkuch: 383 ms +- 5 ms -> 388 ms +- 8 ms: 1.01x slower
regex_compile: 143 ms +- 2 ms -> 145 ms +- 4 ms: 1.01x slower
mako: 11.1 ms +- 0.1 ms -> 11.2 ms +- 0.2 ms: 1.01x slower
chameleon: 7.08 ms +- 0.07 ms -> 7.12 ms +- 0.10 ms: 1.01x slower

Faster (27):

unpack_sequence: 45.9 ns +- 1.1 ns -> 41.6 ns +- 1.0 ns: 1.10x faster
logging_silent: 108 ns +- 11 ns -> 101 ns +- 3 ns: 1.06x faster
nbody: 95.6 ms +- 3.2 ms -> 90.2 ms +- 1.9 ms: 1.06x faster
spectral_norm: 98.3 ms +- 2.3 ms -> 92.8 ms +- 1.6 ms: 1.06x faster
regex_dna: 202 ms +- 3 ms -> 194 ms +- 3 ms: 1.04x faster
scimark_fft: 342 ms +- 12 ms -> 331 ms +- 7 ms: 1.03x faster
crypto_pyaes: 89.6 ms +- 1.7 ms -> 86.8 ms +- 1.1 ms: 1.03x faster
json_loads: 27.4 us +- 0.9 us -> 26.5 us +- 1.3 us: 1.03x faster
scimark_monte_carlo: 69.3 ms +- 1.5 ms -> 67.3 ms +- 1.2 ms: 1.03x faster
pickle_list: 4.62 us +- 0.21 us -> 4.51 us +- 0.15 us: 1.02x faster
scimark_sparse_mat_mult: 5.14 ms +- 0.21 ms -> 5.02 ms +- 0.18 ms: 1.02x faster
xml_etree_parse: 161 ms +- 5 ms -> 157 ms +- 6 ms: 1.02x faster
regex_effbot: 3.07 ms +- 0.05 ms -> 3.00 ms +- 0.07 ms: 1.02x faster
deltablue: 4.36 ms +- 0.14 ms -> 4.27 ms +- 0.14 ms: 1.02x faster
pickle_pure_python: 343 us +- 6 us -> 335 us +- 8 us: 1.02x faster
sqlite_synth: 2.60 us +- 0.06 us -> 2.55 us +- 0.04 us: 1.02x faster
xml_etree_iterparse: 110 ms +- 2 ms -> 108 ms +- 2 ms: 1.02x faster
go: 146 ms +- 2 ms -> 143 ms +- 3 ms: 1.02x faster
pathlib: 20.2 ms +- 0.5 ms -> 19.8 ms +- 0.3 ms: 1.02x faster
scimark_sor: 117 ms +- 3 ms -> 115 ms +- 2 ms: 1.02x faster
dulwich_log: 80.9 ms +- 2.0 ms -> 79.6 ms +- 1.7 ms: 1.02x faster
nqueens: 84.4 ms +- 1.7 ms -> 83.1 ms +- 2.0 ms: 1.02x faster
python_startup: 8.84 ms +- 0.07 ms -> 8.76 ms +- 0.07 ms: 1.01x faster
2to3: 269 ms +- 4 ms -> 266 ms +- 3 ms: 1.01x faster
float: 77.0 ms +- 1.2 ms -> 76.5 ms +- 1.5 ms: 1.01x faster
sympy_integrate: 22.7 ms +- 0.3 ms -> 22.5 ms +- 0.2 ms: 1.01x faster
xml_etree_process: 55.7 ms +- 0.7 ms -> 55.4 ms +- 0.6 ms: 1.01x faster

Benchmark hidden because not significant (24): chaos, django_template, hexiom, logging_format, logging_simple, pickle, pickle_dict, pidigits, pyflate, python_startup_no_site, raytrace, regex_v8, richards, scimark_lu, sqlalchemy_declarative, sqlalchemy_imperative, sympy_expand, sympy_sum, sympy_str, tornado_http, unpickle, unpickle_list, unpickle_pure_python, xml_etree_generate

Geometric mean: 1.01x faster

JelleZijlstra

Interesting idea. I read through most of the math and ceval module changes to verify the types are right.

Include/object.h

markshannon · 2022-01-25T14:08:04Z

Thanks. Another nice little speedup.

The specialized for of Py_DECREF for bool and None are basically the same. Maybe replace them with Py_DECREF_IMMORTAL?

For the other classes, rather than the addition level of macros, I think it would be more readable to use _Py_DECREF_SPECIALIZED directly. Each extra macro means more jumping around the code when reading it.

…ngobject.c

Include/object.h

Include/internal/pycore_floatobject.h

Include/internal/pycore_long.h

Python/ceval.c

markshannon · 2022-01-28T12:24:30Z

Include/internal/pycore_object.h

+#ifdef Py_DEBUG
+    if (op->ob_refcnt <= 0) {
+        // Calls _Py_FatalRefcountError for None, True, and False
+        _Py_Dealloc(op);


Can you change this to call _Py_FatalRefcountError directly?
We might want to use _Py_DECREF_IMMORTAL for other "immortal" objects like 0 or () and there is no guarantee that their de-allocation functions will call _Py_FatalRefcountError.

Done. I moved _Py_FatalRefcountError from pycore_pyerrors to pycore_object as well, rather than having an #include-dependence between the pycore_files.

Python/bltinmodule.c

bedevere-bot · 2022-04-12T05:53:01Z

🤖 New build scheduled with the buildbot fleet by @sweeneyde for commit ad146ef 🤖

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

sweeneyde · 2022-04-12T07:20:09Z

@markshannon would you mind reviewing this again? Thanks

markshannon

I've just benchmarked this and I too see a 1% speedup.

I'm a bit wary of making of making this change, because it feels like we're adding more code for what should be data.
IMO, we should be declaring the shape of objects, not adding more special case code.
But that will have to wait until 3.12.

For 3.11, pragmatism beats purity and we'll take the speedup.

markshannon · 2022-04-19T13:25:08Z

Include/internal/pycore_object.h

+    const char *func,
+    const char *message);
+
+#define _Py_FatalRefcountError(message) _Py_FatalRefcountErrorFunc(__func__, message)


OOI, why move this?

I was trying to avoid a dependence between the pycore_... includes. The unicodeobject.c and tupleobject.c implementations both only used pycore_pyerrors for _Py_FatalRefcountError(), which doesn't really concern itself with the implementation details of exceptions.

markshannon · 2022-04-19T13:42:12Z

Objects/longobject.c


-static inline int is_medium_int(stwodigits x)
+static inline void
+_Py_DECREF_INT(PyLongObject *op)


Isn't this ultimately equivalent to

#define _Py_DECREF_INT(op) _Py_DECREF_SPECIALIZED(op, PyObject_Free)

?

I removed _PyLongExactDealloc, but I'd like to keep the assertion around to make sure we're never accidentally calling this on int subclasses.

Objects/longobject.c

Add specializations of Py_DECREF when types are known

ffe7d7b

the-knights-who-say-ni added the CLA signed label Jan 25, 2022

bedevere-bot added the awaiting review label Jan 25, 2022

📜🤖 Added by blurb_it.

d2d7b6f

sweeneyde marked this pull request as ready for review January 25, 2022 05:40

sweeneyde requested a review from markshannon as a code owner January 25, 2022 05:40

JelleZijlstra approved these changes Jan 25, 2022

View reviewed changes

bedevere-bot added awaiting core review and removed awaiting review labels Jan 25, 2022

This comment has been minimized.

Sign in to view

sweeneyde added the DO-NOT-MERGE label Jan 25, 2022

This comment has been minimized.

Sign in to view

Strings should be freed with unicode_dealloc, not PyObject_Free!

5cb77fd

sweeneyde removed the DO-NOT-MERGE label Jan 25, 2022

markshannon reviewed Jan 25, 2022

View reviewed changes

Include/object.h Outdated Show resolved Hide resolved

sweeneyde added 2 commits January 25, 2022 13:14

_Py_DECREF_IMMORTAL

f58ebcf

Remove a layer of macros. Retain _Py_DECREF_INT for private use in lo…

0046cd7

…ngobject.c

neonene reviewed Jan 26, 2022

View reviewed changes

Include/object.h Outdated Show resolved Hide resolved

markshannon reviewed Jan 26, 2022

View reviewed changes

Include/internal/pycore_floatobject.h Outdated Show resolved Hide resolved

markshannon reviewed Jan 26, 2022

View reviewed changes

Include/internal/pycore_long.h Outdated Show resolved Hide resolved

sweeneyde added 2 commits January 26, 2022 15:41

Hide more functions

8edfa4d

Use PyAPI_FUNC again

d095c43

markshannon reviewed Jan 27, 2022

View reviewed changes

Python/ceval.c Outdated Show resolved Hide resolved

sweeneyde added 2 commits January 27, 2022 15:20

Use PyObject* to avoid casts

fa6a26a

Remove PyAPI_FUNC and mathmodule optimizations

35c0d25

markshannon reviewed Jan 28, 2022

View reviewed changes

Use _Py_FataRefcountError() directly. Move it to the same header file.

feebdb3

neonene reviewed Jan 29, 2022

View reviewed changes

Python/bltinmodule.c Outdated Show resolved Hide resolved

sweeneyde added 3 commits January 28, 2022 23:48

remove (destructor) cast in bltinmodule and longobject

05b050b

Add newline at end of NEWS entry

0d6a598

Merge remote-tracking branch 'upstream/main' into special_decref

731f79d

sweeneyde requested a review from markshannon February 11, 2022 18:19

sweeneyde added 3 commits February 24, 2022 01:27

Merge with main: resolve boolobject.c includes

d5f49b2

merge with main

b331bf8

naming: IMMORTAL --> NO_DEALLOC

ad146ef

sweeneyde changed the title ~~bpo-46509: Add specializations of Py_DECREF when types are known~~ gh-90667: Add specializations of Py_DECREF when types are known Apr 12, 2022

sweeneyde added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Apr 12, 2022

bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Apr 12, 2022

mrege with main

ec763e6

markshannon reviewed Apr 19, 2022

View reviewed changes

Remove _PyLong_ExactDealloc

3dd308a

markshannon merged commit da6c785 into python:main Apr 19, 2022

bedevere-bot removed the awaiting core review label Apr 19, 2022

sweeneyde deleted the special_decref branch April 19, 2022 19:05

This was referenced Apr 19, 2022

gh-90667: Cast to destructor to fix warnings #91711

Merged

Type-specialized Py_DECREF #90667

Closed

erlend-aasland mentioned this pull request Apr 24, 2022

gh-90667: fix incompatible pointer type warnings #91822

Closed

pablogsal mentioned this pull request Oct 18, 2024

_Py_DECREF_SPECIALIZED doesn't respect pymalloc tracking #125703

Closed

Uh oh!

Conversation

sweeneyde commented Jan 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sweeneyde commented Jan 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sweeneyde commented Jan 25, 2022

Uh oh!

JelleZijlstra left a comment

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

This comment has been minimized.

Uh oh!

markshannon commented Jan 25, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

markshannon Jan 28, 2022

Choose a reason for hiding this comment

Uh oh!

sweeneyde Jan 28, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bedevere-bot commented Apr 12, 2022

Uh oh!

sweeneyde commented Apr 12, 2022

Uh oh!

markshannon left a comment

Choose a reason for hiding this comment

Uh oh!

markshannon Apr 19, 2022

Choose a reason for hiding this comment

Uh oh!

sweeneyde Apr 19, 2022

Choose a reason for hiding this comment

Uh oh!

markshannon Apr 19, 2022

Choose a reason for hiding this comment

Uh oh!

sweeneyde Apr 19, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

sweeneyde commented Jan 25, 2022 •

edited

Loading

sweeneyde commented Jan 25, 2022 •

edited

Loading