GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — GH-93911: Fix `LOAD_ATTR_PROPERTY` caches by brandtbucher · Pull Request #96519 · python/cpython

Conversation

@brandtbucher

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

Uh oh!

There was an error while loading. Please reload this page.

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).

  • Issue: More LOAD_ATTR specialisations #93911

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
brandtbucher added 2 commits September 2, 2022 16:34
@brandtbucher
Fix incorrect version calculations for LOAD_ATTR
1b555d2
@brandtbucher
blurb add
b7e7324
@brandtbucher brandtbucher added type-bug An unexpected behavior, bug, or error performance Performance or resource usage labels Sep 2, 2022
@brandtbucher
brandtbucher requested review from Fidget-Spinner and markshannon September 2, 2022 23:55
@brandtbucher brandtbucher self-assigned this Sep 2, 2022
@bedevere-bot bedevere-bot added the awaiting core review label Sep 2, 2022
Fidget-Spinner
Fidget-Spinner reviewed Sep 3, 2022
View reviewed changes

@Fidget-Spinner Fidget-Spinner left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
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)) {

@Fidget-Spinner Fidget-Spinner Sep 3, 2022

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@markshannon markshannon Sep 5, 2022

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
markshannon
markshannon approved these changes Sep 5, 2022
View reviewed changes

@markshannon markshannon left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting core review labels Sep 5, 2022
@Fidget-Spinner

Fidget-Spinner commented Sep 5, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Fidget-Spinner
Fidget-Spinner approved these changes Sep 5, 2022
View reviewed changes
@markshannon
markshannon merged commit cd0ff9b into python:main Sep 6, 2022
@bedevere-bot bedevere-bot removed the awaiting merge label Sep 6, 2022
@brandtbucher

brandtbucher commented Sep 6, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

itamaro added a commit to adphrost/cpython that referenced this pull request Sep 6, 2022
@itamaro
fix race with pythonGH-96519 (removed func_version in LOAD_ATTR_GETAT…
a5e9d13
…TRIBUTE_OVERRIDDEN specialization)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@markshannon markshannon markshannon approved these changes
@Fidget-Spinner Fidget-Spinner Fidget-Spinner approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@brandtbucher brandtbucher

Labels

performance Performance or resource usage type-bug An unexpected behavior, bug, or error

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@brandtbucher @Fidget-Spinner @markshannon @bedevere-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.