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

Faster tracing test #6

Closed
wants to merge 4 commits into from
Closed

Faster tracing test #6

wants to merge 4 commits into from

Conversation

markshannon
Copy link
Member

For review only

Copy link

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

Okay, I think I understand this now, and while it makes me uneasy, I also understand the point, so I guess I support it. But a few things gave me pause...

Python/ceval.c Outdated
Comment on lines 1301 to 1303
#define TARGET(op) \
op: \
TARGET_##op

Choose a reason for hiding this comment

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

Maybe make this macro a one-liner too? It would seem to fit. :-)

Python/ceval.c Outdated
#else
#define TARGET(op) op
#define DISPATCH_GOTO goto dispatch_opcode;

Choose a reason for hiding this comment

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

For stylistic reasons (consistency with e.g. DISPATCH()) I'd prefer making this a function macro, and moving the semicolon to after the "call". Seeing DISPATCH_GOTO (line 1317) without a semicolon makes my mind do a double-take each time I see it.

CFrame *prev_cframe = tstate->cframe;
trace_info.cframe.use_tracing = prev_cframe->use_tracing;
trace_info.cframe.previous = prev_cframe;
tstate->cframe = &trace_info.cframe;

Choose a reason for hiding this comment

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

Hm, this is disturbing -- tstate (which is pretty much a global structure) now has a pointer to a local variable. That means you have to restore this on every exit from the function. Now, we know there's only one exit, and it's handled there, but it still makes me feel weird. And all this to make it possible to set a single int (perhaps even a single bit) from "outside" this function.

Copy link
Member Author

Choose a reason for hiding this comment

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

I know, and it would seem like overkill, if it weren't for how critical reads of that single int are.
That one int gets read 100 million times per second, and the code to read it occurs over 100 times in the interpreter.

@markshannon
Copy link
Member Author

I've made the relevant changes to python#25244
I'll apply the others to the upcoming PR.

@markshannon markshannon force-pushed the faster-tracing-test branch 3 times, most recently from f9fdb65 to 7262620 Compare April 8, 2021 10:47
@markshannon markshannon closed this Apr 8, 2021
@markshannon markshannon reopened this Apr 8, 2021
@markshannon markshannon force-pushed the faster-tracing-test branch from 7262620 to 2cf3c98 Compare April 8, 2021 11:00
@markshannon
Copy link
Member Author

python#25276

@markshannon markshannon closed this Apr 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants