classification
Title: test_descr fails randomly when executed with -R :
Type: Stage: resolved
Components: Tests Versions: Python 3.10, Python 3.9
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: dino.viehland, methane, pablogsal, vstinner
Priority: normal Keywords: patch

Created on 2021-03-26 23:53 by pablogsal, last changed 2021-04-27 13:47 by pablogsal. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 25032 merged pablogsal, 2021-03-27 01:34
Messages (13)
msg389575 - (view) Author: Pablo Galindo Salgado (pablogsal) * (Python committer) Date: 2021-03-26 23:53
❯ ./python -m test test_descr -m test_slots -R 3:3
0:00:00 load avg: 0.26 Run tests sequentially
0:00:00 load avg: 0.26 [1/1] test_descr
beginning 6 repetitions
123456
test test_descr failed -- Traceback (most recent call last):
  File "/home/pablogsal/github/cpython/Lib/test/test_descr.py", line 1201, in test_slots
    c.abc = 5
AttributeError: 'C' object has no attribute 'abc'

test_descr failed

== Tests result: FAILURE ==

1 test failed:
    test_descr

Total duration: 72 ms
Tests result: FAILURE
msg389576 - (view) Author: Pablo Galindo Salgado (pablogsal) * (Python committer) Date: 2021-03-27 00:02
Bisecting points at:

commit ee48c7d54147ae906776b9f6f96e8920e097d0c4
Author: Dino Viehland <dinoviehland@fb.com>
Date:   Sat Mar 20 12:12:05 2021 -0700

    bpo-43452: Micro-optimizations to PyType_Lookup (GH-24804)

    The common case going through _PyType_Lookup is to have a cache hit. There are some small tweaks that can make this a little cheaper:

    * The name field identity is used for a cache hit and is kept alive by the cache. So there's no need to read the hash code o the name - instead, the address can be used as the hash.

    *  There's no need to check if the name is cachable on the lookup either, it probably is, and if it is, it'll be in the cache.

    *  If we clear the version tag when invalidating a type then we don't actually need to check for a valid version tag bit.
msg389578 - (view) Author: Pablo Galindo Salgado (pablogsal) * (Python committer) Date: 2021-03-27 00:03
Dino, could you take a look?
msg389580 - (view) Author: Dino Viehland (dino.viehland) * (Python committer) Date: 2021-03-27 00:20
Looking!
msg389581 - (view) Author: Pablo Galindo Salgado (pablogsal) * (Python committer) Date: 2021-03-27 00:26
Seems that the problem is removing the check:

    if (MCACHE_CACHEABLE_NAME(name) &&
        _PyType_HasFeature(type, Py_TPFLAGS_VALID_VERSION_TAG)) {
msg389582 - (view) Author: Dino Viehland (dino.viehland) * (Python committer) Date: 2021-03-27 01:10
It's probably worth having an assert that the version tag is valid, that'll make it easier to see what's going wrong in the cache hit case.  We should have the version tag being 0 now when it's not valid.  I may not be able to debug it anymore tonight, but if no will look tomorrow.
msg389583 - (view) Author: Dino Viehland (dino.viehland) * (Python committer) Date: 2021-03-27 01:26
And it looks like we have an entry with a 0 version, but with a valid name:

(type_cache_entry) $3 = {
  version = 0
  name = 0x0000000100ec44f0
  value = NULL
}
msg389584 - (view) Author: Pablo Galindo Salgado (pablogsal) * (Python committer) Date: 2021-03-27 01:34
I think I have it, we still neet to validate that the version tag is correct. Can you take a look at https://github.com/python/cpython/pull/25032 ?
msg389585 - (view) Author: Dino Viehland (dino.viehland) * (Python committer) Date: 2021-03-27 01:38
I think the issue here is that in assign_version_tag there's this code:

    if (type->tp_version_tag == 0) {
        // Wrap-around or just starting Python - clear the whole cache
        type_cache_clear(cache, 1);
        return 1;
    }

the return 1 is incorrect, it should be return 0 as a valid version tag hasn't been assigned.
msg389586 - (view) Author: Pablo Galindo Salgado (pablogsal) * (Python committer) Date: 2021-03-27 01:43
Yup, that makes sense. Let me update the PR
msg389587 - (view) Author: Pablo Galindo Salgado (pablogsal) * (Python committer) Date: 2021-03-27 03:51
New changeset 11b85abbae8aaa8410b19f358abd7b401881bb1c by Pablo Galindo in branch 'master':
bpo-43636: Validate the version tag in _PyType_Lookup (GH-25032)
https://github.com/python/cpython/commit/11b85abbae8aaa8410b19f358abd7b401881bb1c
msg389688 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2021-03-29 12:09
Is _PyType_Lookup() still faster after the fix? The optimization is mentioned at:
https://docs.python.org/dev/whatsnew/3.10.html#optimizations
msg389725 - (view) Author: Dino Viehland (dino.viehland) * (Python committer) Date: 2021-03-29 18:55
@vstinner - The fix doesn't change the behavior of _PyType_Lookup and instead just fixes a previous unrelated bug.  The condition will typically ever be hit once (at startup) as it's very unlikely that versions will wrap, so there should be no performance difference after the fix.
History
Date User Action Args
2021-04-27 13:47:19pablogsalsetstatus: open -> closed
resolution: fixed
stage: resolved
2021-03-29 18:55:55dino.viehlandsetstatus: closed -> open
resolution: fixed -> (no value)
messages: + msg389725

stage: resolved -> (no value)
2021-03-29 12:09:19vstinnersetmessages: + msg389688
2021-03-27 03:51:57pablogsalsetstatus: open -> closed
resolution: fixed
stage: resolved
2021-03-27 03:51:53pablogsalsetmessages: + msg389587
2021-03-27 01:43:06pablogsalsetmessages: + msg389586
2021-03-27 01:38:12dino.viehlandsetmessages: + msg389585
2021-03-27 01:34:56pablogsalsetmessages: + msg389584
stage: patch review -> (no value)
2021-03-27 01:34:15pablogsalsetkeywords: + patch
stage: patch review
pull_requests: + pull_request23780
2021-03-27 01:26:40dino.viehlandsetmessages: + msg389583
2021-03-27 01:10:53dino.viehlandsetmessages: + msg389582
2021-03-27 00:26:25pablogsalsetmessages: + msg389581
2021-03-27 00:20:27dino.viehlandsetmessages: + msg389580
2021-03-27 00:03:29pablogsalsetnosy: + methane
2021-03-27 00:03:10pablogsalsetmessages: + msg389578
2021-03-27 00:02:05pablogsalsetnosy: + dino.viehland
messages: + msg389576
2021-03-26 23:53:55pablogsalcreate