Improve performance of multithreaded operation in short running processes #475
Conversation
pawelchcki
changed the title
…nsmission backoff
pawelchcki
force-pushed
the
bugfix/writer_performance_improvements
branch
from
861d9ce
to
3a6d12c
Compare
lib/ddtrace/writer.rb
Outdated
| start() | ||
| if pid != @pid | ||
| @mutex_after_fork.synchronize do | ||
| if pid != @pid |
There was a problem hiding this comment.
You are comparing pid != @pid twice. Intentional?
There was a problem hiding this comment.
Yes - the first comparison is done outside of the mutex to avoid locking as this check is only True once per the lifetime of the worker.
The second comparison is to make sure that other thread didn't already start the worker in between first check and locking of the mutex.
There was a problem hiding this comment.
If this is the case, I think it might be a good idea to add comments explaining this, at each line which it appears. As it's written, it's a bit unintuitive, so such an explanation would be really helpful.
I would also consider adding a link back to the issue, just so we know where this change originated from (so as to help some future version of ourselves understand this in the correct context.)
There was a problem hiding this comment.
Yeah comment with explanation sounds like a good idea.
BTW @delner this could be done a bit cleaner with AtomicFixnum from concurrent-ruby but we would have to add it as a dependency.
There was a problem hiding this comment.
My feelings are that it isn't worth adding a dependency for making things a bit cleaner, and that the tracing library should be as thin as reasonably possible.
pawelchcki
force-pushed
the
bugfix/writer_performance_improvements
branch
from
f9cb42a
to
85d9300
Compare
Avoid using mutex when Pids are the same, reduces lock contention improving throughput 2-3x in processes using more threads
For short running processes we were capped at the worker timeout which was 1s. With this change the worker is shut down immediately once data is flushed. Increasing throughput of applications like resque.