src: handle missing TracingController everywhere#33815
src: handle missing TracingController everywhere#33815addaleax wants to merge 1 commit intonodejs:masterfrom
Conversation
| v8::TracingController* controller = | ||
| node::tracing::TraceEventHelper::GetTracingController(); | ||
| if (controller == nullptr) return 0; |
There was a problem hiding this comment.
may be move this to the beginning of the function? here and in AddMetadataEventImpl ?
There was a problem hiding this comment.
@gireeshpunathil How would I do that without introducing a potential memory leak?
There was a problem hiding this comment.
@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?
There was a problem hiding this comment.
@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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
@gireeshpunathil They are not being allocated here, though – this function only takes ownership of them.
|
CI: https://ci.nodejs.org/job/node-test-pull-request/31833/ (:yellow_heart:) |
|
Landed in dfdbbd1 |
Fixes: #33800
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes