[gentoo-dev] [PATCH 0/7] pypi.eclass: performance optimizations

[gentoo-dev] [PATCH 0/7] pypi.eclass: performance optimizations

[Michał Górny]

Hi,

Here's a set of patches that improve performance of pypi.eclass
by eliminating the subshells from the most common code paths.  It comes
with a trivial benchmarking tool that shows roughly 16 times speedup
from the changes.

Thanks to Sam for bringing the problem up, and to Eli Schwartz for
the shopt idea, that is responsible the final big speedup.


-- 
Best regards,
Michał Górny


Michał Górny (7):
  pypi.eclass: Move setting globals to a function
  eclass/tests: Add pypi-bench.sh for global scope logic
  pypi.eclass: Translate version once in the default scenario
  pypi.eclass: Normalize names without subshell
  pypi.eclass: Translate version without subshell in common case
  pypi.eclass: Replace pypi_sdist_url in global scope
  pypi.eclass: Avoid subshell for extglob setting

 eclass/pypi.eclass         | 128 ++++++++++++++++++++++++++-----------
 eclass/tests/pypi-bench.sh |  23 +++++++
 2 files changed, 113 insertions(+), 38 deletions(-)
 create mode 100755 eclass/tests/pypi-bench.sh

-- 
2.41.0

[gentoo-dev] [PATCH 1/7] pypi.eclass: Move setting globals to a function

[Michał Górny]

Signed-off-by: Michał Górny <mgorny@gentoo.org>
---
 eclass/pypi.eclass | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/eclass/pypi.eclass b/eclass/pypi.eclass
index 13dd56fa4fec..732b0c6184ef 100644
--- a/eclass/pypi.eclass
+++ b/eclass/pypi.eclass
@@ -221,12 +221,20 @@ pypi_wheel_url() {
 	fi
 }
 
-if [[ ${PYPI_NO_NORMALIZE} ]]; then
-	SRC_URI="$(pypi_sdist_url --no-normalize)"
-	S="${WORKDIR}/${PYPI_PN}-$(pypi_translate_version "${PV}")"
-else
-	SRC_URI="$(pypi_sdist_url)"
-	S="${WORKDIR}/$(pypi_normalize_name "${PYPI_PN}")-$(pypi_translate_version "${PV}")"
-fi
+# @FUNCTION: _pypi_set_globals
+# @INTERNAL
+# @DESCRIPTION:
+# Set global variables, SRC_URI and S.
+_pypi_set_globals() {
+	if [[ ${PYPI_NO_NORMALIZE} ]]; then
+		SRC_URI="$(pypi_sdist_url --no-normalize)"
+		S="${WORKDIR}/${PYPI_PN}-$(pypi_translate_version "${PV}")"
+	else
+		SRC_URI="$(pypi_sdist_url)"
+		S="${WORKDIR}/$(pypi_normalize_name "${PYPI_PN}")-$(pypi_translate_version "${PV}")"
+	fi
+}
+
+_pypi_set_globals
 
 fi
-- 
2.41.0

[gentoo-dev] [PATCH 2/7] eclass/tests: Add pypi-bench.sh for global scope logic

[Michał Górny]

The benchmark yield roughly 327 ops / s on my machine.

Signed-off-by: Michał Górny <mgorny@gentoo.org>
---
 eclass/tests/pypi-bench.sh | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)
 create mode 100755 eclass/tests/pypi-bench.sh

diff --git a/eclass/tests/pypi-bench.sh b/eclass/tests/pypi-bench.sh
new file mode 100755
index 000000000000..6c4696c19fcb
--- /dev/null
+++ b/eclass/tests/pypi-bench.sh
@@ -0,0 +1,23 @@
+#!/bin/bash
+# Copyright 2023 Gentoo Authors
+# Distributed under the terms of the GNU General Public License v2
+
+EAPI=8
+source tests-common.sh || exit
+
+PN=foo-bar
+PYPI_PN=Foo.Bar
+PV=1.2.3_beta2
+WORKDIR='<WORKDIR>'
+
+inherit pypi
+
+doit() {
+	for (( i = 0; i < 1000; i++ )); do
+		_pypi_set_globals
+	done
+}
+
+time doit
+
+texit
-- 
2.41.0

[gentoo-dev] [PATCH 3/7] pypi.eclass: Translate version once in the default scenario

[Michał Górny]

Instead of translating version two times, once in pypi_sdist_url
and then when setting S, do it once and store the result.  This gives
roughly 371 ops / s, i.e. a 13% speedup.

Signed-off-by: Michał Górny <mgorny@gentoo.org>
---
 eclass/pypi.eclass | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/eclass/pypi.eclass b/eclass/pypi.eclass
index 732b0c6184ef..a8a179d5a3a4 100644
--- a/eclass/pypi.eclass
+++ b/eclass/pypi.eclass
@@ -226,12 +226,14 @@ pypi_wheel_url() {
 # @DESCRIPTION:
 # Set global variables, SRC_URI and S.
 _pypi_set_globals() {
+	local version=$(pypi_translate_version "${PV}")
+
 	if [[ ${PYPI_NO_NORMALIZE} ]]; then
-		SRC_URI="$(pypi_sdist_url --no-normalize)"
-		S="${WORKDIR}/${PYPI_PN}-$(pypi_translate_version "${PV}")"
+		SRC_URI="$(pypi_sdist_url --no-normalize "${PYPI_PN}" "${version}")"
+		S="${WORKDIR}/${PYPI_PN}-${version}"
 	else
-		SRC_URI="$(pypi_sdist_url)"
-		S="${WORKDIR}/$(pypi_normalize_name "${PYPI_PN}")-$(pypi_translate_version "${PV}")"
+		SRC_URI="$(pypi_sdist_url "${PYPI_PN}" "${version}")"
+		S="${WORKDIR}/$(pypi_normalize_name "${PYPI_PN}")-${version}"
 	fi
 }
 
-- 
2.41.0

[gentoo-dev] [PATCH 4/7] pypi.eclass: Normalize names without subshell

[Michał Górny]

Provide an internal helper to normalize names without a subshell.
This gives 535 ops / s, so a further 44% speedup.

Signed-off-by: Michał Górny <mgorny@gentoo.org>
---
 eclass/pypi.eclass | 39 +++++++++++++++++++++++++++------------
 1 file changed, 27 insertions(+), 12 deletions(-)

diff --git a/eclass/pypi.eclass b/eclass/pypi.eclass
index a8a179d5a3a4..d79e6f06fc1b 100644
--- a/eclass/pypi.eclass
+++ b/eclass/pypi.eclass
@@ -63,6 +63,21 @@ _PYPI_ECLASS=1
 # @CODE
 : "${PYPI_PN:=${PN}}"
 
+# @FUNCTION: _pypi_normalize_name
+# @INTERNAL
+# @USAGE: <name>
+# @DESCRIPTION:
+# Internal normalization function, returns the result
+# via _PYPI_NORMALIZED_NAME variable.
+_pypi_normalize_name() {
+	local name=${1}
+	local shopt_save=$(shopt -p extglob)
+	shopt -s extglob
+	name=${name//+([._-])/_}
+	${shopt_save}
+	_PYPI_NORMALIZED_NAME="${name,,}"
+}
+
 # @FUNCTION: pypi_normalize_name
 # @USAGE: <name>
 # @DESCRIPTION:
@@ -75,12 +90,9 @@ _PYPI_ECLASS=1
 pypi_normalize_name() {
 	[[ ${#} -ne 1 ]] && die "Usage: ${FUNCNAME} <name>"
 
-	local name=${1}
-	local shopt_save=$(shopt -p extglob)
-	shopt -s extglob
-	name=${name//+([._-])/_}
-	${shopt_save}
-	echo "${name,,}"
+	local _PYPI_NORMALIZED_NAME
+	_pypi_normalize_name "${@}"
+	echo "${_PYPI_NORMALIZED_NAME}"
 }
 
 # @FUNCTION: pypi_translate_version
@@ -137,10 +149,10 @@ pypi_sdist_url() {
 	local project=${1-"${PYPI_PN}"}
 	local version=${2-"$(pypi_translate_version "${PV}")"}
 	local suffix=${3-.tar.gz}
-	local fn_project=${project}
-	[[ ${normalize} ]] && fn_project=$(pypi_normalize_name "${project}")
+	local _PYPI_NORMALIZED_NAME=${project}
+	[[ ${normalize} ]] && _pypi_normalize_name "${_PYPI_NORMALIZED_NAME}"
 	printf "https://files.pythonhosted.org/packages/source/%s" \
-		"${project::1}/${project}/${fn_project}-${version}${suffix}"
+		"${project::1}/${project}/${_PYPI_NORMALIZED_NAME}-${version}${suffix}"
 }
 
 # @FUNCTION: pypi_wheel_name
@@ -167,11 +179,12 @@ pypi_wheel_name() {
 		die "Usage: ${FUNCNAME} <project> [<version> [<python-tag> [<abi-platform-tag>]]]"
 	fi
 
-	local project=$(pypi_normalize_name "${1-"${PYPI_PN}"}")
+	local _PYPI_NORMALIZED_NAME
+	_pypi_normalize_name "${1:-"${PYPI_PN}"}"
 	local version=${2-"$(pypi_translate_version "${PV}")"}
 	local pytag=${3-py3}
 	local abitag=${4-none-any}
-	echo "${project}-${version}-${pytag}-${abitag}.whl"
+	echo "${_PYPI_NORMALIZED_NAME}-${version}-${pytag}-${abitag}.whl"
 }
 
 # @FUNCTION: pypi_wheel_url
@@ -232,8 +245,10 @@ _pypi_set_globals() {
 		SRC_URI="$(pypi_sdist_url --no-normalize "${PYPI_PN}" "${version}")"
 		S="${WORKDIR}/${PYPI_PN}-${version}"
 	else
+		local _PYPI_NORMALIZED_NAME
+		_pypi_normalize_name "${PYPI_PN}"
 		SRC_URI="$(pypi_sdist_url "${PYPI_PN}" "${version}")"
-		S="${WORKDIR}/$(pypi_normalize_name "${PYPI_PN}")-${version}"
+		S="${WORKDIR}/${_PYPI_NORMALIZED_NAME}-${version}"
 	fi
 }
 
-- 
2.41.0

[gentoo-dev] [PATCH 5/7] pypi.eclass: Translate version without subshell in common case

[Michał Górny]

Provide an internal helper to translate versions without a subshell,
and use it in the common case.  Now the benchmark gives 685 ops / s,
which means it's another 28% speedup.

Signed-off-by: Michał Górny <mgorny@gentoo.org>
---
 eclass/pypi.eclass | 31 +++++++++++++++++++++----------
 1 file changed, 21 insertions(+), 10 deletions(-)

diff --git a/eclass/pypi.eclass b/eclass/pypi.eclass
index d79e6f06fc1b..c8ad2e03d6e8 100644
--- a/eclass/pypi.eclass
+++ b/eclass/pypi.eclass
@@ -95,6 +95,19 @@ pypi_normalize_name() {
 	echo "${_PYPI_NORMALIZED_NAME}"
 }
 
+# @FUNCTION: _pypi_translate_version
+# @USAGE: <version>
+# @DESCRIPTION:
+# Internal version translation function, returns the result
+# via _PYPI_TRANSLATED_VERSION variable.
+_pypi_translate_version() {
+	local version=${1}
+	version=${version/_alpha/a}
+	version=${version/_beta/b}
+	version=${version/_rc/rc}
+	_PYPI_TRANSLATED_VERSION=${version/_p/.post}
+}
+
 # @FUNCTION: pypi_translate_version
 # @USAGE: <version>
 # @DESCRIPTION:
@@ -106,12 +119,9 @@ pypi_normalize_name() {
 pypi_translate_version() {
 	[[ ${#} -ne 1 ]] && die "Usage: ${FUNCNAME} <version>"
 
-	local version=${1}
-	version=${version/_alpha/a}
-	version=${version/_beta/b}
-	version=${version/_rc/rc}
-	version=${version/_p/.post}
-	echo "${version}"
+	local _PYPI_TRANSLATED_VERSION
+	_pypi_translate_version "${@}"
+	echo "${_PYPI_TRANSLATED_VERSION}"
 }
 
 # @FUNCTION: pypi_sdist_url
@@ -239,16 +249,17 @@ pypi_wheel_url() {
 # @DESCRIPTION:
 # Set global variables, SRC_URI and S.
 _pypi_set_globals() {
-	local version=$(pypi_translate_version "${PV}")
+	local _PYPI_TRANSLATED_VERSION
+	_pypi_translate_version "${PV}"
 
 	if [[ ${PYPI_NO_NORMALIZE} ]]; then
-		SRC_URI="$(pypi_sdist_url --no-normalize "${PYPI_PN}" "${version}")"
+		SRC_URI="$(pypi_sdist_url --no-normalize "${PYPI_PN}" "${_PYPI_TRANSLATED_VERSION}")"
 		S="${WORKDIR}/${PYPI_PN}-${version}"
 	else
 		local _PYPI_NORMALIZED_NAME
 		_pypi_normalize_name "${PYPI_PN}"
-		SRC_URI="$(pypi_sdist_url "${PYPI_PN}" "${version}")"
-		S="${WORKDIR}/${_PYPI_NORMALIZED_NAME}-${version}"
+		SRC_URI="$(pypi_sdist_url "${PYPI_PN}" "${_PYPI_TRANSLATED_VERSION}")"
+		S="${WORKDIR}/${_PYPI_NORMALIZED_NAME}-${_PYPI_TRANSLATED_VERSION}"
 	fi
 }
 
-- 
2.41.0

[gentoo-dev] [PATCH 6/7] pypi.eclass: Replace pypi_sdist_url in global scope

[Michał Górny]

Introduce an internal helper for _pypi_sdist_url that doesn't require
subshell, and therefore eliminate all subshells from global scope.
We're nearing 952 ops / s, further 39% speedup.

Signed-off-by: Michał Górny <mgorny@gentoo.org>
---
 eclass/pypi.eclass | 53 +++++++++++++++++++++++++++++-----------------
 1 file changed, 33 insertions(+), 20 deletions(-)

diff --git a/eclass/pypi.eclass b/eclass/pypi.eclass
index c8ad2e03d6e8..eade3d955ab5 100644
--- a/eclass/pypi.eclass
+++ b/eclass/pypi.eclass
@@ -124,6 +124,31 @@ pypi_translate_version() {
 	echo "${_PYPI_TRANSLATED_VERSION}"
 }
 
+# @FUNCTION: _pypi_sdist_url
+# @INTERNAL
+# @USAGE: [--no-normalize] [<project> [<version> [<suffix>]]]
+# @DESCRIPTION:
+# Internal sdist generated, returns the result via _PYPI_SDIST_URL
+# variable.
+_pypi_sdist_url() {
+	local normalize=1
+	if [[ ${1} == --no-normalize ]]; then
+		normalize=
+		shift
+	fi
+
+	if [[ ${#} -gt 3 ]]; then
+		die "Usage: ${FUNCNAME} [--no-normalize] <project> [<version> [<suffix>]]"
+	fi
+
+	local project=${1-"${PYPI_PN}"}
+	local version=${2-"$(pypi_translate_version "${PV}")"}
+	local suffix=${3-.tar.gz}
+	local _PYPI_NORMALIZED_NAME=${project}
+	[[ ${normalize} ]] && _pypi_normalize_name "${_PYPI_NORMALIZED_NAME}"
+	_PYPI_SDIST_URL="https://files.pythonhosted.org/packages/source/${project::1}/${project}/${_PYPI_NORMALIZED_NAME}-${version}${suffix}"
+}
+
 # @FUNCTION: pypi_sdist_url
 # @USAGE: [--no-normalize] [<project> [<version> [<suffix>]]]
 # @DESCRIPTION:
@@ -146,23 +171,9 @@ pypi_translate_version() {
 # If <format> is unspecified, it defaults to ".tar.gz".  Another valid
 # value is ".zip" (please remember to add a BDEPEND on app-arch/unzip).
 pypi_sdist_url() {
-	local normalize=1
-	if [[ ${1} == --no-normalize ]]; then
-		normalize=
-		shift
-	fi
-
-	if [[ ${#} -gt 3 ]]; then
-		die "Usage: ${FUNCNAME} [--no-normalize] <project> [<version> [<suffix>]]"
-	fi
-
-	local project=${1-"${PYPI_PN}"}
-	local version=${2-"$(pypi_translate_version "${PV}")"}
-	local suffix=${3-.tar.gz}
-	local _PYPI_NORMALIZED_NAME=${project}
-	[[ ${normalize} ]] && _pypi_normalize_name "${_PYPI_NORMALIZED_NAME}"
-	printf "https://files.pythonhosted.org/packages/source/%s" \
-		"${project::1}/${project}/${_PYPI_NORMALIZED_NAME}-${version}${suffix}"
+	local _PYPI_SDIST_URL
+	_pypi_sdist_url "${@}"
+	echo "${_PYPI_SDIST_URL}"
 }
 
 # @FUNCTION: pypi_wheel_name
@@ -249,18 +260,20 @@ pypi_wheel_url() {
 # @DESCRIPTION:
 # Set global variables, SRC_URI and S.
 _pypi_set_globals() {
-	local _PYPI_TRANSLATED_VERSION
+	local _PYPI_SDIST_URL _PYPI_TRANSLATED_VERSION
 	_pypi_translate_version "${PV}"
 
 	if [[ ${PYPI_NO_NORMALIZE} ]]; then
-		SRC_URI="$(pypi_sdist_url --no-normalize "${PYPI_PN}" "${_PYPI_TRANSLATED_VERSION}")"
+		_pypi_sdist_url --no-normalize "${PYPI_PN}" "${_PYPI_TRANSLATED_VERSION}"
 		S="${WORKDIR}/${PYPI_PN}-${version}"
 	else
 		local _PYPI_NORMALIZED_NAME
 		_pypi_normalize_name "${PYPI_PN}"
-		SRC_URI="$(pypi_sdist_url "${PYPI_PN}" "${_PYPI_TRANSLATED_VERSION}")"
+		_pypi_sdist_url "${PYPI_PN}" "${_PYPI_TRANSLATED_VERSION}"
 		S="${WORKDIR}/${_PYPI_NORMALIZED_NAME}-${_PYPI_TRANSLATED_VERSION}"
 	fi
+
+	SRC_URI=${_PYPI_SDIST_URL}
 }
 
 _pypi_set_globals
-- 
2.41.0

[gentoo-dev] [PATCH 7/7] pypi.eclass: Avoid subshell for extglob setting

[Michał Górny]

Suggested by Eli Schwartz.  This gives roughly 5260 ops / s, over 550%
speedup.

The complete patch series therefore increases the speed from roughly
326 ops / s to 5260 ops / s, making the common case 16 times faster.

Closes: https://bugs.gentoo.org/908411
Closes: https://github.com/gentoo/gentoo/pull/31404
Signed-off-by: Michał Górny <mgorny@gentoo.org>
---
 eclass/pypi.eclass | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/eclass/pypi.eclass b/eclass/pypi.eclass
index eade3d955ab5..618fde5e3de8 100644
--- a/eclass/pypi.eclass
+++ b/eclass/pypi.eclass
@@ -71,10 +71,13 @@ _PYPI_ECLASS=1
 # via _PYPI_NORMALIZED_NAME variable.
 _pypi_normalize_name() {
 	local name=${1}
-	local shopt_save=$(shopt -p extglob)
-	shopt -s extglob
+	local prev_extglob=-s
+	if ! shopt -p extglob >/dev/null; then
+		prev_extglob=-u
+		shopt -s extglob
+	fi
 	name=${name//+([._-])/_}
-	${shopt_save}
+	shopt "${prev_extglob}" extglob
 	_PYPI_NORMALIZED_NAME="${name,,}"
 }
 
-- 
2.41.0

Re: [gentoo-dev] [PATCH 7/7] pypi.eclass: Avoid subshell for extglob setting

[Ulrich Mueller]

[-- Attachment #1: Type: text/plain, Size: 792 bytes --]

>>>>> On Tue, 13 Jun 2023, Michał Górny wrote:

>  _pypi_normalize_name() {
>  	local name=${1}
> -	local shopt_save=$(shopt -p extglob)
> -	shopt -s extglob
> +	local prev_extglob=-s
> +	if ! shopt -p extglob >/dev/null; then
> +		prev_extglob=-u
> +		shopt -s extglob
> +	fi
>  	name=${name//+([._-])/_}
> -	${shopt_save}
> +	shopt "${prev_extglob}" extglob
>  	_PYPI_NORMALIZED_NAME="${name,,}"
>  }

In principle you could also do something like this:

	if shopt -pq extglob; then
		name=${name//+([._-])/_}
	else
		shopt -s extglob
		name=${name//+([._-])/_}
		shopt -u extglob
	fi

It duplicates one line of code, but saves a variable and IMHO the code
would be easier to understand.

Also, with the -q option no output redirection is needed.

Ulrich

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 507 bytes --]

Re: [gentoo-dev] [PATCH 7/7] pypi.eclass: Avoid subshell for extglob setting

[Michał Górny]

On Tue, 2023-06-13 at 11:07 +0200, Ulrich Mueller wrote:
> > > > > > On Tue, 13 Jun 2023, Michał Górny wrote:
> 
> >  _pypi_normalize_name() {
> >  	local name=${1}
> > -	local shopt_save=$(shopt -p extglob)
> > -	shopt -s extglob
> > +	local prev_extglob=-s
> > +	if ! shopt -p extglob >/dev/null; then
> > +		prev_extglob=-u
> > +		shopt -s extglob
> > +	fi
> >  	name=${name//+([._-])/_}
> > -	${shopt_save}
> > +	shopt "${prev_extglob}" extglob
> >  	_PYPI_NORMALIZED_NAME="${name,,}"
> >  }
> 
> In principle you could also do something like this:
> 
> 	if shopt -pq extglob; then
> 		name=${name//+([._-])/_}
> 	else
> 		shopt -s extglob
> 		name=${name//+([._-])/_}
> 		shopt -u extglob
> 	fi
> 
> It duplicates one line of code, but saves a variable and IMHO the code
> would be easier to understand.
> 

I was thinking about this but I really dislike repeating the logic
twice.  In my opinion, having such block would be confusing: why are
there two logics for extglob on and off?  Why are both the same?  Is
this some mistake?

Even though this is unlikely, someone could actually end up creating
a missync there, and things would go downhill from there.

-q is a good idea though.

Ideally, we'd avoid extglob at all but I can't think of another pure
bash way of doing this.

-- 
Best regards,
Michał Górny