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.

Sign up for GitHub

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

Jump to bottom

src: handle missing TracingController everywhere #33815

Closed
wants to merge 1 commit into from
Closed

src: handle missing TracingController everywhere #33815

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 14 additions & 5 deletions src/tracing/trace_event.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,8 +70,7 @@ enum CategoryGroupEnabledFlags {
// const uint8_t*
// TRACE_EVENT_API_GET_CATEGORY_GROUP_ENABLED(const char* category_group)
#define TRACE_EVENT_API_GET_CATEGORY_GROUP_ENABLED \
node::tracing::TraceEventHelper::GetTracingController() \
->GetCategoryGroupEnabled
node::tracing::TraceEventHelper::GetCategoryGroupEnabled

// Get the number of times traces have been recorded. This is used to implement
// the TRACE_EVENT_IS_NEW_TRACE facility.
Expand Down Expand Up @@ -115,9 +114,10 @@ enum CategoryGroupEnabledFlags {
// const uint8_t* category_group_enabled,
// const char* name,
// uint64_t id)
#define TRACE_EVENT_API_UPDATE_TRACE_EVENT_DURATION \
node::tracing::TraceEventHelper::GetTracingController() \
->UpdateTraceEventDuration
#define TRACE_EVENT_API_UPDATE_TRACE_EVENT_DURATION \
if (auto controller = \
node::tracing::TraceEventHelper::GetTracingController()) \
controller->UpdateTraceEventDuration

// Adds a metadata event to the trace log. The |AppendValueAsTraceFormat| method
// on the convertable value will be called at flush time.
Expand Down Expand Up @@ -319,6 +319,13 @@ class NODE_EXTERN TraceEventHelper {

static Agent* GetAgent();
static void SetAgent(Agent* agent);

static inline const uint8_t* GetCategoryGroupEnabled(const char* group) {
v8::TracingController* controller = GetTracingController();
static const uint8_t disabled = 0;
if (UNLIKELY(controller == nullptr)) return &disabled;
return controller->GetCategoryGroupEnabled(group);
}
};

// TraceID encapsulates an ID that can either be an integer or pointer. Pointers
Expand Down Expand Up @@ -467,6 +474,7 @@ static inline uint64_t AddTraceEventImpl(
// DCHECK(num_args, 2);
v8::TracingController* controller =
node::tracing::TraceEventHelper::GetTracingController();
if (controller == nullptr) return 0;
Comment on lines 475 to +477
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

may be move this to the beginning of the function? here and in AddMetadataEventImpl ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gireeshpunathil How would I do that without introducing a potential memory leak?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@addaleax - I am sorry, but not following: we have a nullcheck and return (with no trace action) in the middle of a function, it can be moved to the start of the function, to save some cpu cycles in cases where the tracing controller is NULL?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gireeshpunathil Yes, but the lines between the start of the function and here take ownership of memory, so moving this line would cause a memory leak?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you mean the std::unique_ptr<v8::ConvertableToTraceFormat> objects? cant those be allocated only in the normal path? i.e., if TracingController object is not null?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gireeshpunathil They are not being allocated here, though – this function only takes ownership of them.

return controller->AddTraceEvent(phase, category_group_enabled, name, scope, id,
bind_id, num_args, arg_names, arg_types,
arg_values, arg_convertibles, flags);
Expand All @@ -489,6 +497,7 @@ static V8_INLINE uint64_t AddTraceEventWithTimestampImpl(
// DCHECK_LE(num_args, 2);
v8::TracingController* controller =
node::tracing::TraceEventHelper::GetTracingController();
if (controller == nullptr) return 0;
return controller->AddTraceEventWithTimestamp(
phase, category_group_enabled, name, scope, id, bind_id, num_args,
arg_names, arg_types, arg_values, arg_convertables, flags, timestamp);
Expand Down