classification
Title: Optimize type check in pipes.py
Type: performance Stage: resolved
Components: Library (Lib) Versions:
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: anton.gruebel, eric.smith, lukasz.langa, malin, miss-islington, python-dev, serhiy.storchaka
Priority: normal Keywords: patch

Created on 2021-07-22 19:50 by anton.gruebel, last changed 2021-07-29 22:44 by anton.gruebel. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 27291 merged python-dev, 2021-07-22 19:55
PR 27416 merged miss-islington, 2021-07-28 13:38
Messages (8)
msg397995 - (view) Author: Anton Grübel (anton.gruebel) * Date: 2021-07-22 19:50
When I did some work on typeshed I found some weird syntax in pipes.py.

if type(cmd) is not type(''):

which can easily be changed to

if not isinstance(cmd, str):

There are two occurrences and I will directly create the PR :)
msg397996 - (view) Author: Eric V. Smith (eric.smith) * (Python committer) Date: 2021-07-22 19:54
The difference is that type(cmd) is doing an exact match on the type, while isinstance is checking for types or derived types. I don't know if that makes a difference here, but it's worth noting.
msg397998 - (view) Author: Anton Grübel (anton.gruebel) * Date: 2021-07-22 20:06
I know that :) , it is just weird to do also do the type check on an empty string, which can be replaced with str directly, but as far as I know it is usually better to use isinstance instead of type.
msg398000 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2021-07-22 20:27
I suppose it is a very old code written when str was a function, not a type (thus using type('')) and builtin types were not subclassable (thus not using isinstance()).

I once analyzed other similar cases in the stdlib. Seems it is time to revive my old patch.
msg398004 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2021-07-22 20:55
See also issue44712.
msg398022 - (view) Author: Ma Lin (malin) * Date: 2021-07-23 00:54
> I suppose it is a very old code

I also found a few old code may have performance loss.

memoryview.cast() method was add in Python 3.3.
This code doesn't use memoryview.cast(), which will bring extra memory overhead when the amount of data is very large.
https://github.com/python/cpython/blob/v3.10.0b4/Lib/multiprocessing/connection.py#L190-L194
msg398480 - (view) Author: Łukasz Langa (lukasz.langa) * (Python committer) Date: 2021-07-29 11:46
Due to a non-zero (even if close to zero) likelihood that the change from type() comparison to an isinstance() check will change behavior, I merged it to 3.11 and 3.10 as 3.9 is already pretty far out in its bugfix releases.

Thanks, Anton! ✨ 🍰 ✨
msg398522 - (view) Author: Anton Grübel (anton.gruebel) * Date: 2021-07-29 22:44
I'm still super happy to had the chance to contribute a bit to Python and it will be available already in version 3.10 <3

Thanks Serhiy & Lukasz!
History
Date User Action Args
2021-07-29 22:44:49anton.gruebelsetmessages: + msg398522
2021-07-29 11:46:27lukasz.langasetstatus: open -> closed

nosy: + lukasz.langa
messages: + msg398480

resolution: fixed
stage: patch review -> resolved
2021-07-28 13:38:56miss-islingtonsetnosy: + miss-islington
pull_requests: + pull_request25948
2021-07-23 00:54:21malinsetnosy: + malin
messages: + msg398022
2021-07-22 20:55:29serhiy.storchakasetmessages: + msg398004
2021-07-22 20:27:27serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg398000
2021-07-22 20:06:13anton.gruebelsetmessages: + msg397998
2021-07-22 19:55:35python-devsetkeywords: + patch
nosy: + python-dev

pull_requests: + pull_request25833
stage: patch review
2021-07-22 19:54:25eric.smithsetnosy: + eric.smith
messages: + msg397996
2021-07-22 19:50:28anton.gruebelcreate