Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Add __slots__ to replication commands. #16429

Merged
merged 3 commits into from Oct 5, 2023
Merged

Add __slots__ to replication commands. #16429

merged 3 commits into from Oct 5, 2023

Conversation

clokep
Copy link
Contributor

@clokep clokep commented Oct 4, 2023

I'm hoping this will slim down our replication needs very very slightly, but it is also minimal effort.

WIth this change we have the following, sizes are done using _get_size_of.

Class Original size (bytes) Final size (bytes) Savings (bytes)
Any _SimpleCommand subclass 408 360 48
RdataCommand 592 384 208
PositionCommand 672 448 224
UserSyncCommand 728 456 272
ClearUserSyncsCommand 416 360 56
FederationAckCommand 504 400 104
UserIpCommand 752 432 320
LockReleasedCommand 544 376 168
Code to produce the above
from synapse.util.caches.lrucache import _get_size_of
from synapse.replication.tcp.commands import *

def do(inst):
    print(f"{inst.__class__.__name__}: {_get_size_of(inst)}")


do(ServerCommand(""))
do(RdataCommand("", "", None, ()))
do(PositionCommand("", "", 0, 1))
do(UserSyncCommand("", "", None, True, 1))
do(ClearUserSyncsCommand(""))
do(FederationAckCommand("", 1))
do(UserIpCommand("", "", 1, "", None, 1))
do(LockReleasedCommand("", "", ""))

Copy link
Contributor

@DMRobertson DMRobertson left a comment

Choose a reason for hiding this comment

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

Seems reasonable. I guess this only helps a lot if we have a huge queue of replication commands to work through?

(Aside: I was slightly surprised these classes aren't defined with attrs or stdlib dataclasses, which would make it easier to turn on __slots__.)

Copy link
Contributor

@DMRobertson DMRobertson left a comment

Choose a reason for hiding this comment

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

And I even went through and sanity checked that the slots matched the __init__ signatures after I felt guilty for doing a speedy review.

@clokep
Copy link
Contributor Author

clokep commented Oct 4, 2023

(Aside: I was slightly surprised these classes aren't defined with attrs or stdlib dataclasses, which would make it easier to turn on __slots__.)

Due to the NAME class var I was having trouble getting that to work and decided it wasn't worth my effort figuring out how to make those compatible.

@clokep
Copy link
Contributor Author

clokep commented Oct 4, 2023

Seems reasonable. I guess this only helps a lot if we have a huge queue of replication commands to work through?

It should help just in general with allocating less memory.

@clokep clokep marked this pull request as ready for review October 4, 2023 20:12
@clokep clokep requested a review from a team as a code owner October 4, 2023 20:12
@clokep clokep removed the request for review from a team October 4, 2023 20:12
@clokep clokep merged commit 4e302b3 into develop Oct 5, 2023
38 checks passed
@clokep clokep deleted the clokep/slots branch October 5, 2023 11:38
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants