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-90667: Add specializations of Py_DECREF when types are known by sweeneyde · Pull Request #30872 · python/cpython

Conversation

@sweeneyde

@sweeneyde sweeneyde commented Jan 25, 2022 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

#90667

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@sweeneyde
Add specializations of Py_DECREF when types are known
ffe7d7b
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Jan 25, 2022
@bedevere-bot bedevere-bot added the awaiting review label Jan 25, 2022
@blurb-it
📜🤖 Added by blurb_it.
d2d7b6f
@sweeneyde
sweeneyde marked this pull request as ready for review January 25, 2022 05:40
@sweeneyde
sweeneyde requested a review from markshannon as a code owner January 25, 2022 05:40
@sweeneyde

sweeneyde commented Jan 25, 2022 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member Author

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

Sorry, something went wrong.

Uh oh!

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

sweeneyde commented Jan 25, 2022

Copy link
Copy Markdown
Member Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
JelleZijlstra
JelleZijlstra approved these changes Jan 25, 2022
View reviewed changes

@JelleZijlstra JelleZijlstra left a comment

Copy link
Copy Markdown
Member

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
gpshead reacted with heart emoji
All reactions
  • ❤️ 1 reaction
@bedevere-bot bedevere-bot added awaiting core review and removed awaiting review labels Jan 25, 2022
@sweeneyde

This comment has been minimized.

Sign in to view
@sweeneyde sweeneyde added the DO-NOT-MERGE label Jan 25, 2022
@sweeneyde

This comment has been minimized.

Sign in to view
@sweeneyde
Strings should be freed with unicode_dealloc, not PyObject_Free!
5cb77fd
@sweeneyde sweeneyde removed the DO-NOT-MERGE label Jan 25, 2022
markshannon
markshannon reviewed Jan 25, 2022
View reviewed changes
Comment thread Include/object.h Resolved

Uh oh!

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

markshannon commented Jan 25, 2022

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
sweeneyde added 2 commits January 25, 2022 13:14
@sweeneyde
_Py_DECREF_IMMORTAL
f58ebcf
@sweeneyde
Remove a layer of macros. Retain _Py_DECREF_INT for private use in lo…
0046cd7 
…ngobject.c
neonene
neonene reviewed Jan 26, 2022
View reviewed changes
Comment thread Include/object.h Resolved

Uh oh!

There was an error while loading. Please reload this page.
markshannon
markshannon reviewed Jan 26, 2022
View reviewed changes
Comment thread Include/internal/pycore_floatobject.h Resolved

Uh oh!

There was an error while loading. Please reload this page.
markshannon
markshannon reviewed Jan 26, 2022
View reviewed changes
Comment thread Include/internal/pycore_long.h Resolved

Uh oh!

There was an error while loading. Please reload this page.
sweeneyde added 2 commits January 26, 2022 15:41
@sweeneyde
Hide more functions
8edfa4d
@sweeneyde
Use PyAPI_FUNC again
d095c43
markshannon
markshannon reviewed Jan 27, 2022
View reviewed changes
Comment thread Python/ceval.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
sweeneyde added 2 commits January 27, 2022 15:20
@sweeneyde
Use PyObject* to avoid casts
fa6a26a
@sweeneyde
Remove PyAPI_FUNC and mathmodule optimizations
35c0d25
markshannon
markshannon reviewed Jan 28, 2022
View reviewed changes
Comment thread Include/internal/pycore_object.h Outdated
#ifdef Py_DEBUG
if (op->ob_refcnt <= 0) {
// Calls _Py_FatalRefcountError for None, True, and False
_Py_Dealloc(op);

@markshannon markshannon Jan 28, 2022

Copy link
Copy Markdown
Member

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.

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.

Sorry, something went wrong.

Uh oh!

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

@sweeneyde sweeneyde Jan 28, 2022

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@sweeneyde
Use _Py_FataRefcountError() directly. Move it to the same header file.
feebdb3
neonene
neonene reviewed Jan 29, 2022
View reviewed changes
Comment thread Python/bltinmodule.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
sweeneyde added 3 commits January 28, 2022 23:48
@sweeneyde
remove (destructor) cast in bltinmodule and longobject
05b050b
@sweeneyde
Add newline at end of NEWS entry
0d6a598
@sweeneyde
Merge remote-tracking branch 'upstream/main' into special_decref
731f79d
@sweeneyde
sweeneyde requested a review from markshannon February 11, 2022 18:19
sweeneyde added 3 commits February 24, 2022 01:27
@sweeneyde
Merge with main: resolve boolobject.c includes
d5f49b2
@sweeneyde
merge with main
b331bf8
@sweeneyde
naming: IMMORTAL --> NO_DEALLOC
ad146ef
@sweeneyde 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 sweeneyde added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Apr 12, 2022
@bedevere-bot

bedevere-bot commented Apr 12, 2022

Copy link
Copy Markdown

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Apr 12, 2022
@sweeneyde

sweeneyde commented Apr 12, 2022

Copy link
Copy Markdown
Member Author

@markshannon would you mind reviewing this again? Thanks
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@sweeneyde
mrege with main
ec763e6
markshannon
markshannon reviewed Apr 19, 2022
View reviewed changes

@markshannon markshannon left a comment

Copy link
Copy Markdown
Member

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'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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Include/internal/pycore_object.h
const char *func,
const char *message);

#define _Py_FatalRefcountError(message) _Py_FatalRefcountErrorFunc(__func__, message)

@markshannon markshannon Apr 19, 2022

Copy link
Copy Markdown
Member

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.

OOI, why move this?

Sorry, something went wrong.

Uh oh!

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

@sweeneyde sweeneyde Apr 19, 2022

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
markshannon reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
Comment thread Objects/longobject.c

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

@markshannon markshannon Apr 19, 2022

Copy link
Copy Markdown
Member

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.

Isn't this ultimately equivalent to

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

?

Sorry, something went wrong.

Uh oh!

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

@sweeneyde sweeneyde Apr 19, 2022

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 removed _PyLongExactDealloc, but I'd like to keep the assertion around to make sure we're never accidentally calling this on int subclasses.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
markshannon reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
Comment thread Objects/longobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
@sweeneyde
Remove _PyLong_ExactDealloc
3dd308a
@markshannon
markshannon merged commit da6c785 into python:main Apr 19, 2022
@bedevere-bot bedevere-bot removed the awaiting core review label Apr 19, 2022
@sweeneyde
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 erlend-aasland mentioned this pull request Apr 24, 2022
gh-90667: fix incompatible pointer type warnings #91822
Closed
@pablogsal pablogsal mentioned this pull request Oct 18, 2024
_Py_DECREF_SPECIALIZED doesn't respect pymalloc tracking #125703
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@markshannon markshannon markshannon left review comments
@JelleZijlstra JelleZijlstra JelleZijlstra approved these changes
+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

None yet

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.

6 participants

@sweeneyde @markshannon @bedevere-bot @JelleZijlstra @neonene @the-knights-who-say-ni
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.