build: use list for mutable retval rather than tuple #41372
Conversation
We define `retval` as a tuple and then replace the tuple by "appending" items with `+=` but that actually creates a new tuple every time. Because it is intended to be mutable, use a list instead, then return a tuple from the function, as it should be immutable outside the function.
This comment has been hidden.
This comment has been hidden.
Co-authored-by: Mestery <mestery@pm.me>
Approve. Under heavy use, the proposed syntax is a nice performance boost...
% py -3.10 # 100k cycles on an M1 Mac...
Python 3.10.1 (main, Dec 6 2021, 22:18:13) [Clang 13.0.0 (clang-1300.0.29.3)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> from timeit import timeit
>>> print(timeit("abc += ['d']", "abc = list('abc')", number=100_000))
0.011850042035803199
>>> print(timeit("abc.append('d')", "abc = list('abc')", number=100_000))
0.010260708979330957
>>> print(timeit("abc += ('d', )", "abc = tuple('abc')", number=100_000))
20.874202583101578
Commit Queue failed- Loading data for nodejs/node/pull/41372 ✔ Done loading data for nodejs/node/pull/41372 ----------------------------------- PR info ------------------------------------ Title build: use list for mutable retval rather than tuple (#41372) Author Rich Trott (@Trott) Branch Trott:python-nit -> nodejs:master Labels python, author ready Commits 2 - build: use list for mutable retval rather than tuple - Update configure.py Committers 2 - Rich Trott - GitHub PR-URL: https://github.com/nodejs/node/pull/41372 Reviewed-By: Christian Clauss Reviewed-By: Tobias Nießen ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/41372 Reviewed-By: Christian Clauss Reviewed-By: Tobias Nießen -------------------------------------------------------------------------------- ℹ This PR was created on Sat, 01 Jan 2022 17:10:54 GMT ✔ Approvals: 2 ✔ - Christian Clauss (@cclauss): https://github.com/nodejs/node/pull/41372#pullrequestreview-842349446 ✔ - Tobias Nießen (@tniessen) (TSC): https://github.com/nodejs/node/pull/41372#pullrequestreview-842435129 ✔ Last GitHub CI successful ℹ Last Full PR CI on 2022-01-01T17:24:05Z: https://ci.nodejs.org/job/node-test-pull-request/41718/ - Querying data for job/node-test-pull-request/41718/ ✔ Last Jenkins CI successful -------------------------------------------------------------------------------- ✔ No git cherry-pick in progress ✔ No git am in progress ✔ No git rebase in progress -------------------------------------------------------------------------------- - Bringing origin/master up to date... From https://github.com/nodejs/node * branch master -> FETCH_HEAD ✔ origin/master is now up-to-date - Downloading patch for 41372 From https://github.com/nodejs/node * branch refs/pull/41372/merge -> FETCH_HEAD ✔ Fetched commits as 513e9fd0b611..277dc43511ca -------------------------------------------------------------------------------- [master 30b065eb28] build: use list for mutable retval rather than tuple Author: Rich Trott Date: Sat Jan 1 09:05:03 2022 -0800 1 file changed, 3 insertions(+), 3 deletions(-) [master 2f121192c8] Update configure.py Author: Rich Trott Date: Sat Jan 1 09:23:34 2022 -0800 1 file changed, 1 insertion(+), 1 deletion(-) ✔ Patches applied There are 2 commits in the PR. Attempting autorebase. Rebasing (2/4)https://github.com/nodejs/node/actions/runs/1656698573 |
|
Landed in bd92726 |
This is a really small semantic Python nit. Pinging @nodejs/python in case I'm just wrong in my understanding or something.
We define
retvalas a tuple and then replace the tuple by "appending" items with+=but that actually creates a new tuple every time. Because it is intended to be mutable, use a list instead, then return a tuple from the function, as it should be immutable outside the function.The text was updated successfully, but these errors were encountered: