All commit authors signed the Contributor License Agreement.

Rather than rewriting uses of these functions, could it be easier to just do something like this, since we're guranteed not to escape?

Seems feasible. I could try to rewrite all occurences of the variable with a stackref-producing const one. Let me try that.

I've verified no refleak ontest_capi.test_opt locally apart from#132731 which is pre-existing.

There's a lot going on in this PR, probably too much for one PR.

Could we start with a PR to fix up thepure annotations so that they are on the correct instructions and maybe add thepure_guard annotation that Brandt suggested?

Could we have the default code generator generate a function for the body of the pure instruction and then call that from the three interpreters?

Could we have the default code generator generate a function for the body of the pure instruction and then call that from the three interpreters?

Hm, I think I’d prefer not to. Sounds like it could hurt performance, especially for the JIT (where things can’t inline).

I think a good progression would be:

• Implement thepure attribute, and the optimizer changes. Remove thepure attributes where they don’t belong (so nothing breaks) and leave the existing ones as proof that the implementation works. (This PR)
• Audit the existing non-pure bytecodes and addpure where it makes sense. (Follow-up PR)
• Implement thepure_guard attribute, and annotate any bytecodes that can use it. (Follow-up PR)

Could we have the default code generator generate a function for the body of the pure instruction and then call that from the three interpreters?

Hm, I think I’d prefer not to. Sounds like it could hurt performance, especially for the JIT (where things can’t inline).

I thought about this and I think we can inline if we autogenerate a header file and include that directly. But then we're at the mercy of the compiler in both the normal interpreter and the JIT deciding to inline or not to inline the bodyagain. Which I truly do not want.

@brandtbucher@markshannon what can I do to get this PR moving?

@tomasr8 if youd like to review, here's a summary of the PR:

1. If a bytecode operation is pure (no side effects) we can mark it as pure in bytecodes.c.
2. In the optimizer, we automatically generate the body that does evaluation of the symbolic constants by copy pasting the bytecodes.c definition into the optimizer's C code. Of course we check that the inputs are constants first.
3. All changes to the cases generator is for the second point.

Thanks for the ping! I actually wanted to try/review this PR, I was just very busy this week with work :/ I'll have a look this weekend :)

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

And if you don't make the requested changes, you will be poked with soft cushions!

We don't want to be evaluating Disaster() +1 when optimizing BINARY_OP_ADD_INT

In the first place, that's not possible. We only optimize what we specialize in the interpreter. The interpreter will never specialize that to BINARY_OP_ADD_INT. Furthermore, even if it did specializeDisaster() to that by some oddity, we check_GUARD_TOS_INT and_GUARD_NOS_INT first in the specializer.Disaster() would never pass these checks.

Unfortunately this approach has a critical flaw. It is possible for the optimizer to see values that the executing code never would. For example, through a combination of statistical branch profiling and global to constant conversion.

If you want to be more assured, how about I merge#132968 to add type assertions to our optimizer? That should make things safer.

We don't want to be evaluating Disaster() +1 when optimizing BINARY_OP_ADD_INT

In the first place, that's not possible. We only optimize what we specialize in the interpreter. The interpreter will never specialize that to BINARY_OP_ADD_INT. Furthermore, even if it did specializeDisaster() to that by some oddity, we check_GUARD_TOS_INT and_GUARD_NOS_INT first in the specializer.Disaster() would never pass these checks.

I forgot the guards don't actually guard at the optimization time. My bad. Yeah it seems we need some sort of check.

@tomasr8 sorry this is going to make your life harder with the removingsym_is_const andsym_matches_type PR. I think you can still removesym_matches_type, butsym_is_const is now a little harder.

I have made the requested changes; please review again

Thanks for making the requested changes!

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

Note: I mark the object as stackref immortal (but not real immortal!) to simplify the reference management. This means we have no refcounting in the optimizer when constant evaluating stuff. Which makes things easier to reason about, as constants during the lifetime of the optimizer are effectively immortal anyways (the optimizer holds a single reference to all constants).

Waiting for#134284 to be merged first, then I can usePyStackRef_FromPyObjectBorrow

This is also making me realize that wereally should make it possible to detect refleaks/memory leaks on JIT builds soon. The problem is that new executors are allocated all over the place, leading to things like#120501.