bpo-44914: Maintain invariants of type version tags. #27773

markshannon · 2021-08-15T16:01:06Z

Do not reset global version number counter: makes sure that version numbers are unique.
Do not invalidate type versions unnecessarily.

https://bugs.python.org/issue44914

bedevere-bot · 2021-08-15T16:19:15Z

🤖 New build scheduled with the buildbot fleet by @markshannon for commit 68fd56e 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

Fidget-Spinner · 2021-08-15T16:20:27Z

LGTM. If you want tests, I opened another PR at https://github.com/python/cpython/pull/27774/files for your consideration please (thought it also has my alternative fix in there).

Fidget-Spinner · 2021-08-15T16:23:00Z

Objects/typeobject.c

        entry->value = NULL;
    }
-
-    // Mark all version tags as invalid
-    PyType_Modified(&PyBaseObject_Type);


Hmm, I'm confused as to why we don't need this anymore?

Because we don't reuse the version numbers anymore, so the old ones remain valid.

Fidget-Spinner

FWIW: test_descr is now passing on my LOAD_METHOD specialization branch (without requiring any type identity checks :). Woohoo!

bedevere-bot · 2021-08-15T17:50:48Z

🤖 New build scheduled with the buildbot fleet by @markshannon for commit 3850ef2 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

bedevere-bot · 2021-08-15T22:51:15Z

🤖 New build scheduled with the buildbot fleet by @markshannon for commit 68da204 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

vstinner

Can you please add a NEWS entry?

vstinner · 2021-08-16T08:44:29Z

Objects/typeobject.c

@@ -45,7 +45,7 @@ class object "PyObject *" "&PyBaseObject_Type"

 // bpo-42745: next_version_tag remains shared by all interpreters because of static types
 // Used to set PyTypeObject.tp_version_tag
-static unsigned int next_version_tag = 0;
+static unsigned int next_version_tag = 1;


Would it be worth it to change PyTypeObject.tp_version_tag type to uint64_t to reduce the risk of overflow?

2^64 version changes is less likely than 2^32 changes.

PyType_ClearCache is documented to return unsigned int, not uint64_t, so I don't know if we can change that without deprecation. https://docs.python.org/3/c-api/type.html#c.PyType_ClearCache.

64 bit versions would make the type cache twice the size and might well make performance worse.

Classes don't change that much. If it happens that long running programs use 2**32 versions, then it is likely that a small set of classes change often. In that case we could limit the number of version that an individual class can use up.

vstinner · 2021-08-16T08:45:26Z

Objects/typeobject.c

-        // Wrap-around or just starting Python - clear the whole cache
-        type_cache_clear(cache, 1);
+    if (next_version_tag == 0) {
+        /* We have run out of version numbers */


Can you please elaborate the comment to explain what happen on next_version_tag overflow? I understand that old types keep their caches, but types which are are changed are no longer cached.

Random thought which I haven't verified:
Why not call PyType_Modified on base object type and clear cache on overflow, then continue incrementing version tag counter again?

That way even after overflow, new version tags are assigned. Just that every UINT32_TMAX type cache miss event will cause temporarily bad performance. But that seems worth it vs bad performance all the way after we hit UINT32_TMAX.

Can you please elaborate the comment to explain what happen on next_version_tag overflow? I understand that old types keep their caches, but types which are are changed are no longer cached.

It doesn't overflow. Once next_version_tag reaches 0, we stop issuing new versions.

Random thought which I haven't verified:
Why not call PyType_Modified on base object type and clear cache on overflow, then continue incrementing version tag counter again?

That way even after overflow, new version tags are assigned. Just that every UINT32_TMAX type cache miss event will cause temporarily bad performance. But that seems worth it vs bad performance all the way after we hit UINT32_TMAX.

We would also need to find and clear every specialized instruction that uses the version tag. It is possible, but I don't think it is worth it. 2**32 is a large number.

vstinner · 2021-08-16T08:47:01Z

Objects/typeobject.c

+        entry->version = 0;
+        Py_CLEAR(entry->name);
+        entry->value = NULL;
+    }


Why not reusing type_cache_clear() with a parameter to decide how to handle entry->name, to factorize the code?

Because type_cache_clear() does something different. It resets the cache to its starting state.
This code clears it out during finalization.

Any function with a boolean argument that changes it behavior is really two functions.

I've refactored type_cache_clear() to take a PyObject *.

vstinner · 2021-08-16T10:58:51Z

Would it make sense to emit a warning once Python cannot issue new version?

markshannon · 2021-08-16T11:14:33Z

Would it make sense to emit a warning once Python cannot issue new version?

I don't think we should worry about it.
A program that modifies a class, and calls methods on that class, over 4 billion times is pathological.

In the highly unlikely case that it happens in practice, we can reconsider.

vstinner · 2021-08-16T16:44:28Z

Objects/typeobject.c

-        else {
-            Py_CLEAR(entry->name);
-        }
+        Py_XSETREF(entry->name, _Py_XNewRef(value));


Remark: _Py_XNewRef() should not be used, you should use the public Py_XNewRef() instead.

Summary: This is a backport of python/cpython#27773 and python/cpython#27774 from CPython 3.12. They allow us to perform lookups that assign a new valid version tag to a type in callbacks invoked by `PyType_Lookup()`, which I need to do in D42512608 (and probably others in the future). I moved the call to `_PyClassLoader_ClearCache()` from `_PyType_ClearCache()` to `type_cache_clear()` to ensure that it's called by both `_PyType_Fini()` and `PyType_ClearCache()` (the upstream changes made it so `_PyType_Fini()` no longer calls `_PyType_ClearCache()`, instead calling `type_cache_clear()` directly). Original Summaries: bpo-44914: Maintain invariants of type version tags. (GH-27773) * Do not invalidate type versions unnecessarily. ---- bpo-44914: Add tests for some invariants of tp_version_tag (GH-27774) Reviewed By: carljm Differential Revision: D42760412 fbshipit-source-id: 9d5f2a5

markshannon added 2 commits August 15, 2021 16:48

Do not invalidate type versions unnecessarily.

f49a1ce

Remove unused argument.

68fd56e

the-knights-who-say-ni added the CLA signed label Aug 15, 2021

bedevere-bot added the awaiting core review label Aug 15, 2021

markshannon requested a review from Fidget-Spinner August 15, 2021 16:01

markshannon added the skip news label Aug 15, 2021

markshannon added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Aug 15, 2021

bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Aug 15, 2021

Fidget-Spinner reviewed Aug 15, 2021

View reviewed changes

Fidget-Spinner approved these changes Aug 15, 2021

View reviewed changes

Clear references to names in method cache in _PyType_Fini.

3850ef2

markshannon added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Aug 15, 2021

bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Aug 15, 2021

Make sure that type cache is fully cleared in _PyType_Fini.

68da204

markshannon added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Aug 15, 2021

bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Aug 15, 2021

vstinner reviewed Aug 16, 2021

View reviewed changes

markshannon added 2 commits August 16, 2021 11:36

Add NEWS

4e6865a

Refactor code a bit to avoid duplication.

6bbb1cf

markshannon force-pushed the dont-invalidate-type-version-unnecessarily branch from 165ad11 to 6bbb1cf Compare August 16, 2021 10:55

markshannon removed the skip news label Aug 16, 2021

markshannon merged commit 1a511dc into python:main Aug 16, 2021

bedevere-bot removed the awaiting core review label Aug 16, 2021

markshannon mentioned this pull request Aug 16, 2021

bpo-44914: Add tests for some invariants of tp_version_tag #27774

Merged

vstinner reviewed Aug 16, 2021

View reviewed changes

markshannon deleted the dont-invalidate-type-version-unnecessarily branch August 23, 2021 13:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bpo-44914: Maintain invariants of type version tags. #27773

bpo-44914: Maintain invariants of type version tags. #27773

markshannon commented Aug 15, 2021 •

edited

Loading

bedevere-bot commented Aug 15, 2021

Fidget-Spinner commented Aug 15, 2021 •

edited

Loading

Fidget-Spinner Aug 15, 2021

markshannon Aug 15, 2021

Fidget-Spinner left a comment

bedevere-bot commented Aug 15, 2021

bedevere-bot commented Aug 15, 2021

vstinner left a comment

vstinner Aug 16, 2021

Fidget-Spinner Aug 16, 2021 •

edited

Loading

markshannon Aug 16, 2021

vstinner Aug 16, 2021

Fidget-Spinner Aug 16, 2021

markshannon Aug 16, 2021

markshannon Aug 16, 2021

vstinner Aug 16, 2021

markshannon Aug 16, 2021

markshannon Aug 16, 2021

vstinner commented Aug 16, 2021

markshannon commented Aug 16, 2021

vstinner Aug 16, 2021

bpo-44914: Maintain invariants of type version tags. #27773

bpo-44914: Maintain invariants of type version tags. #27773

Conversation

markshannon commented Aug 15, 2021 • edited Loading

bedevere-bot commented Aug 15, 2021

Fidget-Spinner commented Aug 15, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fidget-Spinner left a comment

Choose a reason for hiding this comment

bedevere-bot commented Aug 15, 2021

bedevere-bot commented Aug 15, 2021

vstinner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fidget-Spinner Aug 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vstinner commented Aug 16, 2021

markshannon commented Aug 16, 2021

Choose a reason for hiding this comment

markshannon commented Aug 15, 2021 •

edited

Loading

Fidget-Spinner commented Aug 15, 2021 •

edited

Loading

Fidget-Spinner Aug 16, 2021 •

edited

Loading