FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

GH-93911: Fix `LOAD_ATTR_PROPERTY` caches by brandtbucher · Pull Request #96519 · python/cpython · GitHub

/ cpython Public

GH-93911: Fix LOAD_ATTR_PROPERTY caches - #96519

Merged
markshannon merged 2 commits into
python:mainfrom
brandtbucher:fix-load-attr
Sep 6, 2022
Merged

GH-93911: Fix LOAD_ATTR_PROPERTY caches#96519
markshannon merged 2 commits into
python:mainfrom
brandtbucher:fix-load-attr

Conversation

brandtbucher commented Sep 2, 2022
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Member

Stats show that LOAD_ATTR_PROPERTY has a near-100% failure rate. This is because it always sets the cached function version to 1, regardless of the actual value. LOAD_ATTR_GETATTRIBUTE_OVERRIDDEN has a similar bug, but it doesn't manifest itself since the incorrect function version is never used.

This fixes both bugs, which brings LOAD_ATTR hit rates up from ~80% to ~83% when running the pyperformance suite (although LOAD_ATTR_GETATTRIBUTE_OVERRIDDEN doesn't actually appear in the suite at all).

brandtbucher added type-bug An unexpected behavior, bug, or error performance Performance or resource usage labels Sep 2, 2022
brandtbucher self-assigned this Sep 2, 2022

Fidget-Spinner left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Woops. Thank you for the fix. I forgot && in C doesn't operate like and in Python :).

Comment thread Python/specialize.c
uint32_t func_version = function_check_args(descr, 2, LOAD_ATTR) &&
function_get_version(descr, LOAD_ATTR);
if (func_version == 0) {
if (!function_check_args(descr, 2, LOAD_ATTR)) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Should we write the function version into the 16 bit dk_version field like we were discussing the other day?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Maybe, but probably in another PR that checks version for all specialized calls.

markshannon left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Can we write a test for this?
I can't think of one.

Fidget-Spinner commented Sep 5, 2022
edited
Loading

Copy link
Copy Markdown
Member

Can we write a test for this? I can't think of one.

We can write something stable, then if it deopts the test fails:

class A:
    @property
    def foo(s): pass

a = A()

def f():
    a.foo

for _ in range(8):
    f()
# dissassemble f and inspect that LOAD_ATTR_PROPERTY is inside
for _ in range(x):
    f()
    # dissassemble f and inspect that LOAD_ATTR_PROPERTY is still inside in every single iteration, else fail

markshannon merged commit cd0ff9b into python:main Sep 6, 2022

brandtbucher commented Sep 6, 2022
edited
Loading

Copy link
Copy Markdown
Member Author

Can we write a test for this?
I can't think of one.

In general, I think we should probably have better testing that isn't as heavy as the stuff in test_dis.py. The stuff in test_opcache is good for testing regressions in behavior, but not useful for catching things like this.

It shouldn't be too hard to set up a simple test harness that's able to assert that an opcode in co_code maps to another opcode at the same position in _co_code_adaptive after a specific number of calls. That would be capable of testing that simple, specific optimizations and deoptimizations work without lots of ongoing maintenance every time we change the bytecode.

We could even extend it check cache values, so that we're confident that things like exponential backoff keep working as expected.

itamaro added a commit to adphrost/cpython that referenced this pull request Sep 6, 2022
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
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

Successfully merging this pull request may close these issues.

4 participants


Back | FazBrowse Home | New Git URL