GeezerVison

Why is the make_set here in a thread? It just assigns the same frozenset to s each time.

Maybe change make_set so that it creates a different frozenset each time?

I don't understand this comment. The make_set() binds s to a new frozenset object (at least I intend it to).

The s is indeed updated with a new frozenset, but it is a frozenset with the same elements each time. I was thinking about something like

def make_set():
            nonlocal s
            barrier.wait()
            num_elements = NUM_ITEMS
            while not done:
               num_elements = (num_elements  + 17 %) NUM_ITEMS
                s = frozenset(range(num_elements))

But what are we testing here. There is no concurrent mutation of the s, so this test is not specific to the free-threading build I think?

Oh yes, I'm not sure what I was thinking when writing that test. It doesn't test what I intended. I re-wrote it so that a set() is used as the key and it mutated.

Note to self: this is identical to the ensure_shared_on_read from dictobject.c. The SET_DICT_SHARED would correspond to SET_SET_SHARED, which is an odd name so here it is SET_MARK_SHARED.

set_swap_bodies is only called in two locations in this file. For both cases the second argument b is a newly created temporary which is discarded afterwards. So we could replace part (all?) of these branches with asserts. Part of the other work (e.g. copying back to b) might not be needed for the same reason.

That would simplify the code and be slightly faster (not sure how much it actually matters). I'll create a separate issue for that so we can benchmark it.

Note: this path can be taken. set_lookkey_threadsafe is only called in set_contains_entry. The set_contains_entry is only called in _PySet_Contains. _PySet_Contains is called in set___contains___impl and in bytecodes.c _CONTAINS_OP_SET (the latter can have frozenset as input)

What are we checking here and why? For the original code the PyObject_RichCompareBool could have changed the set, and these checks guard against this. However, in the free-threading setting a concurrent thread can change the set so after the table == FT_ATOMIC_LOAD_PTR_ACQUIRE(so->table) && startkey == FT_ATOMIC_LOAD_PTR_ACQUIRE(ep->key), but before the return cmp;. So any calling method cannot assume that when SET_LOOKKEY_FOUND is returned the key is still in the set.

Maybe adding the check help to preserve backwards compatibility in case of a mutating rich compare?

I believe the "why" is to handle PyObject_RichCompareBool() mutating the set, via the restart. I think we want to preserve that, even for the free-threaded build, since some code that running single-threaded might assume set lookups handle mutation that way.