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

Add BinarySerializer function that uses a thread-local shared buffer, and fix errors #921

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

erer1243
Copy link

@erer1243 erer1243 commented Sep 24, 2024

Fixed dubious undefined behavior stemming from unaligned writes to the serialization buffer. Add serializeSharedBuffer which uses a shared 16MB buffer instead of requiring the caller to have one available. This would make c-api code which uses these functions significantly more efficient. If we like these changes, we can also change ZmqClient to use the shared buffer functions. Every ZmqClient currently allocates its own 16MB serialization buffer, so doing this would save a nonnegligible amount of memory.

Also fixed a couple misc errors - uninitialized array in tests/stringutility_ut.cpp made tests fail to compile; ZMQ_NOBLOCK was deprecated 6 years ago and is now called ZMQ_DONTWAIT. I just noticed these while making changes to & testing BinarySerializer.

Related: #915

@saiarcot895
Copy link
Contributor

saiarcot895 commented Sep 25, 2024

Can you elaborate on the undefined behavior that you saw from the unaligned writes?

@erer1243
Copy link
Author

erer1243 commented Sep 25, 2024

Can you elaborate on the undefined behavior that you saw from the unaligned writes?

Lines like these are UB, as they are writing a size_t to char*. These break strict aliasing/provenance and alignment rules. The WARNINGS_NO_CAST_ALIGN macro is telling gcc to not print the warning, but the problem still exists. The solution to both of these problems is memcpy, so I replaced those pointer casts with read/write_unaligned.

WARNINGS_NO_CAST_ALIGN;
auto pkvp_count = (const size_t*)buffer;
WARNINGS_RESET;

Unaligned reads/writes are acceptable on x86 & arm but isn't unheard of to break on embedded architectures. I'm not sure what hardware sonic supports - if we guarantee x86/arm then it's probably not an issue. Also if the optimizer decided to use a simd register (I have no idea if this would ever happen), then it would fault on x86 too. I thought it would be nicer to just sidestep the questions.

@saiarcot895
Copy link
Contributor

SONiC currently only supported amd64, armhf, and arm64, all of which support unaligned memory access (for armhf, ARMv7 and newer support it, which is what I think compile for). Nevertheless, it would be nice to allow SIMD operations by making sure the memory is aligned.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants