Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

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

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.

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

Already on GitHub?Sign in to your account

Merged
vstinner merged 21 commits intopython:mainfromsrinivasreddy:gh-129149
Jan 23, 2025

Conversation

@srinivasreddy
Copy link
Contributor

@srinivasreddysrinivasreddy commentedJan 22, 2025•
edited by bedevere-appbot
Loading

@srinivasreddysrinivasreddy changed the titleAdd Missing fast path in PyLong_From*() functions for compact integersgh-129149: Add Missing fast path in PyLong_From*() functions for compact integersJan 22, 2025
@skirpichev

This comment was marked as resolved.

@srinivasreddysrinivasreddy marked this pull request as ready for reviewJanuary 22, 2025 09:06
@skirpichevskirpichev self-requested a reviewJanuary 22, 2025 09:34
srinivasreddyand others added2 commitsJanuary 22, 2025 15:16
…e-129149.wAYu43.rstCo-authored-by: Pieter Eendebak <pieter.eendebak@gmail.com>
…e-129149.wAYu43.rstCo-authored-by: Pieter Eendebak <pieter.eendebak@gmail.com>
Copy link
Contributor

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

Copy link
Member

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

@srinivasreddysrinivasreddy marked this pull request as draftJanuary 23, 2025 06:45
@srinivasreddysrinivasreddy changed the titlegh-129149: Add Missing fast path in PyLong_From*() functions for compact integersgh-129149: Add Missing fast path PYLONG_FROM_UINT macro for compact integersJan 23, 2025
@srinivasreddysrinivasreddy changed the titlegh-129149: Add Missing fast path PYLONG_FROM_UINT macro for compact integersgh-129149: Add Missing fast path in PYLONG_FROM_UINT macro for compact integersJan 23, 2025
@srinivasreddysrinivasreddy marked this pull request as ready for reviewJanuary 23, 2025 06:56
@srinivasreddy
Copy link
ContributorAuthor

srinivasreddy commentedJan 23, 2025•
edited
Loading

Action plan for PRs.

  1. Add Missing fast path in PYLONG_FROM_UINT macro for compact integersgh-129149: Add Missing fast path in PYLONG_FROM_UINT macro for compact integers #129168
  2. RefactorPyLong_FromSsize_t(),PyLong_FromLong() andPyLong_FromLongLong() using a macro similar toPYLONG_FROM_UINT to get rid of the repetitive code.

cc@skirpichev@picnixz

Copy link
Contributor

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

This reverts commitaf410e0.
@srinivasreddy
Copy link
ContributorAuthor

@skirpichev Done.

Copy link
Member

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

…e-129149.wAYu43.rstCo-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
@srinivasreddy
Copy link
ContributorAuthor

@picnixz Done 👍🏾

Copy link
Member

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

Copy link
Member

@vstinnervstinner left a comment

Choose a reason for hiding this comment

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

LGTM

…e-129149.wAYu43.rstCo-authored-by: Victor Stinner <vstinner@python.org>
@vstinnervstinnerenabled auto-merge (squash)January 23, 2025 14:36
@vstinnervstinner merged commitab353b3 intopython:mainJan 23, 2025
41 checks passed
@vstinner
Copy link
Member

Merged, thanks@srinivasreddy!

srinivasreddy reacted with hooray emoji

@srinivasreddysrinivasreddy deleted the gh-129149 branchJanuary 23, 2025 15:49
/* Count the number of Python digits. */ \
Py_ssize_tndigits=0; \
INT_TYPEt= (ival); \
Py_ssize_tndigits=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 theINT_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
ContributorAuthor

Choose a reason for hiding this comment

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

Yes. Absolutely.

Copy link
Member

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 forPYLONG_BITS_IN_DIGIT=15 andlong 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 freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@eendebakpteendebakpteendebakpt left review comments

@chris-eiblchris-eiblchris-eibl left review comments

@vstinnervstinnervstinner approved these changes

@skirpichevskirpichevskirpichev approved these changes

@picnixzpicnixzpicnixz approved these changes

+1 more reviewer

@WolframAlphWolframAlphWolframAlph left review comments

Reviewers whose approvals may not affect merge requirements

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
[8]ページ先頭
©2009-2025 Movatter.jp