This issue tracker has been migrated to GitHub, and is currently read-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.

classification
Title: Remove redundant macros used for stack manipulation in interpreter
Type: performance Stage: resolved
Components: Interpreter Core Versions:
process
Status: closed Resolution: wont fix
Dependencies: Superseder:
Assigned To: Nosy List: Dennis Sweeney, Mark.Shannon, corona10, eric.smith, rhettinger, shihai1991, terry.reedy
Priority: normal Keywords: easy (C), patch

Created on 2020-06-09 10:13 by Mark.Shannon, last changed 2022-04-11 14:59 by admin. This issue is now closed.

Files
File name Uploaded Description Edit
perf_diff.txt Dennis Sweeney, 2020-06-13 00:00 Performance changes from PR 20845
Pull Requests
URL Status Linked Edit
PR 20783 merged corona10, 2020-06-10 16:26
PR 20845 closed Dennis Sweeney, 2020-06-12 22:16
Messages (9)
msg371087 - (view) Author: Mark Shannon (Mark.Shannon) * (Python committer) Date: 2020-06-09 10:13
Currently, there are fourteen macros provided for stack manipulation.
Along with the fundamental, POP(), PUSH() and PEEK(n) the following eleven are also provided:

TOP()          
SECOND()       
THIRD()        
FOURTH()       
SET_TOP(v)     
SET_SECOND(v)  
SET_THIRD(v)   
SET_FOURTH(v)  
SET_VALUE(n, v)
STACK_GROW(n)
STACK_SHRINK(n) 

These are unnecessary as compilers have long understood the sequences of pointer increments and decrements that is generated by using POPs and PUSHes.

See https://godbolt.org/z/Htw-2k which shows GCC generating exactly the same code from just POP, PUSH and PEEK as from direct access to items on the stack.

Removing the redundant macros would make the code easier to reason about and analyze.
TOP() and SECOND() are used quite heavily and are trivial to convert to PEEK by tools, so should probably be left alone.

Notes:

I'm ignoring the stack debugging macros here, they aren't a problem.
The EMPTY() macro is only used twice, so might as well be replaced with STACK_LEVEL == 0.
Sadly, there is no "maintainability/code quality" selection for "type" of issue, so I've chosen "performance" :(
msg371130 - (view) Author: Eric V. Smith (eric.smith) * (Python committer) Date: 2020-06-09 17:30
What type of changes are you proposing to make?
msg371183 - (view) Author: Mark Shannon (Mark.Shannon) * (Python committer) Date: 2020-06-10 12:01
I'm proposing that we remove the nine macros above; the eleven listed, except TOP() and SECOND().
They can be replaced by POP(), PUSH() and PEEK() without lost of functionality or performance.
msg371217 - (view) Author: Dong-hee Na (corona10) * (Python committer) Date: 2020-06-10 16:21
AFAIK, SET_VALUE is not used in CPython master codebase.
msg371259 - (view) Author: Mark Shannon (Mark.Shannon) * (Python committer) Date: 2020-06-11 09:39
New changeset 33faf5c4f43e24766cf567bec89ad4c7f1491ff7 by Dong-hee Na in branch 'master':
bpo-40925: Remove unused stack macro SET_VALUE (GH-20783)
https://github.com/python/cpython/commit/33faf5c4f43e24766cf567bec89ad4c7f1491ff7
msg371423 - (view) Author: Dennis Sweeney (Dennis Sweeney) * (Python committer) Date: 2020-06-12 22:18
I just added PR 20845, but I'm concerned about performance. I'm attaching the results of a run of pyperformance before and after PR 20845.
msg371430 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2020-06-13 01:28
I don't see any advantage to making this change.  The current code is readable and the macros have an obvious interpretation.  We also know that they generate clean code on every compiler we've come across and on 32 bits.

Also, this should be marked as type, "performance".  If performance changes at all, it will likely be a net detriment.
msg371447 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2020-06-13 10:24
I could understand wanting to replace TOP,...,FOURTH with PEEK(n) and the SET_XYZs with the now deleted SET(_VALUE)(n, v), but to me, replacing PEEK-SET with POPs and PUSHes (in different orders) that directly mean something different but that compilers (all?) optimize to mean the same thing makes the code initially harder to read.  In particular, the old version of TARGET(ROT_FOUR) is initially clearer without thinking through the effect of the particular order of pushes in the new version.  It depends on what one is accustomed to.
msg371456 - (view) Author: Mark Shannon (Mark.Shannon) * (Python committer) Date: 2020-06-13 13:09
Dennis, thanks for doing the benchmarking.
You seem to have removed rather more macros than I suggested,
but it probably won't make much difference to the results.

Since this may have a negative effect on performance and doesn't seem to be popular, let's drop it.

For the record, my motivation for this change is for tooling.
Instructions that only use POP, PEEK and PUSH, are much easier to analyze for tools like verifiers and optimizers.

Such "nice to have" features aren't worth the slowdown.
History
Date User Action Args
2022-04-11 14:59:32adminsetgithub: 85097
2020-06-13 13:10:03Mark.Shannonsetstatus: open -> closed
stage: patch review -> resolved
2020-06-13 13:09:54Mark.Shannonsetresolution: wont fix
messages: + msg371456
2020-06-13 10:24:02terry.reedysetnosy: + terry.reedy
messages: + msg371447
2020-06-13 05:30:56shihai1991setnosy: + shihai1991
2020-06-13 01:28:36rhettingersetnosy: + rhettinger
messages: + msg371430
2020-06-13 00:00:44Dennis Sweeneysetfiles: - pushpop_perf.txt
2020-06-13 00:00:32Dennis Sweeneysetfiles: - master_perf.txt
2020-06-13 00:00:23Dennis Sweeneysetfiles: + perf_diff.txt
2020-06-12 22:19:13Dennis Sweeneysetfiles: + master_perf.txt
2020-06-12 22:18:33Dennis Sweeneysetfiles: + pushpop_perf.txt

messages: + msg371423
2020-06-12 22:16:30Dennis Sweeneysetnosy: + Dennis Sweeney
pull_requests: + pull_request20038
2020-06-11 09:39:24Mark.Shannonsetmessages: + msg371259
2020-06-10 16:26:24corona10setkeywords: + patch
stage: needs patch -> patch review
pull_requests: + pull_request19979
2020-06-10 16:21:37corona10setmessages: + msg371217
2020-06-10 16:18:11corona10setnosy: + corona10
2020-06-10 12:01:23Mark.Shannonsetmessages: + msg371183
2020-06-09 17:30:35eric.smithsetnosy: + eric.smith
messages: + msg371130
2020-06-09 10:13:40Mark.Shannoncreate