classification
Title: LOAD_ATTR cache does not fully replicate PyObject_GetAttr behavior
Type: behavior Stage: resolved
Components: Interpreter Core Versions: Python 3.10
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: Kevin Modzelewski, Yonatan Goldschmidt, pablogsal, pmpp, yselivanov
Priority: normal Keywords: patch

Created on 2020-11-04 21:56 by Kevin Modzelewski, last changed 2020-11-05 09:23 by pablogsal. This issue is now closed.

Files
File name Uploaded Description Edit
test.py Kevin Modzelewski, 2020-11-04 21:56 Test case with unexpected opcache behavior
Pull Requests
URL Status Linked Edit
PR 23157 merged pablogsal, 2020-11-04 23:12
Messages (7)
msg380370 - (view) Author: Kevin Modzelewski (Kevin Modzelewski) Date: 2020-11-04 21:56
The problem is that the descriptor-ness of a type-level attribute is only checked at opcache-set time, not at opcache-hit time.

$ python3.8 test.py
2

$ ./python --version
Python 3.10.0a2+
$ git rev-parse --short HEAD
789359f47c
$ ./python test.py
1
msg380374 - (view) Author: Pablo Galindo Salgado (pablogsal) * (Python committer) Date: 2020-11-04 22:32
Good catch, Kevin! Would you like to submit a PR for fixing this?
msg380375 - (view) Author: Pablo Galindo Salgado (pablogsal) * (Python committer) Date: 2020-11-04 22:37
Given that having attributes that are classes is quite uncommon, I think we can not optimize of the attribute itself is a class instead of checking for descriptors on every hit, hurting the performance gains
msg380376 - (view) Author: Pablo Galindo Salgado (pablogsal) * (Python committer) Date: 2020-11-04 22:49
Yury, any preference here?
msg380378 - (view) Author: Pablo Galindo Salgado (pablogsal) * (Python committer) Date: 2020-11-04 22:53
We could also store the tag of the type object if is a descriptor and compare against that on the cache hit to check that our assumptions are valid. The price here would be an extra pointer on the cache per opcode that may not even be used most of the time.
msg380379 - (view) Author: Pablo Galindo Salgado (pablogsal) * (Python committer) Date: 2020-11-04 23:10
s/the attribute itself is a class/the attribute itself is in the class/
msg380399 - (view) Author: Pablo Galindo Salgado (pablogsal) * (Python committer) Date: 2020-11-05 09:23
New changeset 80449f243b13311d660eab3a751648029bcdd833 by Pablo Galindo in branch 'master':
bpo-42266: Handle monkey-patching descriptors in LOAD_ATTR cache (GH-23157)
https://github.com/python/cpython/commit/80449f243b13311d660eab3a751648029bcdd833
History
Date User Action Args
2020-11-05 09:23:52pablogsalsetstatus: open -> closed
resolution: fixed
stage: patch review -> resolved
2020-11-05 09:23:33pablogsalsetmessages: + msg380399
2020-11-05 02:09:15Yonatan Goldschmidtsetnosy: + Yonatan Goldschmidt
2020-11-04 23:12:20pablogsalsetkeywords: + patch
stage: patch review
pull_requests: + pull_request22068
2020-11-04 23:10:13pablogsalsetmessages: + msg380379
2020-11-04 22:53:07pablogsalsetmessages: + msg380378
2020-11-04 22:49:28pablogsalsetnosy: + yselivanov
messages: + msg380376
2020-11-04 22:37:43pablogsalsetmessages: + msg380375
2020-11-04 22:32:16pablogsalsetmessages: + msg380374
2020-11-04 22:00:24pmppsetnosy: + pmpp
2020-11-04 21:56:10Kevin Modzelewskicreate