Avoid setting frame->prev_instr = next_instr on some opcodes

[sweeneyde]

Currently, TARGET(NOP): expands to somthing like

    TARGET_NOP:
        frame->prev_instr = next_instr++;
    {
        /* ... */
    }

It would be nice to remove this memory write altogether on some opcodes where it's safe to do so.

To make it explicit where this is happening and where it can be avoided, can we change all of the opcodes to include this info in the instruction body? To include all of the compile-time flags, it might become something like adding these 3 lines to every opcode:

    TARGET(NOP) {
        SET_FRAME_INSTR();
        JUMPBY(1);
        RECORD_STATS();
        /* ... */
    }

Then the SET_FRAME_INSTR() can probably be removed from some opcodes where it's safe: where no tracing can happen and no arbitrary python code can be invoked. Example:

Before:

        TARGET(LOAD_FAST)
            /* JUMPBY and SET_FRAME_INSTR are implicit here */
        {
            PyObject *value = GETLOCAL(oparg);
            if (value == NULL) {
                goto unbound_local_error;
            }
            Py_INCREF(value);
            PUSH(value);
            DISPATCH();
        }

After:

        TARGET(LOAD_FAST) {
            // Don't need to SET_FRAME_INSTR() here.
            JUMPBY(1);
            RECORD_STATS();
            PyObject *value = GETLOCAL(oparg);
            if (value == NULL) {
                goto set_frame_instr_and_unbound_local_error;
            }
            Py_INCREF(value);
            PUSH(value);
            DISPATCH();
        }

IMO it also becomes easier to reason about the various JUMPBY() steps involving cache entries.

[sweeneyde]

It seems that tracemalloc reads the prev_instr during some finalizer, so Py_DECREFs aren't, strictly speaking, safe. One test case is at test.test_warnings.test_tracemalloc, and it verifies that this code:

def func():
    f = open(__file__, "rb")
    # Emit ResourceWarning
    f = None

func()

produces this output:

{filename}:5: ResourceWarning: unclosed file <...>
  f = None
ResourceWarning: Enable tracemalloc to get the object allocation traceback

And this fails if STORE_FAST doesn't set frame->prev_instr.

It doesn't seem that this would be a language guarantee that this would happen in the right place, but it would probably be a nice thing to keep working regardless.

[sweeneyde]

I haven't tried very hard to make a reproducer, but this might be an issue with the LOAD_FAST__STORE_FAST super-instruction as well.

[sweeneyde]

Draft PR is here: python/cpython#92105

[sweeneyde]

Pyperformance: 1.00x faster

Slower (16):
- pickle_dict: 27.1 us +- 0.8 us -> 28.7 us +- 0.6 us: 1.06x slower
- unpickle_list: 4.85 us +- 0.08 us -> 5.06 us +- 0.30 us: 1.04x slower
- nbody: 86.6 ms +- 1.3 ms -> 90.0 ms +- 2.5 ms: 1.04x slower
- pickle_list: 4.47 us +- 0.09 us -> 4.63 us +- 0.11 us: 1.03x slower
- hexiom: 6.22 ms +- 0.05 ms -> 6.40 ms +- 0.10 ms: 1.03x slower
- scimark_lu: 97.3 ms +- 1.3 ms -> 100 ms +- 1 ms: 1.03x slower
- xml_etree_generate: 73.6 ms +- 1.1 ms -> 75.2 ms +- 0.9 ms: 1.02x slower
- xml_etree_process: 51.1 ms +- 0.6 ms -> 52.1 ms +- 0.8 ms: 1.02x slower
- regex_compile: 140 ms +- 2 ms -> 143 ms +- 2 ms: 1.02x slower
- django_template: 37.3 ms +- 1.5 ms -> 37.9 ms +- 1.8 ms: 1.02x slower
- scimark_fft: 330 ms +- 14 ms -> 335 ms +- 11 ms: 1.01x slower
- sqlite_synth: 2.34 us +- 0.05 us -> 2.38 us +- 0.04 us: 1.01x slower
- sqlalchemy_declarative: 144 ms +- 4 ms -> 146 ms +- 4 ms: 1.01x slower
- python_startup: 8.59 ms +- 0.06 ms -> 8.67 ms +- 0.32 ms: 1.01x slower
- xml_etree_iterparse: 105 ms +- 2 ms -> 105 ms +- 2 ms: 1.01x slower
- float: 72.8 ms +- 1.2 ms -> 73.3 ms +- 1.2 ms: 1.01x slower

Faster (14):
- unpack_sequence: 43.8 ns +- 0.5 ns -> 41.2 ns +- 0.7 ns: 1.06x faster
- pickle: 12.3 us +- 1.3 us -> 11.6 us +- 1.3 us: 1.06x faster
- meteor_contest: 110 ms +- 6 ms -> 107 ms +- 2 ms: 1.03x faster
- regex_v8: 21.7 ms +- 0.3 ms -> 21.0 ms +- 0.2 ms: 1.03x faster
- scimark_sor: 108 ms +- 1 ms -> 105 ms +- 1 ms: 1.03x faster
- chameleon: 6.46 ms +- 0.18 ms -> 6.33 ms +- 0.06 ms: 1.02x faster
- raytrace: 302 ms +- 12 ms -> 297 ms +- 11 ms: 1.02x faster
- deltablue: 3.82 ms +- 0.08 ms -> 3.76 ms +- 0.11 ms: 1.02x faster
- regex_effbot: 2.80 ms +- 0.04 ms -> 2.77 ms +- 0.03 ms: 1.01x faster
- logging_format: 7.53 us +- 0.09 us -> 7.43 us +- 0.10 us: 1.01x faster
- go: 140 ms +- 4 ms -> 139 ms +- 2 ms: 1.01x faster
- scimark_monte_carlo: 66.1 ms +- 1.4 ms -> 65.6 ms +- 0.8 ms: 1.01x faster
- pathlib: 19.2 ms +- 0.5 ms -> 19.1 ms +- 0.3 ms: 1.01x faster
- pidigits: 179 ms +- 1 ms -> 178 ms +- 1 ms: 1.00x faster

Benchmark hidden because not significant (29): 2to3, chaos, crypto_pyaes, dulwich_log, fannkuch, html5lib, json_dumps, json_loads, logging_silent, logging_simple, mako, nqueens, pickle_pure_python, pyflate, python_startup_no_site, regex_dna, richards, scimark_sparse_mat_mult, spectral_norm, sqlalchemy_imperative, sympy_expand, sympy_integrate, sympy_sum, sympy_str, telco, tornado_http, unpickle, unpickle_pure_python, xml_etree_parse

Geometric mean: 1.00x faster

[brandtbucher]

It doesn't surprise me that it isn't really any faster. That member is hardly ever read, so issuing a few extra writes (when we're already constantly writing to it) doesn't seem all that expensive to me.

I also find the code that elides these writes for some opcodes quite a bit harder to reason about. Having this member set correctly is super important for correct debugging, exception handling, etc., so I would personally like to see a significant payoff before introducing that additional complexity and opportunity for error.

[markshannon]

IMO, removing writes of frame->prev_instr is best left to a trace optimizer, which can do precise enough escape analysis to know when it can be done safely.