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

Conversation

@clokep
Copy link
Member

@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
Member 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
Member 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
@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.

3 participants