Use custom descriptor instead of properties for Struct attributes #165
Conversation
| # type: (typing.Any, typing.Any) -> typing.Any | ||
| if instance is None: | ||
| return self | ||
| value = getattr(instance, "_{}_value".format(self.name)) |
There was a problem hiding this comment.
I wonder if it's worth precalculating _{}_value and storing it in an attribute? This would increase memory use but speed things up.
There was a problem hiding this comment.
Yeah, I wasn't sure about this. I decided to start with more memory saving option, we can always go back to pre-computing if performance will ever be an issue.
There was a problem hiding this comment.
Actually I don't think we need the original name, so I will just replace it with the formatted one.
| class Attribute(object): | ||
| __slots__ = ("name", "default", "nullable", "user_defined") | ||
|
|
||
| def __init__(self, name, nullable=False, user_defined=False): |
There was a problem hiding this comment.
Brainstorming: What about a separate descriptor class for nullable attributes? This would make the __get__ and __set__ methods slightly faster.
Another idea would be to provide the validator as an attribute of Attribute, instead of putting it in the class dictionary. This way we could benefit from __slots__, and we wouldn't need to use getattr to access it.
There was a problem hiding this comment.
What about a separate descriptor class for nullable attributes? This would make the
__get__and__set__methods slightly faster.
I was also thinking about this, but it probably not worth it. Also again, we can always add it later if we will see any performance issues.
Another idea would be to provide the validator as an attribute of
Attribute
This is a good idea, it will probably not save huge memory but looks cleaner anyway.
|
I pushed some changes but this PR isn't updated, it looks like GitHub is broken again... |
|
It looks like closing and re-opening helped. |
A custom descriptor (essentially de-inlining the current logic) allows to save around 30% of memory at a cost of minor performance penalty.