-
Notifications
You must be signed in to change notification settings - Fork 47
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
Updated Serializer to be more efficient #874
Conversation
e037269
to
4775867
Compare
It seems we are experiencing technical difficulties: |
@qstokkink Nice catch! I didn't even notice, considering the check still passed. |
@egbertbouman I noticed a hanging parenthesis and I wondered why Pylint didn't catch it. Turns out, it wasn't running. |
Our sincere apologies for this technical intermission, please rebase onto the latest |
4775867
to
cc8398f
Compare
@qstokkink Thanks, the pylint check is working now! |
As this is hyper-critical functionality, I'll perform an extended check beyond the PR test suite (running through the documentation, periodic network health scripts, etc.) when this PR is done. The final review will take a bit longer than usual. |
5195cd2
to
329c7bd
Compare
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 have finished the first half of my review: please address these two minor points.
I will now move into the second half of this review and run this code through all of our documentation and scripts.
329c7bd
to
9fd2914
Compare
9fd2914
to
bb538e1
Compare
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.
Thank you for your contribution, fixes and your patience. I ran through all of our scripts and documentation and all of it still works as expected.
This PR:
Serializer
to make it more efficientEZPackOverlay
so that there is less string slicing and fewer lists are being createdSerializable
as an argumentpack(_multiple)
/unpack(_multiple)
functionsoptional_format_list
as its use is limited, and it's hurting unpacking performance. I've intentionally put this in a separate commit, so it's easy to undo in case we decide to keep it.With the updated serializer, the time spent (un)serializing typically decreases by 20+%.
For instance, for intro-request/responses the decrease varies between 21-38%. Currently:
With this PR: