sstelfox
commented
4 years ago We're still in the process of tracking down what about adding the honeycomb client impacted our performance so much, but I'll get back to you when I learn more.
You're right this does change the semantics and does likely introduce a situation where two threads both don't see a client at the same time and one of them is replaced (though the creation of both with will still be thread synchronized), the last one will just win.
If the lock isn't necessary at all though, I would absolutely get rid of it here or at least move the initialization of the client somewhere that happens once so this much hotter path doesn't have to deal with the initialization and lock (which may just be a better solution if there is a better place to handle the initialization).
Locks are relatively expensive and the way it currently is, that lock penalty is being incurred for every call to this method. Moving the check outside the block also prevents the instantiation of unused objects during every call (though the lock performance likely outstrips any impact compared to those).
martin308
robbkidd
While attempting to test honeycomb in our application we experienced a significant throughput performance decrease. While attempting to identify the root cause I noticed the potential for lock contention during this
send_event.Since the lock is only necessary if the client needs to be initialized, this simply moves the short circuit to avoid using the lock when it's not needed.