Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

gh-129149: Add Missing fast path in PYLONG_FROM_UINT macro for compact integers #129168

Merged
merged 21 commits into from
Jan 23, 2025
Merged

gh-129149: Add Missing fast path in PYLONG_FROM_UINT macro for compact integers #129168

merged 21 commits into from
Jan 23, 2025

Conversation

srinivasreddy
Copy link
Contributor

@srinivasreddy srinivasreddy commented Jan 22, 2025 •
edited by bedevere-app bot
Loading

@srinivasreddy srinivasreddy changed the title Add Missing fast path in PyLong_From*() functions for compact integers gh-129149: Add Missing fast path in PyLong_From*() functions for compact integers Jan 22, 2025
@bedevere-app bedevere-app bot mentioned this pull request Jan 22, 2025
Open
@skirpichev

This comment was marked as resolved.

Sign in to view
@srinivasreddy srinivasreddy marked this pull request as ready for review January 22, 2025 09:06
@skirpichev skirpichev self-requested a review January 22, 2025 09:34
srinivasreddy and others added 2 commits January 22, 2025 15:16
@srinivasreddy @eendebakpt
Update Misc/NEWS.d/next/Core_and_Builtins/2025-01-22-14-24-44.gh-issu…
c00ba8c 
…e-129149.wAYu43.rst

Co-authored-by: Pieter Eendebak <pieter.eendebak@gmail.com>
@srinivasreddy @eendebakpt
Update Misc/NEWS.d/next/Core_and_Builtins/2025-01-22-14-24-44.gh-issu…
2b5163f 
…e-129149.wAYu43.rst

Co-authored-by: Pieter Eendebak <pieter.eendebak@gmail.com>
WolframAlph
WolframAlph reviewed Jan 22, 2025
View reviewed changes
Objects/longobject.c Outdated Show resolved Hide resolved
skirpichev
skirpichev approved these changes Jan 22, 2025
View reviewed changes
Copy link
Member

@skirpichev skirpichev left a comment

Choose a reason for hiding this comment

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

LGTM, but I second on comment on declarations in *_FROM_SIGNED. Also, I don't think that PYLONG_FROM_UINT should be moved.

Objects/longobject.c Outdated Show resolved Hide resolved
WolframAlph
WolframAlph reviewed Jan 22, 2025
View reviewed changes
Objects/longobject.c Outdated Show resolved Hide resolved
picnixz
picnixz reviewed Jan 22, 2025
View reviewed changes
Copy link
Member

@picnixz picnixz left a comment

Choose a reason for hiding this comment

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

If the code was only moved around and there's no new fast path, I would prefer to do it in a separate PR rather than in this one.

Misc/NEWS.d/next/Core_and_Builtins/2025-01-22-14-24-44.gh-issue-129149.wAYu43.rst Outdated Show resolved Hide resolved
Objects/longobject.c Outdated Show resolved Hide resolved
@srinivasreddy srinivasreddy marked this pull request as draft January 23, 2025 06:45
@srinivasreddy srinivasreddy changed the title gh-129149: Add Missing fast path in PyLong_From*() functions for compact integers gh-129149: Add Missing fast path PYLONG_FROM_UINT macro for compact integers Jan 23, 2025
@srinivasreddy srinivasreddy changed the title gh-129149: Add Missing fast path PYLONG_FROM_UINT macro for compact integers gh-129149: Add Missing fast path in PYLONG_FROM_UINT macro for compact integers Jan 23, 2025
@srinivasreddy srinivasreddy marked this pull request as ready for review January 23, 2025 06:56
@srinivasreddy
Copy link
Contributor Author

srinivasreddy commented Jan 23, 2025 •
edited
Loading

Action plan for PRs.

  1. Add Missing fast path in PYLONG_FROM_UINT macro for compact integers gh-129149: Add Missing fast path in PYLONG_FROM_UINT macro for compact integers #129168
  2. Refactor PyLong_FromSsize_t(), PyLong_FromLong() and PyLong_FromLongLong() using a macro similar to PYLONG_FROM_UINT to get rid of the repetitive code.

cc @skirpichev @picnixz

skirpichev
skirpichev reviewed Jan 23, 2025
View reviewed changes
Copy link
Member

@skirpichev skirpichev left a comment

Choose a reason for hiding this comment

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

No, you misread me. Refactoring change (conversion PyLong_FromLongLong and PyLong_FromLong to macro) doesn't require a news entry. But this change - does.

@srinivasreddy
Revert "Remove blurb"
5a9e201 
This reverts commit af410e0.
@srinivasreddy
Copy link
Contributor Author

@skirpichev Done.

picnixz
picnixz reviewed Jan 23, 2025
View reviewed changes
Copy link
Member

@picnixz picnixz left a comment

Choose a reason for hiding this comment

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

Nit and LGTM.

Misc/NEWS.d/next/Core_and_Builtins/2025-01-22-14-24-44.gh-issue-129149.wAYu43.rst Outdated Show resolved Hide resolved
@srinivasreddy @picnixz
Update Misc/NEWS.d/next/Core_and_Builtins/2025-01-22-14-24-44.gh-issu…
1f81b87 
…e-129149.wAYu43.rst

Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
@srinivasreddy
Copy link
Contributor Author

@picnixz Done 👍🏾

picnixz
picnixz approved these changes Jan 23, 2025
View reviewed changes
Copy link
Member

@picnixz picnixz left a comment

Choose a reason for hiding this comment

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

This looks fine but I'll @vstinner have a look as I'm not very familiar with this performance part.

vstinner
vstinner approved these changes Jan 23, 2025
View reviewed changes
Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

LGTM

Misc/NEWS.d/next/Core_and_Builtins/2025-01-22-14-24-44.gh-issue-129149.wAYu43.rst Outdated Show resolved Hide resolved
@srinivasreddy @vstinner
Update Misc/NEWS.d/next/Core_and_Builtins/2025-01-22-14-24-44.gh-issu…
f4d74d2 
…e-129149.wAYu43.rst

Co-authored-by: Victor Stinner <vstinner@python.org>
@vstinner vstinner enabled auto-merge (squash) January 23, 2025 14:36
@vstinner vstinner merged commit ab353b3 into python:main Jan 23, 2025
41 checks passed
@vstinner
Copy link
Member

Merged, thanks @srinivasreddy!

@srinivasreddy srinivasreddy deleted the gh-129149 branch January 23, 2025 15:49
eendebakpt
eendebakpt reviewed Jan 30, 2025
View reviewed changes
Objects/longobject.c
/* Count the number of Python digits. */ \
Py_ssize_t ndigits = 0; \
INT_TYPE t = (ival); \
Py_ssize_t ndigits = 2; \
Copy link
Contributor

Choose a reason for hiding this comment

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

Depending on the INT_TYPE the value of ndigits can only be 2 or 3. We can replace the while loop with a single if statement. E.g.

main...eendebakpt:cpython:fast_digit_count

@srinivasreddy Would you be interested in benchmarking this approach and making a PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. Absolutely.

Copy link
Contributor

Choose a reason for hiding this comment

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

or 3

I do not think this works on a platform where e.g. long long is 128 bit? Or for PYLONG_BITS_IN_DIGIT=15 and long long being 64bit?
Maybe both hypothetical, but not impossible.

Copy link
Contributor

Choose a reason for hiding this comment

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

Correct. We would need some guards for that. Whether that bit of complexity is worth is, depends on the benchmark results.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eendebakpt eendebakpt eendebakpt left review comments

@WolframAlph WolframAlph WolframAlph left review comments

@chris-eibl chris-eibl chris-eibl left review comments

@vstinner vstinner vstinner approved these changes

@skirpichev skirpichev skirpichev approved these changes

@picnixz picnixz picnixz approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@srinivasreddy @skirpichev @vstinner @eendebakpt @picnixz @WolframAlph @chris-eibl