Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[mypyc] Promotion to Value Type optimization #17932

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

jairov4
Copy link
Contributor

@jairov4 jairov4 commented Oct 13, 2024

This PR introduce to mypyc the ability to generate efficient classes passed by value
unleashing deeper optimizations in the C generated code.
With this change mypyc will be finding immutable classes to be promoted as Value Types.
Value types are unboxed representation of classes that can be passed by value avoiding
dynamic memory allocations and replacing those with stack based values.

Should be reviewed and rebased after #17934

For example:

For this class

@final
@mypyc_attr(value_type=True)
class Vector2:
    def __init__(self, x: i32, y: i32) -> None:
        self.x: Final = x
        self.y: Final = y

    def __add__(self, other: "Vector2") -> "Vector2"
        return Vector2(self.x + other.x, self.y + other.y)

The add method will produce the code:

Vector2Object CPyDef_Vector2_____add__(Vector2Object cpy_r_self, Vector2Object cpy_r_other) {
    int32_t cpy_r_r0;
    int32_t cpy_r_r1;
    int32_t cpy_r_r2;
    int32_t cpy_r_r3;
    int32_t cpy_r_r4;
    int32_t cpy_r_r5;
    Vector2Object cpy_r_r6;
    Vector2Object cpy_r_r7;
    cpy_r_r0 = cpy_r_self._x;
    cpy_r_r1 = cpy_r_other._x;
    cpy_r_r2 = cpy_r_r0 + cpy_r_r1;
    cpy_r_r3 = cpy_r_self._y;
    cpy_r_r4 = cpy_r_other._y;
    cpy_r_r5 = cpy_r_r3 + cpy_r_r4;
    cpy_r_r6 = CPyDef_Vector2(cpy_r_r2, cpy_r_r5);
    if (unlikely(cpy_r_r6.vtable == NULL)) {
        CPy_AddTraceback("engine/vectors2.py", "__add__", 12, CPyStatic_globals);
        goto CPyL2;
    }
    return cpy_r_r6;
CPyL2: ;
    cpy_r_r7 = ((Vector2Object){0});
    return cpy_r_r7;
}

Note the values passed as structs directly and returning it directly too.
It has an impact similar to the RTuple but should work transparently with more flexible definitions due the class syntax.
It works similar to Value Types in C# (CLR) or structures in Swift.
It will allow the language be a more suitable option for performance centric tasks like video game engines or real time simulation computation.

Implementation details

It uses the vtable field to signal if any exception happen similar to the empty flag in RTuple.
A new RType called RInstanceValue is created to represent value types.
All semantics of boxed and unboxed values are preserved where (RInstanceValue is unboxed) and RInstance continues being (boxed).
In order to easily box values using the existing infrastructure, the type's setup method has been used and promoted to exported function.

@JukkaL
Copy link
Collaborator

JukkaL commented Oct 15, 2024

Value types are a great feature, thanks! This seems to address mypyc/mypyc#841 -- what do you think?

Since using a value type can change semantics (at least is / object identity), and sometimes a heap type is preferred even if all attributes are final (e.g. there are dozens of attributes -> copying values would be slow), it may be better to require value classes to be annotated explicitly. We could start with @mypyc_attr(<something>=True) and come up with a better decorator name in mypy_extensions. See the referred issue for more about this.

@jairov4
Copy link
Contributor Author

jairov4 commented Oct 17, 2024

@JukkaL

Value types are a great feature, thanks! This seems to address mypyc/mypyc#841 -- what do you think?

Yes, it goes in that direction, however the implementation presented here do not cover the @record for your use case.
Additionally, I think that functionality is already provided by python using NamedTuple, where a NamedTuple._replace() method (maybe ugly named) can be used for modifying the object keeping the immutability (thats the way I use today to avoid extra deps in some contexts). Not sure if frozen dataclasses have something similar. We could adapt it to namedtuple as next step to avoid additional syntatic sugar

Since using a value type can change semantics (at least is / object identity), and sometimes a heap type is preferred even if all attributes are final (e.g. there are dozens of attributes -> copying values would be slow), it may be better to require value classes to be annotated explicitly. We could start with @mypyc_attr(<something>=True) and come up with a better decorator name in mypy_extensions. See the referred issue for more about this.

For sure makes sense, I already updated the PR for using @mypyc_attr(value_type=True)

@JukkaL
Copy link
Collaborator

JukkaL commented Oct 17, 2024

Additionally, I think that functionality is already provided by python using NamedTuple

I don't think NamedTuple can be a value type, since it supports subclassing and subclasses can add settable attributes. The idea with @record (or whatever syntax we'd use for this) is that it's both a value type and makes it easy to modify values in-place.

Not sure if frozen dataclasses have something similar.

I don't think they anything like that, and besides dataclasses have reference semantics, even if they are frozen, and support subclassing.

@jairov4
Copy link
Contributor Author

jairov4 commented Oct 17, 2024

makes sense, I didnt saw that NamedTuples can be subclassed and add new mutable members

@jairov4
Copy link
Contributor Author

jairov4 commented Oct 18, 2024

@JukkaL can you review at this point. I think is ready due the tests I have made, let me know if you consider any other test.

@JukkaL
Copy link
Collaborator

JukkaL commented Oct 18, 2024

Can you merge master? It now includes the dunder changes. This is a big PR so it will take some time to review and test. I'll start looking into this early next week probably.

@jairov4
Copy link
Contributor Author

jairov4 commented Oct 19, 2024

Done

Copy link
Collaborator

@JukkaL JukkaL left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did a quick review pass, focusing on the semantics of value types (didn't look at implementation details carefully yet).


@final
@mypyc_attr(value_type=True)
class B(A):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if it makes sense for value types to subclass non-value types (other than the implicit object). This could break LSP in some use cases that assume that object identity is preserved across the lifetime of an object. For example, assume that some extra information is stored in some dict based on object identity. Now if using a value type subclass, that extra information can be lost when the value is passed around.

Also, calling inherited methods seems tricky, and currently seems to generate compile errors:

from typing import final, Final
from mypy_extensions import mypyc_attr

class Bar:
    def __init__(self) -> None:
        self.xx: Final = 0

    def method(self) -> int:
        return self.xx


@final
@mypyc_attr(value_type=True)
class Foo(Bar):
    def __init__(self) -> None:
        self.yy: Final = 0


def foo(f: Foo) -> int:
    return f.method()  # This seems to generate bad C (didn't investigate in detail)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right, I will change this to support only inheritance on other value types only.


@final
@mypyc_attr(value_type=True)
class B:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test that we generate an error if @final is omitted but a class is declared as a value type.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Due the previous comment, I will relax this constraint, so value types can be non final classes. But they still need to be immutable.

"run-integers.test",
"run-lists.test",
"run-traits.test",
] + (["run-match.test"] if sys.version_info >= (3, 10) else [])
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will probably slow down the test suite a lot, since we'll run many tests separately. We need a way to test this without duplicating many tests.

@@ -0,0 +1,336 @@
[case testValueTypeBasic]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you also add some irbuild tests for basic functionality to ensure that we generate efficient IR?

if mtd is not None:
module = module_by_fullname.get(mtd.info.module_name)
errors.error(
f"Value types must not define method '{mtd_name}'",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add test for this error.

@JukkaL JukkaL changed the title [mpypc] Promotion to Value Type optimization [mypyc] Promotion to Value Type optimization Oct 22, 2024
@JukkaL
Copy link
Collaborator

JukkaL commented Oct 22, 2024

These features may not have tests (and some are probably not implemented):

  • Fixed-length tuple items with value types
  • Undefined attribute/local variable with value type (e.g. conditionally assign value)
  • Module-level attribute with value type (may involve boxing, unfortunately)
  • Final module-level attribute with value type (shouldn't involve boxing)
  • Default argument value with value type
  • Reference counting when value type attribute uses reference counting (similar to tuples)

If some of these are unclear, I can help. Also, implementing all of these in a single PR may make the PR too big for comfortable reviewing. It's okay to implement these in multiple PRs, if the functionality is behind a feature flag until complete. We have way of hiding features behind feature flags (--enable-incomplete-feature, see INCOMPLETE_FEATURES in mypy.options).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants