-
Notifications
You must be signed in to change notification settings - Fork 23
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
Trace aggregator decouple #90
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This reverts commit 793f79e.
shuhaowu
force-pushed
the
trace-aggregator-decouple
branch
from
July 24, 2024 04:27
000f6ee
to
86cf4b6
Compare
Previously we were leaking `ThreadTracer` objects in the `TraceAggregator` as creating new threads means `ThreadTracer` gets pushed into `TraceAggregator` but it is never removed. This causes a memory leak and also makes the `TraceAggregator` slower. This refactors the entire code to make this work. Some of the highlights are: - Removed the feature of dynamically adding in trace `Sink`s. `Sink` can now only be specified when the trace session is started. Since we don't really have any use case of dynamically hooking in sinks, this feature is a lot of complexity for no reason. - This also removed the sticky packet feature within the `TraceAggregator` which saved additional complexity. - Remove the feature of the `TraceAggregator` mirroring data to multiple `Sink`s. This is also unnecessary. Could create a `MultiSink` feature if necessary to emulate this. So now, when you start a trace session, you must give a single sink for the data to be pushed to. - `TraceAggregator` is now a permanent object (as a `shared_ptr`) on the `App` instead of being dynamically created and deleted when the trace session is started and stopped. This skips the need of having App cache the list of `ThreadTracer`s and pass it to the `TraceAggregator` during its construction when the trace session starts. - Instead, the `TraceAggregator` internally has a `SessionData` object (`session_`). This object is recreated and deleted when the trace session starts and stops. - Since the `TraceAggregator` is permanent now, the `Thread` directly register the `ThreadTracer` with the `TraceAggregator` during its start up procedure. This replaces registering through the `App`. The `Thread` are also now holding a `weak_ptr` to the `TraceAggregator`. - When a thread shuts down, the `ThreadTracer` is marked as "done". The `TraceAggregator` will check for this "done" status if no more events are available from the queue. If it is done, the `ThreadTracer` will be removed from the `TraceAggregator`. - `TraceAggregator` no longer supports `RequestStop` and `Join`. This is replaced with a single `Stop` call which will wait for the `TraceAggregator` to fully stop and reset the state of the TraceAggregator (and any registered `ThreadTracer`'s string interner). - Right now, there is a potential data race during the `TraceAggregator.Stop`, as we access `session_` without a lock. This is most likely OK as we don't expect `TraceAggregator.Stop` to be called from multiple threads or rapidly recreated for now. **Should probably fix it in the future.** - String interner states are now reset when a trace session stops. This means if another trace session is started, the strings it remembers are reset. The id starting positions are also reset. - Since we no longer have sticky packets, we also no longer emit a trace event packet that contains interned data from a previous trace session.
shuhaowu
force-pushed
the
trace-aggregator-decouple
branch
from
July 24, 2024 04:28
86cf4b6
to
04969a8
Compare
stephanie-eng
approved these changes
Jul 25, 2024
@@ -1,6 +1,7 @@ | |||
#ifndef CACTUS_TRACING_THREAD_TRACER_H_ | |||
#define CACTUS_TRACING_THREAD_TRACER_H_ | |||
|
|||
#include <atomic> |
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.
This should be with the other includes at line 11
Co-authored-by: Stephanie Eng <[email protected]>
shuhaowu
force-pushed
the
trace-aggregator-decouple
branch
from
July 25, 2024 23:30
2f970e0
to
089cbbb
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Previously we were leaking
ThreadTracer
objects in theTraceAggregator
as creating new threads meansThreadTracer
getspushed into
TraceAggregator
but it is never removed. This causes amemory leak and also makes the
TraceAggregator
slower.This refactors the entire code to make this work. Some of the highlights
are:
Sink
s.Sink
cannow only be specified when the trace session is started. Since we
don't really have any use case of dynamically hooking in sinks, this
feature is a lot of complexity for no reason.
TraceAggregator
which saved additional complexity.TraceAggregator
mirroring data to multipleSink
s. This is also unnecessary. Could create aMultiSink
featureif necessary to emulate this. So now, when you start a trace session,
you must give a single sink for the data to be pushed to.
TraceAggregator
is now a permanent object (as ashared_ptr
) on theApp
instead of being dynamically created and deleted when the tracesession is started and stopped. This skips the need of having App
cache the list of
ThreadTracer
s and pass it to theTraceAggregator
during its construction when the trace session starts.
TraceAggregator
internally has aSessionData
object(
session_
). This object is recreated and deleted when the tracesession starts and stops.
TraceAggregator
is permanent now, theThread
directlyregister the
ThreadTracer
with theTraceAggregator
during itsstart up procedure. This replaces registering through the
App
. TheThread
are also now holding aweak_ptr
to theTraceAggregator
.ThreadTracer
is marked as "done". TheTraceAggregator
will check for this "done" status if no more eventsare available from the queue. If it is done, the
ThreadTracer
willbe removed from the
TraceAggregator
.TraceAggregator
no longer supportsRequestStop
andJoin
. This isreplaced with a single
Stop
call which will wait for theTraceAggregator
to fully stop and reset the state of theTraceAggregator (and any registered
ThreadTracer
's string interner).TraceAggregator.Stop
, as we accesssession_
without a lock. Thisis most likely OK as we don't expect
TraceAggregator.Stop
to becalled from multiple threads or rapidly recreated for now. Should
probably fix it in the future.
means if another trace session is started, the strings it remembers
are reset. The id starting positions are also reset.
trace event packet that contains interned data from a previous trace
session.