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

"Freeze" data classes and routes + some slotting fixes/refactor #611

Closed

Conversation

FasterSpeeding
Copy link
Collaborator

@FasterSpeeding FasterSpeeding commented May 15, 2021

Summary

Turns out previous assumptions on performance implications of freezing attr data classes were incorrect and the documentation on https://www.attrs.org/en/stable/how-does-it-work.html?highlight=frozen#slotted-classes is misleading. While frozen slotted classes are slower to initiate than frozen dict classes, there isn't any difference between frozen slotted classes and non-frozen slotted classes as far as this goes.

Other features this PR introduces

  • A user ID to DM ID cache store to replace the previous caching of DM channel IDs on user objects which is now not possible with objects being immutable.
  • A switch over to preferring tuples in the entity and event factory over lists to avoid passing around mutable state as much as possible

Removals

  • Removed usage of copy in the cache in most places as this is now unnecessary with objects being immutable.
  • Removed hikari.iternal.attr_extensions as this only existed as an impl detail aid to the usage of copy in the cache.
  • The guild info member_count, joined_at and is_large are no longer persisted between updates in the cache due to the limitation of models being immutable (this could be changed as to update the model by calling object.__setattr__(new_guild, "field_name", old_state) but that'd be unorthodox at best)

Changes this PR doesn't introduce but marks as TODO:

  • Possibly splitting out the Embed class into an immutable received embed object and a mutable embed builder. This means that a lot of the embed objects for now will not be defined as frozen for now.

Barely in scope fixes/changes introduced

  • Some event models were missing slot declarations and usages of attrs.define on abcs with no actual fields has been further removed in places in favour of explicitly declaring an empty init.

Checklist

  • I have run nox and all the pipelines have passed.
  • I have made unittests according to the code I have added/modified/deleted.

Related issues

hikari/applications.py Outdated Show resolved Hide resolved
* Replace some overkill attrs.define calls on pure abcs with explicitly declared empty slots
* Add missing empty slots to some event abcs
…ring mutable state

* There isn't really any good standard way to do this with mappings so for now dicts will still be used
* Since we type as Sequence rather than MutableSequence or List this is esxsentially a free change as far as our public interface is concerned
@FasterSpeeding FasterSpeeding added documentation Improvements or additions to documentation enhancement New feature or request optimization Optimizations to the code and removed documentation Improvements or additions to documentation labels May 16, 2021
@davfsa
Copy link
Member

davfsa commented May 21, 2021

Btw, exceptions shouldnt be frozen, right?

@FasterSpeeding
Copy link
Collaborator Author

Gonna close this for now, since python-attrs/attrs#817 likely brings back the performance difference between default parameter define and default parameter frozen classes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request optimization Optimizations to the code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants