-
-
Notifications
You must be signed in to change notification settings - Fork 31.2k
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
GH-93911: Fix LOAD_ATTR_PROPERTY
caches
#96519
Conversation
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 :).
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)) { |
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?
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.
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.
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 |
In general, I think we should probably have better testing that isn't as heavy as the stuff in It shouldn't be too hard to set up a simple test harness that's able to assert that an opcode in We could even extend it check cache values, so that we're confident that things like exponential backoff keep working as expected. |
…TRIBUTE_OVERRIDDEN specialization)
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 thepyperformance
suite (althoughLOAD_ATTR_GETATTRIBUTE_OVERRIDDEN
doesn't actually appear in the suite at all).