Profiler changes #95
Profiler changes #95
Conversation
ETTrace/Tracer/EMGTracer.mm
Outdated
| @@ -49,25 +39,23 @@ + (BOOL)isRecording { | |||
| + (void)stopRecording:(void (^)(NSDictionary *))stopped { | |||
| [sStackRecordingThread cancel]; | |||
| sStackRecordingThread = nil; | |||
| NSLog(@"exiting"); | |||
|
|
||
| auto emplaceResult = threadsMap.try_emplace(threads[i], threads[i], mainMachThread); | ||
| size_t startIndex = addressStorage.size(); | ||
| // TODO: previously, we caught an std::length_error here. why was that happening? |
There was a problem hiding this comment.
It looks like we didn't handle that case very well in the catch block, so I'm fine with removing it
| #import <vector> | ||
| #import <unordered_map> | ||
| #import <mach/mach.h> | ||
| #import <QuartzCore/QuartzCore.h> |
There was a problem hiding this comment.
Minor thing but I think you can remove this import since CACurrentMediaTime is only used in the implementation file
There was a problem hiding this comment.
We still need something for CFTimeInterval
| } | ||
|
|
||
| void EMGStackTraceRecorder::recordStackForAllThreads(bool recordAllThreads, thread_t mainMachThread, thread_t etTraceThread) { | ||
| std::lock_guard<std::mutex> lockGuard(threadsLock); |
There was a problem hiding this comment.
Did you also want to remove this by ensuring the collectThreadSummaries was always called on the same recording thread? Or is that not a bottleneck so not worth doing?
There was a problem hiding this comment.
It was less of a performance thing, more of a logic thing (e.g., not accidentally putting code above the lock in the future). However, doing it the other way has its own complications, so I just stuck with the mutex approach
|
testing looks good. i will say, profiling in general (with or without my change) seems off on real devices (not seeing my own functions in the profiling result), but maybe i'm doing something wrong |
|
@noahsmartin i can't merge, can you do it |
newpointers which also avoids memory leaks)As far as performance goes though, it's largely bottlenecked both before and after this PR by FIRCLSReadMemory, which calls vm_read_overwrite. I imagine it's because it doesn't trust the address to be safe to directly dereference without causing a segfault. It currently takes about 0.1 ms for my test case for a single sampling of the main thread, although it's largely dependent on stack depth I imagine. Replacing it with a memcpy makes it take about 0.04ms, at which point thread_get_state is the next-biggest culprit.