Conversation
There was a problem hiding this comment.
Pull request overview
This PR improves profiler performance by migrating trace processing from JavaScript to C++, reducing string copying and improving memory stability for long-running traces.
Changes:
- Refactored the tracing agent to store traces in C++ instead of processing them in JavaScript
- Updated inspector interfaces to use const references for better performance
- Added JSON library (nlohmann/json) to third_party for JSON parsing in C++
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| v8ios.xcodeproj/project.pbxproj | Added third_party directory to project structure |
| NativeScript/inspector/ns-v8-tracing-agent-impl.mm | Refactored trace writer to use chunked storage and removed JavaScript trace processing |
| NativeScript/inspector/ns-v8-tracing-agent-impl.h | Updated interfaces to return vector of strings and added chunking methods |
| NativeScript/inspector/JsV8InspectorClient.mm | Added JSON parsing for trace configuration and removed JavaScript-based trace processing |
| NativeScript/inspector/JsV8InspectorClient.h | Updated method signatures to use const references |
| NativeScript/inspector/InspectorServer.mm | Updated method signatures to use const references |
| NativeScript/inspector/InspectorServer.h | Updated method signatures to use const references |
| .clang-format-ignore | Added third_party directory to format ignore list |
Comments suppressed due to low confidence (1)
NativeScript/inspector/JsV8InspectorClient.mm:1
- Lambda captures
messageby value, creating an unnecessary string copy. SinceSendtakes aconst std::string&, the lambda parameter should also beconst std::string&:[channel, q](const std::string& message).
#include <Foundation/Foundation.h>
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| int traces_per_chunk_ = 20; | ||
|
|
||
| void NSInMemoryTraceWriter::AppendTraceEvent(TraceObject* trace_event) { | ||
| MaybeCreateChunk(); | ||
|
|
||
| json_trace_writer_->AppendTraceEvent(trace_event); | ||
| total_traces_++; | ||
| if (total_traces_ > 0 && (total_traces_ % traces_per_chunk_ == 0)) { |
There was a problem hiding this comment.
Global variable traces_per_chunk_ uses a trailing underscore, which is typically reserved for member variables in C++. Consider renaming to kTracesPerChunk or TRACES_PER_CHUNK to follow naming conventions for global constants, or move it into a namespace or make it a static const member.
| int traces_per_chunk_ = 20; | |
| void NSInMemoryTraceWriter::AppendTraceEvent(TraceObject* trace_event) { | |
| MaybeCreateChunk(); | |
| json_trace_writer_->AppendTraceEvent(trace_event); | |
| total_traces_++; | |
| if (total_traces_ > 0 && (total_traces_ % traces_per_chunk_ == 0)) { | |
| int kTracesPerChunk = 20; | |
| void NSInMemoryTraceWriter::AppendTraceEvent(TraceObject* trace_event) { | |
| MaybeCreateChunk(); | |
| json_trace_writer_->AppendTraceEvent(trace_event); | |
| total_traces_++; | |
| if (total_traces_ > 0 && (total_traces_ % kTracesPerChunk == 0)) { |
| tracing_controller_->Initialize(TraceBuffer::CreateTraceBufferRingBuffer( | ||
| TraceBuffer::kRingBufferChunks, current_trace_writer_)); | ||
| // todo: create TraceConfig based on params. | ||
| TraceConfig* config = new TraceConfig(); |
There was a problem hiding this comment.
Potential memory leak: The TraceConfig object is allocated with new but never explicitly deleted. The StartTracing method may take ownership, but this should be verified. Consider using std::unique_ptr<TraceConfig> or verifying the ownership semantics of StartTracing.
| std::unique_ptr<TraceWriter> json_trace_writer_; | ||
| class NSInMemoryTraceWriter : public TraceWriter { | ||
| public: | ||
| NSInMemoryTraceWriter(std::string prefix, std::string suffix) |
There was a problem hiding this comment.
Constructor parameters prefix and suffix are passed by value, causing unnecessary string copies. Consider changing to const std::string& to avoid copying: NSInMemoryTraceWriter(const std::string& prefix, const std::string& suffix).
| NSInMemoryTraceWriter(std::string prefix, std::string suffix) | |
| NSInMemoryTraceWriter(const std::string& prefix, const std::string& suffix) |
| lastTrace_.push_back( | ||
| R"({"method": "Tracing.tracingComplete", "params": {"dataLossOccurred": false}})"); | ||
| } | ||
|
|
There was a problem hiding this comment.
Memory leak: current_trace_writer_ is allocated with new in the start() method but is only set to nullptr here without being deleted. The TraceBuffer may take ownership, but if it doesn't, this creates a memory leak. Consider using std::unique_ptr<NSInMemoryTraceWriter> for automatic memory management or explicitly delete before setting to nullptr.
| delete current_trace_writer_; |
1 participant
This improves the profiler by reducing string copying as well as running it all in C++ (no more entering JS to generate the output). As a result, memory is much more stable and performance is improved, allowing you to run long and accurate traces