Add BinarySerializer function that uses a thread-local shared buffer, and fix errors #921
Conversation
|
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. sonic-swss-common/common/binaryserializer.h Lines 56 to 58 in 24979b0 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. |
|
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. |
erer1243
force-pushed
the
improve-serializer
branch
from
29faf00 to
39b2e81
Compare
Implement a C interface to some of libswsscommon in support of sonic-dash-ha. Related: sonic-net/sonic-dash-ha#6 #921 Incoming follow up PR: erer1243#1 --------- Co-authored-by: erer1243 <[email protected]>
| { | ||
| { | ||
| // ZMQ socket is not thread safe: http://api.zeromq.org/2-1:zmq | ||
| std::lock_guard<std::mutex> lock(m_socketMutex); | ||
|
|
||
| // Use none block mode to use all bandwidth: http://api.zeromq.org/2-1%3Azmq-send | ||
| rc = zmq_send(m_socket, sendbuffer.data(), serializedlen, ZMQ_NOBLOCK); | ||
| // Use nonblocking mode to use all bandwidth: http://api.zeromq.org/2-1%3Azmq-send |
Fixed dubious undefined behavior stemming from unaligned writes to the serialization buffer. Add
serializeSharedBufferwhich 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