From 108e65bc3a2a142ed2bb347e5222a626c477d93c Mon Sep 17 00:00:00 2001 From: Alexander Plaikner Date: Wed, 3 Jul 2024 11:21:24 +0200 Subject: [PATCH 01/14] Add clean code --- Modules/posixmodule.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/Modules/posixmodule.c b/Modules/posixmodule.c index fc218383d5ff95..ca8bbf007c608f 100644 --- a/Modules/posixmodule.c +++ b/Modules/posixmodule.c @@ -11342,6 +11342,20 @@ os_read_impl(PyObject *module, int fd, Py_ssize_t length) length = Py_MIN(length, _PY_READ_MAX); + static long page_size; + if (page_size == 0) + page_size = sysconf(_SC_PAGE_SIZE); + +#ifndef MS_WINDOWS + if (length > page_size * 16) { + struct stat statbuffer; + fstat(fd, &statbuffer); + if (S_ISFIFO(statbuffer.st_mode)) { + length = Py_MIN(page_size* 16, length); + } + } +#endif + buffer = PyBytes_FromStringAndSize((char *)NULL, length); if (buffer == NULL) return NULL; From 37ca6062c9194950a1798b5727c57d6a02505c04 Mon Sep 17 00:00:00 2001 From: Alexander Plaikner Date: Wed, 3 Jul 2024 11:34:19 +0200 Subject: [PATCH 02/14] Fix linting error & sysconf unsupported error --- Modules/posixmodule.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Modules/posixmodule.c b/Modules/posixmodule.c index ca8bbf007c608f..ed4fe81a3010ad 100644 --- a/Modules/posixmodule.c +++ b/Modules/posixmodule.c @@ -11342,11 +11342,11 @@ os_read_impl(PyObject *module, int fd, Py_ssize_t length) length = Py_MIN(length, _PY_READ_MAX); +#ifndef MS_WINDOWS static long page_size; - if (page_size == 0) + if (page_size == 0) page_size = sysconf(_SC_PAGE_SIZE); -#ifndef MS_WINDOWS if (length > page_size * 16) { struct stat statbuffer; fstat(fd, &statbuffer); From 89936c213e0b66aa89d95f7cd88626c94d109f4f Mon Sep 17 00:00:00 2001 From: "blurb-it[bot]" <43283697+blurb-it[bot]@users.noreply.github.com> Date: Wed, 3 Jul 2024 10:11:54 +0000 Subject: [PATCH 03/14] =?UTF-8?q?=F0=9F=93=9C=F0=9F=A4=96=20Added=20by=20b?= =?UTF-8?q?lurb=5Fit.?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../next/C API/2024-07-03-10-11-53.gh-issue-121313.D7gARW.rst | 1 + 1 file changed, 1 insertion(+) create mode 100644 Misc/NEWS.d/next/C API/2024-07-03-10-11-53.gh-issue-121313.D7gARW.rst diff --git a/Misc/NEWS.d/next/C API/2024-07-03-10-11-53.gh-issue-121313.D7gARW.rst b/Misc/NEWS.d/next/C API/2024-07-03-10-11-53.gh-issue-121313.D7gARW.rst new file mode 100644 index 00000000000000..7664094ce187b6 --- /dev/null +++ b/Misc/NEWS.d/next/C API/2024-07-03-10-11-53.gh-issue-121313.D7gARW.rst @@ -0,0 +1 @@ +- Limit reading size in os.read for pipes to default pipe size in order to avoid memory overallocation From 31c65b4af7ccf930bfdfd37c4e711eddecb3c87b Mon Sep 17 00:00:00 2001 From: Alexander Plaikner Date: Wed, 3 Jul 2024 12:47:46 +0200 Subject: [PATCH 04/14] Fix news --- .../next/C API/2024-07-03-10-11-53.gh-issue-121313.D7gARW.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Misc/NEWS.d/next/C API/2024-07-03-10-11-53.gh-issue-121313.D7gARW.rst b/Misc/NEWS.d/next/C API/2024-07-03-10-11-53.gh-issue-121313.D7gARW.rst index 7664094ce187b6..ee52a416374c75 100644 --- a/Misc/NEWS.d/next/C API/2024-07-03-10-11-53.gh-issue-121313.D7gARW.rst +++ b/Misc/NEWS.d/next/C API/2024-07-03-10-11-53.gh-issue-121313.D7gARW.rst @@ -1 +1 @@ -- Limit reading size in os.read for pipes to default pipe size in order to avoid memory overallocation +Limit reading size in os.read for pipes to default pipe size in order to avoid memory overallocation From 0a4e4a338b896a5e8a5e54fcd233cff86db6b9a9 Mon Sep 17 00:00:00 2001 From: Alexander Plaikner Date: Wed, 3 Jul 2024 13:55:10 +0200 Subject: [PATCH 05/14] Make page size non static --- Modules/posixmodule.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/Modules/posixmodule.c b/Modules/posixmodule.c index ed4fe81a3010ad..9aaa122eb14775 100644 --- a/Modules/posixmodule.c +++ b/Modules/posixmodule.c @@ -11343,9 +11343,7 @@ os_read_impl(PyObject *module, int fd, Py_ssize_t length) length = Py_MIN(length, _PY_READ_MAX); #ifndef MS_WINDOWS - static long page_size; - if (page_size == 0) - page_size = sysconf(_SC_PAGE_SIZE); + long page_size = sysconf(_SC_PAGE_SIZE); if (length > page_size * 16) { struct stat statbuffer; From e6c64fec3e06f0f67d22e63cfb879cdbdeba6cc6 Mon Sep 17 00:00:00 2001 From: Alexander Plaikner Date: Thu, 4 Jul 2024 09:51:21 +0200 Subject: [PATCH 06/14] Remove redundant call to pymin --- Modules/posixmodule.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Modules/posixmodule.c b/Modules/posixmodule.c index 9aaa122eb14775..aa0672ca442633 100644 --- a/Modules/posixmodule.c +++ b/Modules/posixmodule.c @@ -11349,7 +11349,7 @@ os_read_impl(PyObject *module, int fd, Py_ssize_t length) struct stat statbuffer; fstat(fd, &statbuffer); if (S_ISFIFO(statbuffer.st_mode)) { - length = Py_MIN(page_size* 16, length); + length = page_size* 16; } } #endif From 936b601d22e26d6fe373382a48e43660cc34376a Mon Sep 17 00:00:00 2001 From: Alexander Plaikner Date: Fri, 5 Jul 2024 09:43:44 +0200 Subject: [PATCH 07/14] Shift pipe check to connection.py _recv --- Lib/multiprocessing/connection.py | 9 ++++++++- Modules/posixmodule.c | 12 ------------ 2 files changed, 8 insertions(+), 13 deletions(-) diff --git a/Lib/multiprocessing/connection.py b/Lib/multiprocessing/connection.py index b7e1e132172d02..18c7848896e856 100644 --- a/Lib/multiprocessing/connection.py +++ b/Lib/multiprocessing/connection.py @@ -18,6 +18,7 @@ import time import tempfile import itertools +import stat from . import util @@ -391,8 +392,14 @@ def _recv(self, size, read=_read): buf = io.BytesIO() handle = self._handle remaining = size + is_pipe = False + if not _winapi: + mode = os.fstat(handle).st_mode + is_pipe = stat.S_ISFIFO(mode) + limit = 2 << 15 if is_pipe else remaining while remaining > 0: - chunk = read(handle, remaining) + to_read = min(limit, remaining) + chunk = read(handle, to_read) n = len(chunk) if n == 0: if remaining == size: diff --git a/Modules/posixmodule.c b/Modules/posixmodule.c index aa0672ca442633..fc218383d5ff95 100644 --- a/Modules/posixmodule.c +++ b/Modules/posixmodule.c @@ -11342,18 +11342,6 @@ os_read_impl(PyObject *module, int fd, Py_ssize_t length) length = Py_MIN(length, _PY_READ_MAX); -#ifndef MS_WINDOWS - long page_size = sysconf(_SC_PAGE_SIZE); - - if (length > page_size * 16) { - struct stat statbuffer; - fstat(fd, &statbuffer); - if (S_ISFIFO(statbuffer.st_mode)) { - length = page_size* 16; - } - } -#endif - buffer = PyBytes_FromStringAndSize((char *)NULL, length); if (buffer == NULL) return NULL; From 49d8adb07c2e63e41614ea0e63bc5c66a7a0bbee Mon Sep 17 00:00:00 2001 From: Alexander Plaikner Date: Fri, 5 Jul 2024 13:04:07 +0200 Subject: [PATCH 08/14] Make pipe size dependant on systems page size --- Lib/multiprocessing/connection.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/Lib/multiprocessing/connection.py b/Lib/multiprocessing/connection.py index 18c7848896e856..9754dbc8ad957d 100644 --- a/Lib/multiprocessing/connection.py +++ b/Lib/multiprocessing/connection.py @@ -393,10 +393,13 @@ def _recv(self, size, read=_read): handle = self._handle remaining = size is_pipe = False + page_size = 0 if not _winapi: mode = os.fstat(handle).st_mode is_pipe = stat.S_ISFIFO(mode) - limit = 2 << 15 if is_pipe else remaining + if (is_pipe): + page_size = os.sysconf(os.sysconf_names['SC_PAGESIZE']) + limit = 16 * page_size if is_pipe else remaining while remaining > 0: to_read = min(limit, remaining) chunk = read(handle, to_read) From 43e19ddd381ae1e011f4a883a35912556874c825 Mon Sep 17 00:00:00 2001 From: Alexander Plaikner Date: Fri, 5 Jul 2024 13:49:15 +0200 Subject: [PATCH 09/14] Only execute fstat in case reading size is bigger than default pipe size --- Lib/multiprocessing/connection.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/Lib/multiprocessing/connection.py b/Lib/multiprocessing/connection.py index 9754dbc8ad957d..4797ca4df8d23d 100644 --- a/Lib/multiprocessing/connection.py +++ b/Lib/multiprocessing/connection.py @@ -395,10 +395,10 @@ def _recv(self, size, read=_read): is_pipe = False page_size = 0 if not _winapi: - mode = os.fstat(handle).st_mode - is_pipe = stat.S_ISFIFO(mode) - if (is_pipe): - page_size = os.sysconf(os.sysconf_names['SC_PAGESIZE']) + page_size = os.sysconf(os.sysconf_names['SC_PAGESIZE']) + if size > 16 * page_size: + mode = os.fstat(handle).st_mode + is_pipe = stat.S_ISFIFO(mode) limit = 16 * page_size if is_pipe else remaining while remaining > 0: to_read = min(limit, remaining) From b0b86e5758d722fbc415259c175dbbf2a10090f5 Mon Sep 17 00:00:00 2001 From: Alexander Plaikner Date: Sun, 7 Jul 2024 07:17:10 +0200 Subject: [PATCH 10/14] Update news message --- .../next/C API/2024-07-03-10-11-53.gh-issue-121313.D7gARW.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Misc/NEWS.d/next/C API/2024-07-03-10-11-53.gh-issue-121313.D7gARW.rst b/Misc/NEWS.d/next/C API/2024-07-03-10-11-53.gh-issue-121313.D7gARW.rst index ee52a416374c75..06abce9e67da44 100644 --- a/Misc/NEWS.d/next/C API/2024-07-03-10-11-53.gh-issue-121313.D7gARW.rst +++ b/Misc/NEWS.d/next/C API/2024-07-03-10-11-53.gh-issue-121313.D7gARW.rst @@ -1 +1 @@ -Limit reading size in os.read for pipes to default pipe size in order to avoid memory overallocation +Limit reading size in multiprocessing connection._recv for pipes to default pipe size of 16 times base page size, in order to avoid memory overallocation and unnecessary memory management system calls. From 2b6ff241c00a30bab30d005917b68bc5bb8f521c Mon Sep 17 00:00:00 2001 From: Alexander Plaikner Date: Sun, 7 Jul 2024 07:20:35 +0200 Subject: [PATCH 11/14] Make imports order alphabetical --- Lib/multiprocessing/connection.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Lib/multiprocessing/connection.py b/Lib/multiprocessing/connection.py index 4797ca4df8d23d..b0d3e7a5703800 100644 --- a/Lib/multiprocessing/connection.py +++ b/Lib/multiprocessing/connection.py @@ -11,14 +11,14 @@ import errno import io +import itertools import os +import stat import sys import socket import struct -import time import tempfile -import itertools -import stat +import time from . import util From e726f5142f769a61989220f09b9548626ebfa819 Mon Sep 17 00:00:00 2001 From: Alexander Plaikner Date: Sun, 7 Jul 2024 07:38:37 +0200 Subject: [PATCH 12/14] Shift calculation for pipe size to existing if _winapi check --- Lib/multiprocessing/connection.py | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/Lib/multiprocessing/connection.py b/Lib/multiprocessing/connection.py index b0d3e7a5703800..11b7868e46145a 100644 --- a/Lib/multiprocessing/connection.py +++ b/Lib/multiprocessing/connection.py @@ -373,11 +373,14 @@ def _close(self, _close=_multiprocessing.closesocket): _close(self._handle) _write = _multiprocessing.send _read = _multiprocessing.recv + _default_pipe_size = 0 else: def _close(self, _close=os.close): _close(self._handle) _write = os.write _read = os.read + _base_page_size = os.sysconf(os.sysconf_names['SC_PAGESIZE']) + _default_pipe_size = _base_page_size * 16 def _send(self, buf, write=_write): remaining = len(buf) @@ -393,13 +396,10 @@ def _recv(self, size, read=_read): handle = self._handle remaining = size is_pipe = False - page_size = 0 - if not _winapi: - page_size = os.sysconf(os.sysconf_names['SC_PAGESIZE']) - if size > 16 * page_size: - mode = os.fstat(handle).st_mode - is_pipe = stat.S_ISFIFO(mode) - limit = 16 * page_size if is_pipe else remaining + if size > self._default_pipe_size > 0: + mode = os.fstat(handle).st_mode + is_pipe = stat.S_ISFIFO(mode) + limit = self._default_pipe_size if is_pipe else remaining while remaining > 0: to_read = min(limit, remaining) chunk = read(handle, to_read) From 59cff4d4373a1af3836204f2760e1432baa4c008 Mon Sep 17 00:00:00 2001 From: Alexander Plaikner Date: Sun, 7 Jul 2024 07:41:09 +0200 Subject: [PATCH 13/14] Fix linting error --- Lib/multiprocessing/connection.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Lib/multiprocessing/connection.py b/Lib/multiprocessing/connection.py index 11b7868e46145a..31c805fdb5372c 100644 --- a/Lib/multiprocessing/connection.py +++ b/Lib/multiprocessing/connection.py @@ -379,7 +379,7 @@ def _close(self, _close=os.close): _close(self._handle) _write = os.write _read = os.read - _base_page_size = os.sysconf(os.sysconf_names['SC_PAGESIZE']) + _base_page_size = os.sysconf(os.sysconf_names['SC_PAGESIZE']) _default_pipe_size = _base_page_size * 16 def _send(self, buf, write=_write): From 94d4c4a5127326d7de52d2c142c563082b7c69ba Mon Sep 17 00:00:00 2001 From: Alexander Plaikner Date: Sun, 7 Jul 2024 17:57:17 +0200 Subject: [PATCH 14/14] Create constant for default number of pages per pipe --- Lib/multiprocessing/connection.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/Lib/multiprocessing/connection.py b/Lib/multiprocessing/connection.py index 31c805fdb5372c..d84b52fe6d4f88 100644 --- a/Lib/multiprocessing/connection.py +++ b/Lib/multiprocessing/connection.py @@ -361,6 +361,11 @@ def _get_more_data(self, ov, maxsize): f.write(ov.getbuffer()) return f +""" +The default size of a pipe on Linux systems is 16 times the base page size: +https://man7.org/linux/man-pages/man7/pipe.7.html +""" +PAGES_PER_PIPE = 16 class Connection(_ConnectionBase): """ @@ -380,7 +385,7 @@ def _close(self, _close=os.close): _write = os.write _read = os.read _base_page_size = os.sysconf(os.sysconf_names['SC_PAGESIZE']) - _default_pipe_size = _base_page_size * 16 + _default_pipe_size = _base_page_size * PAGES_PER_PIPE def _send(self, buf, write=_write): remaining = len(buf)