bpo-37371: Optimize and refactor readline(). #14306

shlomif · 2019-06-22T13:39:04Z

Merge two duplicate branches.
Extract some constants.
Convert to prefix-++ instead of suffix-++.

===========

Benchmarks:

before:

PATH=/home/shlomif/apps/python3/bin:/home/shlomif/apps/fcs/bin:/home/shlomif/progs/freecell/git/0fc-b/:/home/shlomif/progs/freecell/git/0fc-b//board_gen:/home/shlomif/bin:/home/shlomif/apps/perl/modules/bin:/home/shlomif/apps/perl/modules/local/bin:/home/shlomif/apps/neovim/bin:/home/shlomif/apps/fop/fop-20140425:/home/shlomif/apps/vim/bin:/home/shlomif/apps/golang/bin:/home/shlomif/.local/bin:/home/shlomif/.perl6/bin:/home/shlomif/.cargo/bin:/usr/local/bin:/usr/bin:/usr/lib64/qt5/bin:/usr/lib64/qt4/bin:/usr/games
export LD_LIBRARY_PATH=/home/shlomif/apps/python3/lib
LD_LIBRARY_PATH=/home/shlomif/apps/python3/lib
python3 collect-stats.py 0fc-log.txt
S 18474775
Int 617438

real 0m6.655s
user 0m6.599s
sys 0m0.054s

PATH=/home/shlomif/apps/python3/bin:/home/shlomif/apps/fcs/bin:/home/shlomif/progs/freecell/git/0fc-b/:/home/shlomif/progs/freecell/git/0fc-b//board_gen:/home/shlomif/bin:/home/shlomif/apps/perl/modules/bin:/home/shlomif/apps/perl/modules/local/bin:/home/shlomif/apps/neovim/bin:/home/shlomif/apps/fop/fop-20140425:/home/shlomif/apps/vim/bin:/home/shlomif/apps/golang/bin:/home/shlomif/.local/bin:/home/shlomif/.perl6/bin:/home/shlomif/.cargo/bin:/usr/local/bin:/usr/bin:/usr/lib64/qt5/bin:/usr/lib64/qt4/bin:/usr/games
export LD_LIBRARY_PATH=/home/shlomif/apps/python3/lib
LD_LIBRARY_PATH=/home/shlomif/apps/python3/lib
python3 collect-stats.py 0fc-log.txt
S 18474775
Int 617438

real 0m6.817s
user 0m6.772s
sys 0m0.045s

PATH=/home/shlomif/apps/python3/bin:/home/shlomif/apps/fcs/bin:/home/shlomif/progs/freecell/git/0fc-b/:/home/shlomif/progs/freecell/git/0fc-b//board_gen:/home/shlomif/bin:/home/shlomif/apps/perl/modules/bin:/home/shlomif/apps/perl/modules/local/bin:/home/shlomif/apps/neovim/bin:/home/shlomif/apps/fop/fop-20140425:/home/shlomif/apps/vim/bin:/home/shlomif/apps/golang/bin:/home/shlomif/.local/bin:/home/shlomif/.perl6/bin:/home/shlomif/.cargo/bin:/usr/local/bin:/usr/bin:/usr/lib64/qt5/bin:/usr/lib64/qt4/bin:/usr/games
export LD_LIBRARY_PATH=/home/shlomif/apps/python3/lib
LD_LIBRARY_PATH=/home/shlomif/apps/python3/lib
python3 collect-stats.py 0fc-log.txt
S 18474775
Int 617438

real 0m6.763s
user 0m6.706s
sys 0m0.057s

============

After:

PATH=/home/shlomif/apps/python3/bin:/home/shlomif/apps/fcs/bin:/home/shlomif/progs/freecell/git/0fc-b/:/home/shlomif/progs/freecell/git/0fc-b//board_gen:/home/shlomif/bin:/home/shlomif/apps/perl/modules/bin:/home/shlomif/apps/perl/modules/local/bin:/home/shlomif/apps/neovim/bin:/home/shlomif/apps/fop/fop-20140425:/home/shlomif/apps/vim/bin:/home/shlomif/apps/golang/bin:/home/shlomif/.local/bin:/home/shlomif/.perl6/bin:/home/shlomif/.cargo/bin:/usr/local/bin:/usr/bin:/usr/lib64/qt5/bin:/usr/lib64/qt4/bin:/usr/games
export LD_LIBRARY_PATH=/home/shlomif/apps/python3/lib
LD_LIBRARY_PATH=/home/shlomif/apps/python3/lib
python3 collect-stats.py 0fc-log.txt
S 18474775
Int 617438

real 0m6.651s
user 0m6.595s
sys 0m0.056s

PATH=/home/shlomif/apps/python3/bin:/home/shlomif/apps/fcs/bin:/home/shlomif/progs/freecell/git/0fc-b/:/home/shlomif/progs/freecell/git/0fc-b//board_gen:/home/shlomif/bin:/home/shlomif/apps/perl/modules/bin:/home/shlomif/apps/perl/modules/local/bin:/home/shlomif/apps/neovim/bin:/home/shlomif/apps/fop/fop-20140425:/home/shlomif/apps/vim/bin:/home/shlomif/apps/golang/bin:/home/shlomif/.local/bin:/home/shlomif/.perl6/bin:/home/shlomif/.cargo/bin:/usr/local/bin:/usr/bin:/usr/lib64/qt5/bin:/usr/lib64/qt4/bin:/usr/games
export LD_LIBRARY_PATH=/home/shlomif/apps/python3/lib
LD_LIBRARY_PATH=/home/shlomif/apps/python3/lib
python3 collect-stats.py 0fc-log.txt
S 18474775
Int 617438

real 0m6.625s
user 0m6.559s
sys 0m0.066s

PATH=/home/shlomif/apps/python3/bin:/home/shlomif/apps/fcs/bin:/home/shlomif/progs/freecell/git/0fc-b/:/home/shlomif/progs/freecell/git/0fc-b//board_gen:/home/shlomif/bin:/home/shlomif/apps/perl/modules/bin:/home/shlomif/apps/perl/modules/local/bin:/home/shlomif/apps/neovim/bin:/home/shlomif/apps/fop/fop-20140425:/home/shlomif/apps/vim/bin:/home/shlomif/apps/golang/bin:/home/shlomif/.local/bin:/home/shlomif/.perl6/bin:/home/shlomif/.cargo/bin:/usr/local/bin:/usr/bin:/usr/lib64/qt5/bin:/usr/lib64/qt4/bin:/usr/games
export LD_LIBRARY_PATH=/home/shlomif/apps/python3/lib
LD_LIBRARY_PATH=/home/shlomif/apps/python3/lib
python3 collect-stats.py 0fc-log.txt
S 18474775
Int 617438

real 0m6.601s
user 0m6.559s
sys 0m0.042s

=========

Based on https://github.com/shlomif/freecell-pro-0fc-deals .

https://bugs.python.org/issue37371

tirkarthi · 2019-06-22T13:55:44Z

Please open an issue for this at https://bugs.python.org along with the benchmark findings.

shlomif · 2019-06-22T14:12:12Z

@tirkarthi : see https://bugs.python.org/issue37371

mangrisano · 2019-06-22T15:48:39Z

/cc @benjaminp

serhiy-storchaka · 2019-06-25T07:07:14Z

Modules/_io/iobase.c

+            {
+                const Py_ssize_t len_readahead = PyBytes_GET_SIZE(readahead);
+                if (len_readahead > 0) {
+#ifndef min


You can use Py_MIN.

1. Merge two duplicate branches. 2. Extract some constants. 3. Convert to prefix-++ instead of suffix-++. =========== Benchmarks: before: + PATH=/home/shlomif/apps/python3/bin:/home/shlomif/apps/fcs/bin:/home/shlomif/progs/freecell/git/0fc-b/:/home/shlomif/progs/freecell/git/0fc-b//board_gen:/home/shlomif/bin:/home/shlomif/apps/perl/modules/bin:/home/shlomif/apps/perl/modules/local/bin:/home/shlomif/apps/neovim/bin:/home/shlomif/apps/fop/fop-20140425:/home/shlomif/apps/vim/bin:/home/shlomif/apps/golang/bin:/home/shlomif/.local/bin:/home/shlomif/.perl6/bin:/home/shlomif/.cargo/bin:/usr/local/bin:/usr/bin:/usr/lib64/qt5/bin:/usr/lib64/qt4/bin:/usr/games + export LD_LIBRARY_PATH=/home/shlomif/apps/python3/lib + LD_LIBRARY_PATH=/home/shlomif/apps/python3/lib + python3 collect-stats.py 0fc-log.txt S 18474775 Int 617438 real 0m6.655s user 0m6.599s sys 0m0.054s + PATH=/home/shlomif/apps/python3/bin:/home/shlomif/apps/fcs/bin:/home/shlomif/progs/freecell/git/0fc-b/:/home/shlomif/progs/freecell/git/0fc-b//board_gen:/home/shlomif/bin:/home/shlomif/apps/perl/modules/bin:/home/shlomif/apps/perl/modules/local/bin:/home/shlomif/apps/neovim/bin:/home/shlomif/apps/fop/fop-20140425:/home/shlomif/apps/vim/bin:/home/shlomif/apps/golang/bin:/home/shlomif/.local/bin:/home/shlomif/.perl6/bin:/home/shlomif/.cargo/bin:/usr/local/bin:/usr/bin:/usr/lib64/qt5/bin:/usr/lib64/qt4/bin:/usr/games + export LD_LIBRARY_PATH=/home/shlomif/apps/python3/lib + LD_LIBRARY_PATH=/home/shlomif/apps/python3/lib + python3 collect-stats.py 0fc-log.txt S 18474775 Int 617438 real 0m6.817s user 0m6.772s sys 0m0.045s + PATH=/home/shlomif/apps/python3/bin:/home/shlomif/apps/fcs/bin:/home/shlomif/progs/freecell/git/0fc-b/:/home/shlomif/progs/freecell/git/0fc-b//board_gen:/home/shlomif/bin:/home/shlomif/apps/perl/modules/bin:/home/shlomif/apps/perl/modules/local/bin:/home/shlomif/apps/neovim/bin:/home/shlomif/apps/fop/fop-20140425:/home/shlomif/apps/vim/bin:/home/shlomif/apps/golang/bin:/home/shlomif/.local/bin:/home/shlomif/.perl6/bin:/home/shlomif/.cargo/bin:/usr/local/bin:/usr/bin:/usr/lib64/qt5/bin:/usr/lib64/qt4/bin:/usr/games + export LD_LIBRARY_PATH=/home/shlomif/apps/python3/lib + LD_LIBRARY_PATH=/home/shlomif/apps/python3/lib + python3 collect-stats.py 0fc-log.txt S 18474775 Int 617438 real 0m6.763s user 0m6.706s sys 0m0.057s ============ After: + PATH=/home/shlomif/apps/python3/bin:/home/shlomif/apps/fcs/bin:/home/shlomif/progs/freecell/git/0fc-b/:/home/shlomif/progs/freecell/git/0fc-b//board_gen:/home/shlomif/bin:/home/shlomif/apps/perl/modules/bin:/home/shlomif/apps/perl/modules/local/bin:/home/shlomif/apps/neovim/bin:/home/shlomif/apps/fop/fop-20140425:/home/shlomif/apps/vim/bin:/home/shlomif/apps/golang/bin:/home/shlomif/.local/bin:/home/shlomif/.perl6/bin:/home/shlomif/.cargo/bin:/usr/local/bin:/usr/bin:/usr/lib64/qt5/bin:/usr/lib64/qt4/bin:/usr/games + export LD_LIBRARY_PATH=/home/shlomif/apps/python3/lib + LD_LIBRARY_PATH=/home/shlomif/apps/python3/lib + python3 collect-stats.py 0fc-log.txt S 18474775 Int 617438 real 0m6.651s user 0m6.595s sys 0m0.056s + PATH=/home/shlomif/apps/python3/bin:/home/shlomif/apps/fcs/bin:/home/shlomif/progs/freecell/git/0fc-b/:/home/shlomif/progs/freecell/git/0fc-b//board_gen:/home/shlomif/bin:/home/shlomif/apps/perl/modules/bin:/home/shlomif/apps/perl/modules/local/bin:/home/shlomif/apps/neovim/bin:/home/shlomif/apps/fop/fop-20140425:/home/shlomif/apps/vim/bin:/home/shlomif/apps/golang/bin:/home/shlomif/.local/bin:/home/shlomif/.perl6/bin:/home/shlomif/.cargo/bin:/usr/local/bin:/usr/bin:/usr/lib64/qt5/bin:/usr/lib64/qt4/bin:/usr/games + export LD_LIBRARY_PATH=/home/shlomif/apps/python3/lib + LD_LIBRARY_PATH=/home/shlomif/apps/python3/lib + python3 collect-stats.py 0fc-log.txt S 18474775 Int 617438 real 0m6.625s user 0m6.559s sys 0m0.066s + PATH=/home/shlomif/apps/python3/bin:/home/shlomif/apps/fcs/bin:/home/shlomif/progs/freecell/git/0fc-b/:/home/shlomif/progs/freecell/git/0fc-b//board_gen:/home/shlomif/bin:/home/shlomif/apps/perl/modules/bin:/home/shlomif/apps/perl/modules/local/bin:/home/shlomif/apps/neovim/bin:/home/shlomif/apps/fop/fop-20140425:/home/shlomif/apps/vim/bin:/home/shlomif/apps/golang/bin:/home/shlomif/.local/bin:/home/shlomif/.perl6/bin:/home/shlomif/.cargo/bin:/usr/local/bin:/usr/bin:/usr/lib64/qt5/bin:/usr/lib64/qt4/bin:/usr/games + export LD_LIBRARY_PATH=/home/shlomif/apps/python3/lib + LD_LIBRARY_PATH=/home/shlomif/apps/python3/lib + python3 collect-stats.py 0fc-log.txt S 18474775 Int 617438 real 0m6.601s user 0m6.559s sys 0m0.042s ========= Based on https://github.com/shlomif/freecell-pro-0fc-deals .

@serhiy-storchaka

Thanks to @serhiy-storchaka .

shlomif · 2020-01-28T08:46:46Z

Hi all,

can this be merged now, or do you need anything beforehand?

benjaminp · 2020-01-29T03:12:16Z

Modules/_io/iobase.c

+                const Py_ssize_t len_readahead = PyBytes_GET_SIZE(readahead);
+                if (len_readahead > 0) {
+                    const Py_ssize_t upper_limit = (((limit >= 0) ? Py_MIN(limit, len_readahead) : len_readahead)-1);
+                    Py_ssize_t n = -1;


This initialization is a bit awkward. Can we "add 1" to everything? i.e., make this n = 0 and remove the -1 above?

Well, I did it that way so we can use prefix-++ instead of suffix-++ because the former is believed to be faster. What do you think?

I doubt it makes any difference with a modern compiler.

I concur with @benjaminp.

Also, why not use memchr?

iritkatriel · 2022-11-07T16:41:46Z

I removed the performance label because it's not clear that this makes a difference we would need benchmarks to see that it does).

Added the pending label because it seems to have been abandoned (there was no response to @benjaminp 's review). This means that this PR will be closed in a few weeks if there is no followup.

shlomif · 2022-11-08T03:56:59Z

On Mon, 07 Nov 2022 11:08:01 -0800 Serhiy Storchaka ***@***.***> wrote: @serhiy-storchaka commented on this pull request. > @@ -572,26 +572,20 @@ _io__IOBase_readline_impl(PyObject *self, Py_ssize_t > limit) Py_DECREF(readahead); goto fail; } - if (PyBytes_GET_SIZE(readahead) > 0) { - Py_ssize_t n = 0; - const char *buf = PyBytes_AS_STRING(readahead); - if (limit >= 0) { + { + const Py_ssize_t len_readahead = PyBytes_GET_SIZE(readahead); + if (len_readahead > 0) { + const Py_ssize_t upper_limit = (((limit >= 0) ? Py_MIN(limit, len_readahead) : len_readahead)-1); + Py_ssize_t n = -1; I concur with @benjaminp. Also, why not use `memchr`?

hi. you can close this pull-req now. i'm not attched to it.

…

-- Shlomi Fish https://www.shlomifish.org/ Emma Watson Factoids - https://shlom.in/emwatson-facts Sarah: Ken, would you like some more juice? Ken: No, Sarah, that's OK, I’m stuffed. Anyway, I think I’ll return to my dimension of evil now - they will be clueless without me. Thanks for everything. — https://www.shlomifish.org/humour/Buffy/A-Few-Good-Slayers/ Please reply to list if it's a mailing list post - https://shlom.in/reply .

iritkatriel · 2022-11-08T06:38:02Z

Thanks.

the-knights-who-say-ni added the CLA signed label Jun 22, 2019

bedevere-bot added the awaiting review label Jun 22, 2019

serhiy-storchaka reviewed Jun 25, 2019

View reviewed changes

shlomif force-pushed the shlomif1-opt branch from a769b77 to 8eccf10 Compare December 7, 2019 20:19

shlomif added 2 commits December 28, 2019 17:35

Convert to the Py_MIN macro.

89bdc1c

Thanks to @serhiy-storchaka .

shlomif force-pushed the shlomif1-opt branch from 8eccf10 to 89bdc1c Compare December 28, 2019 15:35

csabella changed the title ~~Optimize and refactor readline().~~ bpo-37371: Optimize and refactor readline(). Jan 13, 2020

csabella requested review from serhiy-storchaka, benjaminp and gpshead January 28, 2020 09:30

benjaminp reviewed Jan 29, 2020

View reviewed changes

ezio-melotti removed the CLA signed label Jul 13, 2022

eendebakpt mentioned this pull request Aug 31, 2022

Triage the open "performance" issues on cpython repo faster-cpython/ideas#440

Closed

iritkatriel added the performance Performance or resource usage label Aug 31, 2022

iritkatriel added pending The issue will be closed if no feedback is provided and removed performance Performance or resource usage labels Nov 7, 2022

Merge branch 'main' into shlomif1-opt

2d697dd

iritkatriel closed this Nov 8, 2022

shlomif mannequin mentioned this pull request Nov 8, 2022

Optimize and refactor readline(). #81552

Closed

AA-Turner removed the pending The issue will be closed if no feedback is provided label Apr 6, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

bpo-37371: Optimize and refactor readline(). #14306

bpo-37371: Optimize and refactor readline(). #14306

Uh oh!

shlomif commented Jun 22, 2019 •

edited by bedevere-bot

Loading

Uh oh!

tirkarthi commented Jun 22, 2019

Uh oh!

shlomif commented Jun 22, 2019

Uh oh!

mangrisano commented Jun 22, 2019

Uh oh!

serhiy-storchaka Jun 25, 2019

Uh oh!

shlomif commented Jan 28, 2020

Uh oh!

benjaminp Jan 29, 2020

Uh oh!

shlomif Jan 29, 2020

Uh oh!

benjaminp Jan 29, 2020

Uh oh!

serhiy-storchaka Nov 7, 2022

Uh oh!

iritkatriel commented Nov 7, 2022

Uh oh!

shlomif commented Nov 8, 2022 via email

Uh oh!

iritkatriel commented Nov 8, 2022

Uh oh!

Uh oh!

Uh oh!

bpo-37371: Optimize and refactor readline(). #14306

bpo-37371: Optimize and refactor readline(). #14306

Uh oh!

Conversation

shlomif commented Jun 22, 2019 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tirkarthi commented Jun 22, 2019

Uh oh!

shlomif commented Jun 22, 2019

Uh oh!

mangrisano commented Jun 22, 2019

Uh oh!

serhiy-storchaka Jun 25, 2019

Choose a reason for hiding this comment

Uh oh!

shlomif commented Jan 28, 2020

Uh oh!

benjaminp Jan 29, 2020

Choose a reason for hiding this comment

Uh oh!

shlomif Jan 29, 2020

Choose a reason for hiding this comment

Uh oh!

benjaminp Jan 29, 2020

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka Nov 7, 2022

Choose a reason for hiding this comment

Uh oh!

iritkatriel commented Nov 7, 2022

Uh oh!

shlomif commented Nov 8, 2022 via email

Uh oh!

iritkatriel commented Nov 8, 2022

Uh oh!

Uh oh!

shlomif commented Jun 22, 2019 •

edited by bedevere-bot

Loading