Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gh-131738: optimize builtin any/all/tuple calls with a generator expression arg #131737

Merged
merged 17 commits into from
Mar 28, 2025

Conversation

iritkatriel
Copy link
Member

@iritkatriel iritkatriel commented Mar 25, 2025

@iritkatriel iritkatriel changed the title optimize any/all/tuple calls with a generator expression arg gh-131738: optimize builtin any/all/tuple calls with a generator expression arg Mar 25, 2025
@TeamSpen210
Copy link

I don't think LOAD_COMMON_CONST is safe right now, since users could have messed with the builtins module. Probably better to stash the funcs in interp->callable_cache like other builtins used for optimisation?

Copy link
Member

@brandtbucher brandtbucher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very cool! Any plans to do this for set or list?

dict/max/min/sum are trickier, but may be worth exploring too.

@bedevere-app
Copy link

bedevere-app bot commented Mar 25, 2025

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

@brandtbucher
Copy link
Member

This should fix the JIT builds:

diff --git a/Tools/jit/_targets.py b/Tools/jit/_targets.py
index aa2b56abf44..73a4210eee0 100644
--- a/Tools/jit/_targets.py
+++ b/Tools/jit/_targets.py
@@ -522,7 +522,14 @@ def get_target(host: str) -> _COFF | _ELF | _MachO:
         args = ["-fms-runtime-lib=dll"]
         target = _COFF(host, args=args)
     elif re.fullmatch(r"x86_64-.*-linux-gnu", host):
-        args = ["-fno-pic", "-mcmodel=medium", "-mlarge-data-threshold=0"]
+        args = [
+            # Jump tables generate R_X86_64_32S relocations, which assume a
+            # signed 32-bit address space:
+            "-fno-jump-tables",
+            "-fno-pic",
+            "-mcmodel=medium",
+            "-mlarge-data-threshold=0",
+        ]
         target = _ELF(host, args=args)
     else:
         raise ValueError(host)

@markshannon
Copy link
Member

The overall approach looks good.

Do you have stats for this PR?
If there is enough of an increase in the execution counts for CALL_INTRINSIC_1 then we should move CALL_INTRINSIC_1 INTRINSIC_LIST_TO_TUPLE into a new LIST_TO_TUPLE instruction.

@rhettinger
Copy link
Contributor

dict/max/min/sum are trickier, but may be worth exploring too.

The latter three would be nice wins.

@iritkatriel
Copy link
Member Author

@iritkatriel
Copy link
Member Author

Do you have stats for this PR? If there is enough of an increase in the execution counts for CALL_INTRINSIC_1 then we should move CALL_INTRINSIC_1 INTRINSIC_LIST_TO_TUPLE into a new LIST_TO_TUPLE instruction.

image

@iritkatriel
Copy link
Member Author

I have made the requested changes; please review again.

@bedevere-app
Copy link

bedevere-app bot commented Mar 27, 2025

Thanks for making the requested changes!

@brandtbucher: please review the changes made to this pull request.

@bedevere-app bedevere-app bot requested a review from brandtbucher March 27, 2025 11:10
Copy link
Member

@brandtbucher brandtbucher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. You can revert the JIT change, it's not needed anymore.

(Also I'm running JIT benchmarks just because I'm curious, but no need to block on those results at all.)

@@ -19,6 +19,8 @@
#include "pycore_warnings.h" // _PyErr_WarnUnawaitedCoroutine()


#include "opcode_ids.h" // RESUME, etc
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this needed?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I had to remove this include from another header so it needs to be included when needed.

@brandtbucher
Copy link
Member

(Also, I think you need to make regen-cases.)

@iritkatriel iritkatriel enabled auto-merge (squash) March 28, 2025 10:13
@iritkatriel iritkatriel merged commit 2c8f329 into python:main Mar 28, 2025
64 checks passed
diegorusso pushed a commit to diegorusso/cpython that referenced this pull request Apr 1, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants