X Tutup
The Wayback Machine - https://web.archive.org/web/20221027092057/https://github.com/python/cpython/pull/27262
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.

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

bpo-44353: Implement typing.NewType __call__ method in C #27262

Merged
merged 17 commits into from Jul 22, 2021
Merged

bpo-44353: Implement typing.NewType __call__ method in C #27262

merged 17 commits into from Jul 22, 2021
+137 −16

Conversation

uriyyo
Copy link
Member

@uriyyo uriyyo commented Jul 20, 2021 •
edited by bedevere-bot

@uriyyo
Copy link
Member Author

uriyyo commented Jul 20, 2021

@Fidget-Spinner Could you please review this PR?

JelleZijlstra
JelleZijlstra reviewed Jul 20, 2021
View changes
Modules/_functoolsmodule.c Outdated Show resolved Hide resolved
@uriyyo uriyyo requested a review from JelleZijlstra Jul 20, 2021
Fidget-Spinner
Fidget-Spinner reviewed Jul 20, 2021
View changes
Copy link
Member

@Fidget-Spinner Fidget-Spinner left a comment

LGTM, but maybe we should wait for the functools maintainers in case they don't want it in that module.

Also, thanks a bunch for reviewing this too Jelle :). It's nice to have more reviewers around for typing.

Modules/_functoolsmodule.c Outdated Show resolved Hide resolved
Misc/NEWS.d/next/Library/2021-07-20-18-34-16.bpo-44353.ATuYq4.rst Outdated Show resolved Hide resolved
uriyyo and others added 3 commits Jul 20, 2021
@uriyyo uriyyo requested a review from Fidget-Spinner Jul 20, 2021
@Fidget-Spinner Fidget-Spinner added the type-feature A feature request or enhancement label Jul 20, 2021
@JelleZijlstra
Copy link
Member

JelleZijlstra commented Jul 20, 2021

@Fidget-Spinner feel free to request my review on other typing PRs!

I don't have a strong opinion on where to put the helper function. I feel like operator actually makes the most sense.

@uriyyo
Copy link
Member Author

uriyyo commented Jul 20, 2021

@JelleZijlstra Yup, I agree - operator is better place for such helper

@uriyyo uriyyo requested a review from JelleZijlstra Jul 20, 2021
@Fidget-Spinner Fidget-Spinner removed the request for review from rhettinger Jul 21, 2021
Fidget-Spinner
Fidget-Spinner approved these changes Jul 21, 2021
View changes
Copy link
Member

@Fidget-Spinner Fidget-Spinner left a comment

LG. I removed Raymond from the reviewer list since this doesn't touch functools anymore.

dlax
dlax reviewed Jul 21, 2021
View changes
Modules/_operator.c Outdated Show resolved Hide resolved
dlax
dlax reviewed Jul 21, 2021
View changes
Modules/_operator.c Outdated Show resolved Hide resolved
Modules/_operator.c Outdated Show resolved Hide resolved
@uriyyo @dlax
Update Modules/_operator.c
ceea1bc 
Co-authored-by: Denis Laxalde <denis@laxalde.org>
@uriyyo
Copy link
Member Author

uriyyo commented Jul 22, 2021

I have made the requested changes; please review again

@bedevere-bot
Copy link

bedevere-bot commented Jul 22, 2021

Thanks for making the requested changes!

@corona10: please review the changes made to this pull request.

@bedevere-bot bedevere-bot requested a review from corona10 Jul 22, 2021
@Fidget-Spinner
Copy link
Member

Fidget-Spinner commented Jul 22, 2021 •
edited

@Fidget-Spinner do we need backport it to python 3.10?

Probably not. Usually only doc/bugfixes can be backported. Performance improvements are usually not backported, unless the old version has performance that was so bad it caused a security vulnerability/bug/crash (e.g. regex DOS). We are too late into the 3.10 dev cycle to backport an improvement.

Also, thinking abit more, @corona10 is right about adding a separate module for this. I know I've said before that we should be conservative about adding more typing stuff in C, but if it's a small simple optimization like the current PR (which doesn't break compatibility with things like PyPy), I'm +1. In case we need to add more things in the future, it's better to create a new module now and not pollute the other modules.

corona10
corona10 requested changes Jul 22, 2021
View changes
Copy link
Member

@corona10 corona10 left a comment

Once you added the accelerated module, you have to carefully manage both versions.
To manage the identical implementation within different languages, you should update unit tests to run both versions.

Please refer how other modules did.

cpython/Lib/test/test_statistics.py

Line 182 in c05a790

py_statistics = import_helper.import_fresh_module('statistics',

@bedevere-bot
Copy link

bedevere-bot commented Jul 22, 2021

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@uriyyo
Copy link
Member Author

uriyyo commented Jul 22, 2021

I have made the requested changes; please review again

@bedevere-bot
Copy link

bedevere-bot commented Jul 22, 2021

Thanks for making the requested changes!

@corona10: please review the changes made to this pull request.

@bedevere-bot bedevere-bot requested a review from corona10 Jul 22, 2021
corona10
corona10 approved these changes Jul 22, 2021
View changes
Copy link
Member

@corona10 corona10 left a comment

LGTM about module architecture.
Now I am turning over reviews to typing module experts

@ambv ambv merged commit 96c4cbd into python:main Jul 22, 2021
13 checks passed
@bedevere-bot
Copy link

bedevere-bot commented Jul 22, 2021

@ambv: Please replace # with GH- in the commit message next time. Thanks!

@ambv
Copy link
Contributor

ambv commented Jul 22, 2021

Thanks, @uriyyo!

@pablogsal
Copy link
Member

pablogsal commented Jul 23, 2021

This PR was introduced reference leaks and currently all refleak buildbots are red:

Example:

https://buildbot.python.org/all/#/builders/205/builds/102/steps/5/logs/stdio

Ran 367 tests in 0.198s
OK (skipped=1)
.
test_typing leaked [220, 220, 220] references, sum=660
test_typing leaked [69, 68, 68] memory blocks, sum=205
1 test failed again:
    test_typing
== Tests result: FAILURE then FAILURE ==

Per our buildbot policy, if a fix is not performed in 24 h we will need to revert

@uriyyo
Copy link
Member Author

uriyyo commented Jul 23, 2021 •
edited

@pablogsal Looks like this because of this line

cpython/Modules/_typingmodule.c

Line 26 in 17575f7

Py_INCREF(x);

I will open PR to fix it.

UPD: that no problem of a memory leak

UPD: Root cause of memory leak - #27305 (comment)

@uriyyo uriyyo mentioned this pull request Jul 23, 2021
Merged
ambv pushed a commit that referenced this pull request Jul 23, 2021
@uriyyo
bpo-44353: Fix memory leak introduced by GH-27262 (GH-27305)
8f42106
Reviewers

@dlax dlax

@corona10 corona10

@Fidget-Spinner Fidget-Spinner

@gvanrossum gvanrossum

@JelleZijlstra JelleZijlstra

@python/windows-team windows-team

Assignees
No one assigned
Labels
type-feature A feature request or enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
9 participants
@uriyyo @JelleZijlstra @bedevere-bot @Fidget-Spinner @ambv @pablogsal @dlax @corona10 @the-knights-who-say-ni
X Tutup