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

Remove RETURN_CONST to allow static specialization of RETURN and LOAD_CONST bytecodes #577

Closed
markshannon opened this issue Apr 19, 2023 · 4 comments

Comments

@markshannon
Copy link
Member

With immortal objects and the desire to shrink code objects, we want to add LOAD_COMMON_CONST for loading None, True, etc.

The RETURN bytecodes do largely different things for generators and normal frames. It would make sense to split this into "normal" and "generator" returns.

The existence of RETURN_CONST prevents this, as we would need 4 versions of it to allow specialization of return for generator/normal frames and for normal/"common" const.

@gvanrossum
Copy link
Collaborator

So I think what you are saying is that in Tier 1:

(1) For return in a generator (or coroutine) generate a different opcode, e.g. RETURN_VALUE_FROM_GENERATOR, vs. RETURN_VALUE for return in regular functions (as today).

(2) We'd have two variants for LOAD_CONST: regular LOAD_CONST and LOAD_COMMON_CONST.

(3) RETURN_CONST, which currently is generated for LOAD_CONST + RETURN_VALUE, would have to be replaced by four separate opcodes, e.g. RETURN_CONST, RETURN_COMMON_CONST, RETURN_CONST_FROM_GENERATOR, and RETURN_COMMON_CONST_FROM_GENERATOR.

(4) In addition we'd need instrumented versions (?).

Of course we could choose to only generate the "super" instruction RETURN_CONST for the combination LOAD_CONST + RETURN_VALUE. But then it would be less useful.

And in Tier 2 there would be no difference -- RETURN_CONST is just LOAD_CONST + _POP_FRAME.

So yeah, once we're ready to do this (or part of this, e.g. the LOAD_COMMON_CONST part), we can just get rid of RETURN_CONST.

@markshannon Do you feel that LOAD_COMMON_CONST is worth doing for 3.13? Or is this a 3.14 idea you don't want to forget? Presumably it would become _LOAD_CONST_INLINE_BORROW in Tier 2. The most common case, for loading None, might not actually reduce most code objects' sizes, because functions without docstrings have a None as constant 0 anyway.

Separately, I'm not sure what benefit we'd get from having RETURN_VALUE_FROM_GENERATOR. I don't see anything conditional on generators in _POP_FRAME. But I assume I'm missing something. Could you give me a hint?

@markshannon
Copy link
Member Author

Do you feel that LOAD_COMMON_CONST is worth doing for 3.13?

It might be, but the real benefit of removing RETURN_CONST is that we can split RETURN_VALUE into a generator/coroutine form and a normal function form.

because functions without docstrings have a None as constant 0 anyway.

Is that part of the language spec, or can we change that?

I'm not sure what benefit we'd get from having RETURN_VALUE_FROM_GENERATOR

The code paths for generators and normal functions are quite different. Most of the work in RETURN_VALUE is done by _PyEval_FrameClearAndPop which then calls either clear_thread_frame or clear_gen_frame. Splitting RETURN_VLAUE into two would make for simpler code paths.
RETURN_VALUE_FROM_GENERATOR also needs to use return_offset, whereas RETURN_VALUE does not.

@gvanrossum
Copy link
Collaborator

Is that part of the language spec, or can we change that?

It's not, but changing it might require some refactoring elsewhere (basically, code objects don't have a field to store the docstring). We played with this in the 3.11 era but never dared do it.

The rest is clear, thank you.

@markshannon
Copy link
Member Author

python/cpython#125878

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

No branches or pull requests

2 participants