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

pystats specialization deferred numbers are wrong #113010

Open
mdboom opened this issue Dec 12, 2023 · 3 comments
Open

pystats specialization deferred numbers are wrong #113010

mdboom opened this issue Dec 12, 2023 · 3 comments
Assignees
Labels
performance Performance or resource usage type-bug An unexpected behavior, bug, or error

Comments

@mdboom
Copy link
Contributor

mdboom commented Dec 12, 2023

Bug report

Bug description:

For many of the "deferred" values in the "specialization" section for each bytecode, the number is enormous. For example

Kind Count Ratio
deferred 737,869,762,948,500,785,112 53,482,007,414,484.3%
hit 1,120,068,160 81.2%
miss 138,055,084 10.0%

We need to investigate where this is coming from (either the Python runtime or the summarize_stats.py script) and fix appropriately.

CPython versions tested on:

CPython main branch

Operating systems tested on:

Linux

Linked PRs

@mdboom mdboom added type-bug An unexpected behavior, bug, or error performance Performance or resource usage labels Dec 12, 2023
@mdboom mdboom self-assigned this Dec 12, 2023
@mdboom
Copy link
Contributor Author

mdboom commented Dec 12, 2023

The source of the issue seems to be decrementing the deferred counter more times than it's incremented, thus going negative. (The larger than 64 bit number is created because it's an aggregate of multiple runs, from the summarize_stats.py script which uses variable length integer arithmetic).

The deferred value is decremented inside of DEOPT_IF (inside UPDATE_MISS_STATS), with a comment that it's to counteract an erroneous increment elsewhere, but maybe that no longer happens? @markshannon: Does that make sense to you given recent changes? Maybe we just remove the STAT_DEC?

#ifdef Py_STATS
#define UPDATE_MISS_STATS(INSTNAME)                              \
    do {                                                         \
        STAT_INC(opcode, miss);                                  \
        STAT_INC((INSTNAME), miss);                              \
        /* The counter is always the first cache entry: */       \
        if (ADAPTIVE_COUNTER_IS_ZERO(next_instr->cache)) {       \
            STAT_INC((INSTNAME), deopt);                         \
        }                                                        \
        else {                                                   \
            /* This is about to be (incorrectly) incremented: */ \
            STAT_DEC((INSTNAME), deferred);                      \
        }                                                        \
    } while (0)
#else
#define UPDATE_MISS_STATS(INSTNAME) ((void)0)
#endif

https://github.com/python/cpython/blob/main/Python/ceval_macros.h#L263

@markshannon
Copy link
Member

Yes, it looks like we specifically emit the INSTRUCTION_STATS macro before the label so that it isn't counted twice.

Do we actually need the deferred stats at all? It doesn't look we increment them anywhere.
I think they date from the days when we had a distinct "base" and "deferred" version of instructions.

@mdboom
Copy link
Contributor Author

mdboom commented Dec 12, 2023

We do increment deferred from a number of bytecodes in bytecodes.c, for example: https://github.com/python/cpython/blob/main/Python/bytecodes.c#L328

However, I can't speak to whether they are still needed. It's easy enough to remove the whole field.

I'm also re-running pystats now with the decrement removed so we can see whether the results look plausible.

mdboom added a commit to mdboom/cpython that referenced this issue Dec 12, 2023
gvanrossum pushed a commit that referenced this issue Dec 12, 2023
This fixes a recently introduced bug where the deferred count is being unnecessarily decremented to counteract an increment elsewhere that is no longer happening. This caused the values to flip around to "very large" 64-bit numbers.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Performance or resource usage type-bug An unexpected behavior, bug, or error
Projects
None yet
Development

No branches or pull requests

2 participants