-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
base: master
Are you sure you want to change the base?
Conversation
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 |
5471267
to
6868d34
Compare
6755c87
to
51379e2
Compare
Yes, it goes in that direction, however the implementation presented here do not cover the
For sure makes sense, I already updated the PR for using |
I don't think NamedTuple can be a value type, since it supports subclassing and subclasses can add settable attributes. The idea with
I don't think they anything like that, and besides dataclasses have reference semantics, even if they are frozen, and support subclassing. |
makes sense, I didnt saw that NamedTuples can be subclassed and add new mutable members |
@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. |
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. |
d926eb6
to
4fde6f9
Compare
Done |
There was a problem hiding this 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): |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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: |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 []) |
There was a problem hiding this comment.
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] |
There was a problem hiding this comment.
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}'", |
There was a problem hiding this comment.
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.
These features may not have tests (and some are probably not implemented):
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 ( |
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
The add method will produce the code:
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.