Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

GH-105848: Simplify the arrangement ofCALL's stack#107788

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
brandtbucher merged 6 commits intopython:mainfrombrandtbucher:calls
Aug 9, 2023

Conversation

@brandtbucher
Copy link
Member

@brandtbucherbrandtbucher commentedAug 8, 2023•
edited by bedevere-bot
Loading

Instead of supporting both[callable, self, args...] and[NULL, callable, args...], changeCALL's stack to be[callable, self_or_null, args...]. This always puts the desired callable at the same location on the stack, simplifying (or removing) the resulting shuffles and adjustments necessary for the shared code paths that follow.

This also changesCALL_FUNCTION_EX,DICT_MERGE,LOAD_ATTR,LOAD_GLOBAL,LOAD_SUPER_ATTR to support the new stack layout. In some cases, I've also changed the names of the stack items for consistency within a family.

As a side-effect of the new layout,LOAD_ATTR_PROPERTY andLOAD_ATTR_GETATTRIBUTE_OVERRIDDEN (which are both implemented as inlined calls) don't support pushing an additional item to the stack for a followingCALL (oparg & 1) anymore. I think this is fine, since it seems rare to call a "method" that's looked up in this manner.

Copy link
Member

@gvanrossumgvanrossum left a comment

Choose a reason for hiding this comment

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

I think this can go in just like this.

Comment on lines -1092 to +1093
if (oparg&1) {stack_pointer[-1- (oparg&1 ?1 :0)]=null; }
stack_pointer[-1]=v;
stack_pointer[-1- (oparg&1 ?1 :0)]=res;
if (oparg&1) {stack_pointer[-(oparg&1 ?1 :0)]=null; }
Copy link
Member

Choose a reason for hiding this comment

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

Theoretically this could be slightly less performant than the original (not that it matters, and the C compiler might see right through this).

brandtbucher reacted with thumbs up emoji
}

inst(DICT_MERGE, (update--)) {
PyObject*dict=PEEK(oparg+1);// update is still on the stack
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we could make the DSL less of a lie, e.g. using

inst(DICT_MERGE, (dict, unused[oparg-1], update -- dict, unused[oparg-1])) {

?

(I guess the peeks in the error handling code suggest it's even more complicated.)

brandtbucher reacted with thumbs up emoji
Copy link
MemberAuthor

Choose a reason for hiding this comment

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

I figured the presence of thePEEK was an indicator that someone had tried that already. Maybe not, though... let me see!

Copy link
Member

Choose a reason for hiding this comment

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

Maybe it was an oversight. Maybe the code generator didn't use to handle this case.

I tried what I suggested and it seems to work. I didn't try to figure out what to do for the error handling (probably some more dummies belowdict).

};

inst(LOAD_SUPER_ATTR, (unused/1,global_super,class,self--res2if (oparg&1),res)) {
inst(LOAD_SUPER_ATTR, (unused/1,global_super,class,self--attr,self_or_nullif (oparg&1))) {
Copy link
Member

Choose a reason for hiding this comment

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

self_or_null is unused, so replace withunused?

brandtbucher reacted with thumbs up emoji
Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Puttingunused here breaks things. Since this is named it's set toNULL in the generated code... if it's calledunused, it just ignores it and now there's a garbage value there.

Copy link
Member

Choose a reason for hiding this comment

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

Oh sneaky. I think you found a hole in the code generator. If you want it to (conditionally) push NULL you should initialize it, not depend on the default initialization. IIRC the default initialization is there only to avoid C compiler warnings.

brandtbucher reacted with thumbs up emoji
};

inst(LOAD_SUPER_ATTR_ATTR, (unused/1,global_super,class,self--res2if (oparg&1),res)) {
inst(LOAD_SUPER_ATTR_ATTR, (unused/1,global_super,class,self--attr,self_or_nullif (oparg&1))) {
Copy link
Member

Choose a reason for hiding this comment

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

Ditto.

brandtbucher reacted with thumbs up emoji

inst(INSTRUMENTED_CALL, (-- )) {
intis_meth=PEEK(oparg+2)!=NULL;
intis_meth=PEEK(oparg+1)!=NULL;
Copy link
Member

Choose a reason for hiding this comment

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

Yes!

@gvanrossum
Copy link
Member

How about some buildbot runs?

brandtbucher reacted with thumbs up emoji

@brandtbucherbrandtbucher added 🔨 test-with-buildbotsTest PR w/ buildbots; report in status section 🔨 test-with-refleak-buildbotsTest PR w/ refleak buildbots; report in status section labelsAug 8, 2023
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by@brandtbucher for commitfd6607f 🤖

If you want to schedule another build, you need to add the🔨 test-with-buildbots label again.

@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by@brandtbucher for commitfd6607f 🤖

If you want to schedule another build, you need to add the🔨 test-with-refleak-buildbots label again.

@bedevere-botbedevere-bot removed 🔨 test-with-refleak-buildbotsTest PR w/ refleak buildbots; report in status section 🔨 test-with-buildbotsTest PR w/ buildbots; report in status section labelsAug 8, 2023
@brandtbucher
Copy link
MemberAuthor

Perf isneutral.

@brandtbucherbrandtbucher added 🔨 test-with-buildbotsTest PR w/ buildbots; report in status section 🔨 test-with-refleak-buildbotsTest PR w/ refleak buildbots; report in status section labelsAug 9, 2023
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by@brandtbucher for commita5a2d8f 🤖

If you want to schedule another build, you need to add the🔨 test-with-buildbots label again.

@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by@brandtbucher for commita5a2d8f 🤖

If you want to schedule another build, you need to add the🔨 test-with-refleak-buildbots label again.

@bedevere-botbedevere-bot removed 🔨 test-with-buildbotsTest PR w/ buildbots; report in status section 🔨 test-with-refleak-buildbotsTest PR w/ refleak buildbots; report in status section labelsAug 9, 2023
@markshannon
Copy link
Member

No new buildbot failures.

Copy link
Member

@markshannonmarkshannon left a comment

Choose a reason for hiding this comment

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

A bunch of minor style issues, but nothing blocking.

inttotal_args=oparg;
if (is_meth) {
callable=method;
if (self_or_null) {
Copy link
Member

Choose a reason for hiding this comment

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

This is perfectly correct, but could you make this consistent with the other NULL checks and useself_or_null != NULL to make it clear to the reader that this is aNULL check not a test of a boolean/int flag.

brandtbucher reacted with thumbs up emoji
intargcount=oparg;
if (is_meth) {
callable=method;
if (self_or_null) {
Copy link
Member

Choose a reason for hiding this comment

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

ditto

brandtbucher reacted with thumbs up emoji
inttotal_args=oparg;
if (is_meth) {
callable=method;
if (self_or_null) {
Copy link
Member

Choose a reason for hiding this comment

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

ditto

brandtbucher reacted with thumbs up emoji
inttotal_args=oparg;
if (is_meth) {
callable=method;
if (self_or_null) {
Copy link
Member

Choose a reason for hiding this comment

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

likewise

brandtbucher reacted with thumbs up emoji
inttotal_args=oparg;
if (is_meth) {
callable=method;
if (self_or_null) {
Copy link
Member

Choose a reason for hiding this comment

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

and here

brandtbucher reacted with thumbs up emoji
inst(CALL_METHOD_DESCRIPTOR_FAST_WITH_KEYWORDS, (unused/1,unused/2,callable,self_or_null,args[oparg]--res)) {
inttotal_args=oparg;
if (is_meth) {
if (self_or_null) {
Copy link
Member

Choose a reason for hiding this comment

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

another

brandtbucher reacted with thumbs up emoji
intis_meth=method!=NULL;
inttotal_args=oparg;
if (is_meth) {
if (self_or_null) {
Copy link
Member

Choose a reason for hiding this comment

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

almost the last one

brandtbucher reacted with thumbs up emoji
intis_meth=method!=NULL;
inttotal_args=oparg;
if (is_meth) {
if (self_or_null) {
Copy link
Member

Choose a reason for hiding this comment

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

the last one

brandtbucher reacted with thumbs up emoji
Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Actually, you missed one. But don't worry, I got it. ;)

}

inst(LOAD_ATTR_METHOD_WITH_VALUES, (unused/1,type_version/2,keys_version/2,descr/4,self--res2if (1),res)) {
inst(LOAD_ATTR_METHOD_WITH_VALUES, (unused/1,type_version/2,keys_version/2,descr/4,self--attr,self_or_nullif (1))) {
Copy link
Member

Choose a reason for hiding this comment

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

This isn'tself_or_null, it's justself.

brandtbucher reacted with thumbs up emoji
};

inst(LOAD_SUPER_ATTR_ATTR, (unused/1,global_super,class,self--res2if (oparg&1),res)) {
inst(LOAD_SUPER_ATTR_ATTR, (unused/1,global_super,class,self--attr,unusedif (oparg&1))) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
inst(LOAD_SUPER_ATTR_ATTR, (unused/1,global_super,class,self--attr,unusedif (oparg&1))) {
inst(LOAD_SUPER_ATTR_ATTR, (unused/1,global_super,class,self--attr,unusedif (0))) {

brandtbucher reacted with thumbs up emoji
}

inst(LOAD_ATTR_PROPERTY, (unused/1,type_version/2,func_version/2,fget/4,owner--unusedif (oparg&1),unused)) {
inst(LOAD_ATTR_PROPERTY, (unused/1,type_version/2,func_version/2,fget/4,owner--unused,unusedif (oparg&1))) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
inst(LOAD_ATTR_PROPERTY, (unused/1,type_version/2,func_version/2,fget/4,owner--unused,unusedif (oparg&1))) {
inst(LOAD_ATTR_PROPERTY, (unused/1,type_version/2,func_version/2,fget/4,owner--unused,unusedif (0))) {

brandtbucher reacted with thumbs up emoji
@brandtbucherbrandtbucherenabled auto-merge (squash)August 9, 2023 17:54
@brandtbucherbrandtbucher merged commita9caf9c intopython:mainAug 9, 2023
facebook-github-bot pushed a commit to facebookincubator/cinderx that referenced this pull requestAug 22, 2025
Summary:This reflects the changes inpython/cpython#107788.The changes to the relevant `LOAD_` instructions are mostly covered by updating `LoadMethodResult` to have a non-default constructor in 3.14+ which normalizes the content.Reviewed By: alexmalyshevDifferential Revision: D80667929fbshipit-source-id: 2d62f1850056792695faf8d9ceea61a0b4b31253
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@markshannonmarkshannonmarkshannon left review comments

@gvanrossumgvanrossumgvanrossum approved these changes

@iritkatrieliritkatrielAwaiting requested review from iritkatrieliritkatriel is a code owner

@brettcannonbrettcannonAwaiting requested review from brettcannonbrettcannon is a code owner

@ericsnowcurrentlyericsnowcurrentlyAwaiting requested review from ericsnowcurrentlyericsnowcurrently is a code owner

@ncoghlanncoghlanAwaiting requested review from ncoghlanncoghlan is a code owner

@warsawwarsawAwaiting requested review from warsawwarsaw is a code owner

Assignees

@brandtbucherbrandtbucher

Labels

interpreter-core(Objects, Python, Grammar, and Parser dirs)performancePerformance or resource usage

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@brandtbucher@gvanrossum@bedevere-bot@markshannon
[8]ページ先頭
©2009-2025 Movatter.jp