Movatterモバイル変換

Skip to content

python/cpythonPublic

NotificationsYou must be signed in to change notification settings
Fork32k
Star67.3k

GH-131798: Remove JIT guards for`dict`,`frozenset`,`list`,`set`, and`tuple`#132289

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.

Sign up for GitHub

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

Already on GitHub?Sign in to your account

Merged

brandtbucher merged 7 commits intopython:mainfrombrandtbucher:jit-guard-dict-tuple-list

Apr 9, 2025

Merged

GH-131798: Remove JIT guards for`dict`,`frozenset`,`list`,`set`, and`tuple`#132289

brandtbucher merged 7 commits intopython:mainfrombrandtbucher:jit-guard-dict-tuple-list

Apr 9, 2025

Conversation

brandtbucher

Copy link

Member

brandtbucher commentedApr 8, 2025•
edited by bedevere-appbot
Loading

According to thestats, this removes:

144 milliontuple guards (25%)
280 millionset/frozenset guards (19%)
484 milliondict guards (42%)
1.3 billionlist guards (34%)

It also fixes a bug that I encountered in the optimizer's logic for_UNPACK_SEQUENCE_TUPLE, which puts items on the stack inreverse order.

Issue:Better uop coverage in the JIT optimizer #131798

brandtbucher added7 commits

April 1, 2025 18:04

@brandtbucher

Add guards for dicts, lists, sets, and tuples

10a16c5

@brandtbucher

Remove old sym_set_type call

99d6288

@brandtbucher

blurb add

a471ee3

@brandtbucher

Add regression tests

0efca44

@brandtbucher

Fix backward tuple unpacking

0f643c4

@brandtbucher

Catch up with main

790faa9

@brandtbucher

Catch up with main

972f74c

@brandtbucher

brandtbucher added performance

Performance or resource usage

interpreter-core

(Objects, Python, Grammar, and Parser dirs)

topic-JIT labels

@brandtbucher

brandtbucher self-assigned this

@brandtbucher

brandtbucher requested review fromFidget-Spinner andmarkshannon ascode owners

April 8, 2025 18:55

@bedevere-app

bedevere-appbot added the awaiting core review label

@bedevere-app

bedevere-appbot mentioned this pull request

Better uop coverage in the JIT optimizer#131798

Open

@brandtbucher

Copy link

MemberAuthor

brandtbucher commentedApr 8, 2025

@Zheaoli and@tomasr8, you might be interested in checking this out. Specifically, see the changes inPython/bytecodes.c which use the same syntax asPython/optimizer_bytecodes.c, but describes theactual implementations of these instructions.

You can see that this PR breaks up the old, largerinstr definitions into smallerop definitions combined into amacro. This doesn't change the semantics of the instruction itself, but allows the JIT to remove parts of the instruction as it sees fit (in this case, separating out the type checks allows the JIT to remove them).

Fidget-Spinner reviewed

View reviewed changes

Copy link

Member

Fidget-Spinner left a comment

Choose a reason for hiding this comment

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

Just one concern.

Lib/test/test_capi/test_opt.pyShow resolvedHide resolved

Python/optimizer_bytecodes.c

		@@ -396,7 +396,6 @@ dummy_func(void) {
		op(_TO_BOOL_LIST, (value -- res)) {
		int already_bool = optimize_to_bool(this_instr, ctx, value, &res);
		if (!already_bool) {
		sym_set_type(value, &PyList_Type);

Copy link

Member

Fidget-SpinnerApr 8, 2025

Choose a reason for hiding this comment

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

Does CONTAIN_OP_SET and CONTAIN_OP_DICT need to remove their sym setting type as well? Or is that not even happening at the moment?

Copy link

MemberAuthor

brandtbucherApr 8, 2025

Choose a reason for hiding this comment

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

Not happening currently,._CONTAINS_OP_SET can't even set anything, since it handles bothset andfrozenset (this might be worth re-evaluating, or splitting up).

@Zheaoli

Copy link

Contributor

Zheaoli commentedApr 8, 2025

@Zheaoli and@tomasr8, you might be interested in checking this out. Specifically, see the changes inPython/bytecodes.c which use the same syntax asPython/optimizer_bytecodes.c, but describes theactual implementations of these instructions.
You can see that this PR breaks up the old, largerinstr definitions into smallerop definitions combined into amacro. This doesn't change the semantics of the instruction itself, but allows the JIT to remove parts of the instruction as it sees fit (in this case, separating out the type checks allows the JIT to remove them).

Thanks! I think I might need some time to understand this PR! Thanks for your patience!

Fidget-Spinner approved these changes

View reviewed changes

@bedevere-app

bedevere-appbot added awaiting merge and removed awaiting core review labels

@brandtbucher

Copy link

MemberAuthor

brandtbucher commentedApr 9, 2025

Going to go ahead and merge this to reduce the risk of merge conflicts, but let me know if you have any questions@Zheaoli!

@brandtbucher

brandtbucher merged commit20926c7 intopython:main

76 checks passed

@bedevere-app

bedevere-appbot removed the awaiting merge label

@Zheaoli

Zheaoli mentioned this pull request

gh-131798: Usesym_new_type instead ofsym_new_not_null for_BUILD_LIST,_BUILD_SLICE, and_BUILD_MAP#132434

Merged

seehwan pushed a commit to seehwan/cpython that referenced this pull request

@brandtbucher

@seehwan80

pythonGH-131798: Remove JIT guards for dict, frozenset, list, set, an…

cd4602d

…d tuple (pythonGH-132289)

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Labels

interpreter-core

(Objects, Python, Grammar, and Parser dirs)

Performance or resource usage

3 participants

@brandtbucher

@Zheaoli

@Fidget-Spinner

[8]ページ先頭

©2009-2025 Movatter.jp