capi-workgroup / decisions

Discussion and voting on specific issues
8 stars 1 forks source link

Add PyType_Freeze() function #40

Closed vstinner closed 2 months ago

vstinner commented 3 months ago

API: int PyType_Freeze(PyTypeObject *type)

The function is added to the stable ABI (version 3.14).

Feature requested by Cython to support the limited C API: https://github.com/python/cpython/issues/121654#issue-2406237843

Pull request: https://github.com/python/cpython/pull/122457

cc @encukou @da-woods

vstinner commented 3 months ago

Vote to add this API:

encukou commented 2 months ago

For me, this is a step toward breaking PyType_From* down into 3 steps:

This was sketched in markshannon/New-C-API-for-Python#4.

zooba commented 2 months ago

Can we also add a flag to make the original class unusable until it's frozen? It seems like we shouldn't let them escape in a partially-constructed state, and while that's usually something I'd happily leave to users to get right (and just document "don't let it escape or it may break one day"), in other cases we've ensured this with some kind of builder pattern.

I don't think we need an entirely new API for not-yet-frozen types. But perhaps a type slot that indicates "deferred construction" or something like that.

vstinner commented 2 months ago

Can we also add a flag to make the original class unusable until it's frozen?

What do you mean by "unusable"? As @encukou wrote, the pattern is to use regular APIs to prepare the class, like using "setattr". Do you want to use a new special API to prepare a class? It doesn't sound convenient.

da-woods commented 2 months ago

I don't think freezing classes is supposed to be a compulsory part of constructing the class. It just makes a mutable class immutable. But it'd be perfectly fine to let an unfrozen class escape if you intend it to be mutable.

encukou commented 2 months ago

Unlike tuple or str, here immutability is a bit on the instance. We don't want separate “initialization API” for what you can do to mutable types at runtime. If we do want to mark the original class unusable -- which operations would you disable?

I'm not too worried about letting a class escape while it's still mutable. Nearly all the danger with escaped objects is in someone “seeing” a partially-constructed instance, like a tuple with NULLs. Or someone might, say, use a still-to-be-modified string as a dict key, inadvertently caching the wrong hash. For types, AFAIK we're worried about “unauthorized” monkey-patching or instantiating. If someone does that to “escaped” objects, like ones they find in gc.get_objects, I think their warranty is void.

zooba commented 2 months ago

If we do want to mark the original class unusable -- which operations would you disable?

Instantiating it, mainly.

But really, I want to preserve our ability to later make significant changes to the type in PyType_Freeze that would be impossible if we can't assume that we've got the only reference to it. Things like producing a more efficient layout for the instantiated object (which is something people already have implemented, and it relies on assuming that a type is immutable).

We could achieve the same thing by documentation ("do not use the type if you intend to freeze it"). But if we can make it harder to do, then we're less likely to get people complaining when we change it (and if you think this is a hypothetical, go catch up on d.p.o for the latest complaints about changes and come back).

I don't think freezing classes is supposed to be a compulsory part of constructing the class.

I suggested adding a type slot that marks it as "deferred construction". Meaning that if that slot isn't set, PyType_Freeze would complain, and if it is set then instantiation would fail (or whatever other limit we decide should apply - PyType_Freeze would clear it). So it's not compulsory, but it is something that you would have to select at the initial construction time, not something that you could apply to any type at any time during runtime.

encukou commented 2 months ago

Instantiating it, mainly.

But at this level, “instatiating” is vague. One could call tp_new, or create an object with a compatible layout and set its ob_type (not something we want people to do, but quite possible with current API). And lots of things in between.

IMO, if a type supports something like optimized layouts, then that mechanism should check the invariant it needs -- for example, set a bit that says “the class was instantiated while immutable, disable optimization” (or raise a “instantiated a mutable class” error) from the allocator. Or from tp_new if that is when the class needs to be immutable.

zooba commented 2 months ago

IMO, if a type supports something like optimized layouts, then that mechanism should check the invariant it needs

I'm thinking of the nearly-hypothetical future where we do it for all types. At that stage, a limited API that prevents us from doing it broadly becomes a huge amount of dead weight, whereas we can make it easy to opt-in now and avoid that issue in the future.

Another alternative that I'd be happy with is if PyType_Freeze returns the frozen type, which may be the same pointer (incref'd) as the input, but may also be a different one. You get in just as much trouble if you're already using the pre-freeze type, but we're being explicit in our design that it's trouble, as opposed to merely hoping that people are doing everything right.

encukou commented 2 months ago

I'm thinking of the nearly-hypothetical future where we do it for all types. At that stage, a limited API that prevents us from doing it broadly becomes a huge amount of dead weight

We already have fully mutable types, so we can't apply such an optimization to all types. IMO, whenever we add the optimization, we can also add a “deferred construction” flag. (Existing libraries won't be able to use it, but I generally think that it's OK to require some churn if libraries want the latest optimizations.)

Also, PyType_From* isn't going anywhere, and I expect most classes will continue to use it for convenience. If we do manage to turn it into a convenience function for 3 calls to lower-level public API, it can start adding a “deferred construction” flag whenever it sees Py_TPFLAGS_IMMUTABLETYPE.

Another alternative that I'd be happy with is if PyType_Freeze returns the frozen type, which may be the same pointer (incref'd) as the input, but may also be a different one.

IMO, we're very unlikely to design a good API today that we can seamlessly switch to require copying types. Copying types is such a huge can of worms. (To start on the devilish details: do you expect the metaclass tp_new to be called?)

zooba commented 2 months ago

We already have fully mutable types, so we can't apply such an optimization to all types.

It turns out you can, because if you make a good enough guess as to what members a type will have, it's actually incredibly rare for more attributes to be added later. This was all analysed by Guido and others prior when implementing the optimisation (I believe Pinterest use it? Or possibly Instagram). The let down IIRC was the cost of de-optimising, but that's exactly the kind of thing that only takes one creative idea to fix.

On the other hand, the proposal here is to always create the most flexible types, and then tell some of them that they can't be changed. That absolutely relies on the assumption that we can't apply anything else in between (and only rely on a flag to block out some operations later).

All I'm proposing is the API to create the readonly type in a mutable state, and then it should be frozen before use. In the motivating case, they already know they want a readonly type, so it makes no difference there, and we ought to optimise the API for semantic intent (i.e. "this is a readonly type") and not optimisation intent (i.e. "internally this type is structured differently in a way that I neither see nor influence"). If we get the semantics right now, we don't have to worry about them in the future.

encukou commented 2 months ago

Given that Mark recommended this approach, I have doubts about it blocking optimizations; I'd rather let him chime in here.

But before that, a practical question: what, exactly, should a “deferred construction” flag prevent?

zooba commented 2 months ago

But before that, a practical question: what, exactly, should a “deferred construction” flag prevent?

Ideally, INCREF. Obviously that's not practical, but what it implies is that "nobody else should have a reference to this type before construction is complete (a.k.a. frozen)".

The more practical proposal would be to disallow instantiation, which is almost certainly unintended and also the most likely error to be made with a partially constructed type.

Mere references to the type (e.g. as a value in a dict) are unlikely to ever be problematic if the type goes from unfrozen to frozen. But once we've constructed an instance of that type, I expect it may be a problem to later change it from an unfrozen type to a frozen one. e.g. if another thread is already in one of its functions and we later decide that freezing types changes them to a linear layout instead of a dict.

The other side here is asking what does "freezing" a type mean? If it's just to avoid instance dicts, then I'm pretty sure that is trivial already (perhaps not via FromSpec, but easier done with a type slot flag than a new function). If it's to prevent all setattr, then I'd rather avoid the term "freezing" completely so we can save it for actual frozen types.

(namedtuple comes to mind as an example of an optimised frozen type, though that starts as a tuple and then adds names rather than the proposal here that starts with names and then aims to add restrictions. Adding __slots__ is another similar optimisation, but again, the slots are known at construction and applied before the type is usable in any form.)

vstinner commented 2 months ago

Let's say that a special type can have an optimized layout when it's frozen. I understand the desire to disallow instanciation before the type is frozen, it would be bad to have objects created before freeze with a layout A, and objects created created after freeze with layout B :-(

IMO such special type should be directly created as frozen, with Py_TPFLAGS_IMMUTABLETYPE, not using PyType_Freeze(). If the flag is set since the creation, objects with layout A cannot be created: optimized layout B is always used.

If tomorrow, we want to support such special type with PyType_Freeze() (defer the freeze step), I would suggest to wait until someone comes with a concrete implementation, so we can discuss about details.

Disallowing instanciation is complex since PyTypeObject and typeobject.c are complex. I would also prefer to not touch type_call() (not add a test) to avoid any performance degradation. Again, at least not touch it before someone comes with a more concrete implementation.

If tomorrow, someone comes with such "optimized layout" implementation, IMO Steve's design to "disallow instanciation until PyType_Freeze()" using a flag is a good idea. In the meanwhile, I don't think that it's needed.

encukou commented 2 months ago

The more practical proposal would be to disallow instantiation

But again, what is instantiation? tp_new can be easily bypassed.

Perhaps the choke point is tp_alloc? Would the concrete proposal be to add a flag that:

(since type flags are running out, this could also be done by setting tp_alloc to a new function that always raises, which PyType_Freeze would special-case and replace?)

Then again, for this optimization, it seems we could also add a “has been instantianed yet” flag. tp_alloc (or wherever the “instantiation” check should be) could then:

The other side here is asking what does "freezing" a type mean?

If it's just to avoid instance dicts, then I'm pretty sure that is trivial already

Hm, instance dicts aren't really related to Py_TPFLAGS_IMMUTABLETYPE. This is about freezing the type object, not the instances.

zooba commented 2 months ago

But again, what is instantiation? tp_new can be easily bypassed.

The purpose is to explicitly discourage it enough that we can reserve the right to change it in the future. Historically, even though we've documented things as "don't use it like this", we've still respected code that misused it and avoided making changes that would break it.

I want enough of that kind of code to be broken up front, so that when it's time to change it, we aren't causing (many) new breaks.

this could also be done by setting tp_alloc to a new function that always raises, which PyType_Freeze would special-case and replace?

This works for me. I'm more concerned about the intent than the actual mechanism. I don't want to reach a point where we can't change this because Cython or whoever took a dependency on the fact that we didn't actively prevent it.

vstinner commented 2 months ago

@zooba: We already have a flag to disallow instanciation, it's: Py_TPFLAGS_DISALLOW_INSTANTIATION. One option would be to require a type to be declared with Py_TPFLAGS_DISALLOW_INSTANTIATION (in its spec) in PyType_Freeze() and clear that flag in PyType_Freeze(). So you cannot create instances before PyType_Freeze() and you can after PyType_Freeze().

I'm not sure if we want to entangle Py_TPFLAGS_IMMUTABLETYPE with Py_TPFLAGS_DISALLOW_INSTANTIATION :-(

For example, what if you still want to disallow instanciation after PyType_Freeze()? Do we need a second function (!) to set again the cleared flag Py_TPFLAGS_DISALLOW_INSTANTIATION? In the limited C API, the PyTypeObject.tp_flags member is private and cannot be modified directly. (That's why I'm proposing an API to set the Py_TPFLAGS_IMMUTABLETYPE flag).


For me, the simplest approach is to treat C extensions developers as responsible developers knowing what they do, and so not add any requirement on PyType_Freeze(). Obviously, we can highlight the issue in the documentation.

Developers are also creative to find new ways to use an API that we didn't expect to make cool things sometimes ;-)

vstinner commented 2 months ago

@serhiy-storchaka: You didn't vote, do you have an opinion on this API?

zooba commented 2 months ago

For me, the simplest approach is to treat C extensions developers as responsible developers knowing what they do, and so not add any requirement on PyType_Freeze(). Obviously, we can highlight the issue in the documentation.

This is also fine with me, provided that when we change it later and Cython (or whoever) breaks, we are willing to point at the docs and tell them "too bad" rather than "okay, we'll avoid improving anything for 10 years until you catch up"

vstinner commented 2 months ago

This is also fine with me, provided that when we change it later and Cython (or whoever) breaks, we are willing to point at the docs and tell them "too bad" rather than "okay, we'll avoid improving anything for 10 years until you catch up"

Does it mean that you're fine with the current API? You didn't vote +1 on the API.

zooba commented 2 months ago

I wanted a written acknowledgement from the API supporters that they're willing to stand up to 3rd party projects in the future rather than backing down when we need to make changes. Historically, we haven't done that.

Right now, the PR doesn't include anything in the docs about how to safely create a frozen type, and whether it's safe to create the type, use it, and freeze it later. So based on the current PR, I'm still opposed.

encukou commented 2 months ago

In the future, using this API could prevent optimizations. In that case:

In other words, the change we make in the future needs to be backwards-compatible. Maybe that means the current Py_TPFLAGS_IMMUTABLETYPE flag will need to stay and a separate optimized immutable flag is added (and perhaps enabled automatically if Py_TPFLAGS_IMMUTABLETYPE is specified in single-shot type creation).

But, IMO, we don't see enough into the future to add the new flag now.

zooba commented 2 months ago

Then why do we have immutable types in the first place? Why aren't they all just mutable? (i.e. new_type.__dict__ stops returning a read-only proxy)

zooba commented 2 months ago

Or here's another idea - can we add a typeslot for a function that adds to the initial __dict__? If it's present, we call it during creation and pass it the type's dict directly, so it can add whatever it likes before creation is complete. It can even get a reference to the type itself at that stage, which didn't seem to be needed by Cython, but I'm sure will be needed by someone eventually.

vstinner commented 2 months ago

@encukou:

But, IMO, we don't see enough into the future to add the new flag now.

The address space (unsigned int) for type flags is saturated, so I would prefer to not waste a bit there, until there is a clear use case for it. So far, the "optimizations" discussed here are only hypothetical.

@zooba:

Then why do we have immutable types in the first place?

Py_TPFLAGS_IMMUTABLETYPE was added to be able to convert static types to heap types without changing their API. Cython use case for PyType_Freeze() is similar: https://github.com/python/cpython/issues/121654#issue-2406237843 It's just that Cython cannot use Py_TPFLAGS_IMMUTABLETYPE since they need to make a few changes before making a type immutable.

@zooba:

Right now, the PR doesn't include anything in the docs about how to safely create a frozen type, and whether it's safe to create the type, use it, and freeze it later. So based on the current PR, I'm still opposed.

I updated my PR to add:

The type must not be used before it's made immutable. For example, type instances must not be created before the type is made immutable.

da-woods commented 2 months ago

If this is proving sufficiently contentious - I did manage work out how to get access to the underlying mutable dict object of an immutable class using the limited API (essentially via a Python attribute lookup for type.__dictoffset__, convert to a C int, and apply that to the type pointer). That's not perfect but better than what we were doing when I originally requested the feature.

So Cython doesn't strictly need this. I do still think this feature would be an improvement though. But we can probably get away without it.

Although I guess all the arguments about future optimisations breaking things still apply to our current hack.

zooba commented 2 months ago

essentially via a Python attribute lookup for type.__dictoffset__, convert to a C int, and apply that to the type pointer

Yeah, I'd rather you didn't have to do this either :) Obviously we need something (or Cython needs to decide that cdef types are no longer immutable and so are more consistent with Python types than with static types).

So far, the "optimizations" discussed here are only hypothetical.

Yes, because my concern is that you're suggesting an API that will impede any optimisation in this area. I'm sorry you can't see that directly, and I'm sorry you can't see it with hypothetical examples. This argument is not worth my time to implement an entirely new optimisation just to prevent you from making a mistake now, so go ahead. I abstain from any vote on this.

encukou commented 2 months ago

can we add a typeslot for a function that adds to the initial dict? [...] It can even get a reference to the type itself at that stage

So, pass the partially initialized type to user code, presumably with some limitations on what can be done with the type? The first such callback might be fine, but when we add another one, we run into issues about what order they should be called in (and so, whether one an expect if another one already ran). Compared to allowing user code between initial creation and a _Freeze(), a callback sounds like a complication.

my concern is that you're suggesting an API that will impede any optimisation in this area.

I think I can see the kind of optimization this will impede, but, weighing the chance that we'll get that optimization against the extra code needed for that optimization to work with PyType_Freeze(), and taking into consideration that this was [proposed by @markshannon] (an expert on internal PyObject memory layout), I think PyType_Freeze() is OK. I'd definitely like to get Mark's thoughts on this conversation though.

zooba commented 2 months ago

The first such callback might be fine, but when we add another one, we run into issues about what order they should be called in (and so, whether one an expect if another one already ran).

Huh? The callback is during PyType_FromSpec, which has a regular dict object that becomes the type object's members. It gets filled up from Py_tp_members and Py_tp_getset, etc., but has no way to add PyObject* items to it. After FromSpec is complete, the callback is gone and never called again. (But providing an array of name-object pairs in the same way that Py_tp_members has an array of name-offsets or Py_tp_getset has an array of name-C functions is probably easier.)

serhiy-storchaka commented 2 months ago

Sorry for the delay. I postponed my decision while the discussion was going on.

I think we shouldn't worry about that now. We will think about it when (if) the time comes. And the future problem may be different from what we hypothesize now.

vstinner commented 2 months ago

Since @zooba abstain from voting, this feature is approved by the C API Working Group. Thanks everyone for reviewing this API. I close the issue.

@zooba asked to design an API which prevents creating instances of the type between its creation and when the type is made immutable. For example, a type may use a different and more efficient memory layout once it becomes immutable. Different options were discussed but we didn't agree on a solution. We agreed that we should think about this issue but not implement a solution now. We can revisit the design once someone will step in and actually work on such optimization.

vstinner commented 2 months ago

I didn't understand @zooba's callback idea, so I discussed with him in private. From what I understood, the idea would be to add a callback somehow into spec of PyType_FromSpec(spec) which would be called once the type is created but before PyType_FromSpec() returns.

Pseudo-code, instead of:

int create_callback(PyObject *type)
{
   // ... customize the type ...
   // return -1 on error
   return 0;
}

PyObject* create_type(void)
{
    PyObject *type = PyType_FromSpec(spec);
    if (type == NULL) {
        return NULL;
    }

    if (create_callback(type) < 0)
    {
        Py_DECREF(type);
        return NULL;
    }

    return type;
}

We would have:

PyType_Spec myspec = {
   ...
   {Py_create_callback, callback},
   ...
}

PyObject* create_type(void)
{
    return PyType_FromSpec(spec);
}

And to freeze the type, we can add another slot, such as (still pseudo-code):

PyType_Spec myspec = {
   ...
   {Py_create_callback, callback},
   {Py_freeze, 1},
   ...
}

The advantage would be to make PyType_Spec() "atomic" and not leak a mutable type until PyType_Freeze() is called.

vstinner commented 2 months ago

From what I understood, the idea would be to add a callback somehow into spec of PyType_FromSpec(spec) which would be called once the type is created but before PyType_FromSpec() returns.

I implemented the idea to be able to compare it with PyType_Freeze(): https://github.com/python/cpython/pull/124789