GH-93911: Fix LOAD_ATTR_PROPERTY caches#96519
Conversation
Fidget-Spinner
left a comment
There was a problem hiding this comment.
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.
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.
Maybe, but probably in another PR that checks version for all specialized calls.
markshannon
left a comment
There was a problem hiding this comment.
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_PROPERTYhas 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_OVERRIDDENhas a similar bug, but it doesn't manifest itself since the incorrect function version is never used.This fixes both bugs, which brings
LOAD_ATTRhit rates up from ~80% to ~83% when running thepyperformancesuite (althoughLOAD_ATTR_GETATTRIBUTE_OVERRIDDENdoesn't actually appear in the suite at all).