Add experimental support for tracking the invocations of runtime functions #11910
Conversation
|
@swift-ci please smoke test |
|
@moiseev Max, could you have a look at the stdlib part? |
|
@aschwaighofer Oh, I see, you are right! |
…cations of runtime functions It allows for collecting the state of runtime function counters, which are used to determine how many times a given runtime function was called. It is possible to get the global counters, which represent the total number of invocations, or per-object counters, which represent the number of runtime functions calls for a specific object.
swiftix
force-pushed
the
resilience-performance1
branch
from
4d7ff14 to
4b2c6c0
Compare
|
@swift-ci please test |
|
Build failed |
|
@swift-ci please test |
|
@gottesmm Michael, do you want to have a look at this attempt to provide runtime function counters? |
|
Build failed |
|
@swift-ci please smoke test OS X |
1 similar comment
|
@swift-ci please smoke test OS X |
|
@swift-ci please test OS X |
1 similar comment
|
@swift-ci please test OS X |
|
Build failed |
|
@swift-ci please test OS X |
|
Build failed |
|
@swift-ci please test OS X |
1 similar comment
|
@swift-ci please test OS X |
|
@swift-ci please smoke test linux |
… and initialization of static or stack-promoted objects
|
@swift-ci please smoke test |
| static public func dumpObjectsRuntimeFunctionPointers() | ||
|
|
||
| @_silgen_name("setGlobalRuntimeFunctionCountersUpdateHandler") | ||
| static public func setGlobalRuntimeFunctionCountersUpdateHandler( |
| let runtimeFunctionNames = _RuntimeFunctionCounters._getRuntimeFunctionNames() | ||
| let numRuntimeFunctionCounters = | ||
| _RuntimeFunctionCounters.getNumRuntimeFunctionCounters() | ||
| var runtimtFunctionNames : [String] = [] |
There was a problem hiding this comment.
Since you already know the future length of a resulting array, calling runtimeFunctionNames.reserveCapacity(numRuntimeFunctionCounters) will help avoid reallocation.
| _ value: Any, | ||
| references: inout [UnsafeRawPointer], | ||
| visitedItems: inout [ObjectIdentifier : Int] | ||
| ) { |
There was a problem hiding this comment.
Common style is to outdent (which turns out to be the term for 'un-indent') this line.
|
|
||
| // Recursively visit the children of the current value. | ||
| var currentIndex = mirror.children.startIndex | ||
| for _ in 0..<count { |
There was a problem hiding this comment.
Why can't you just for child in mirror.children?
| } | ||
| } | ||
|
|
||
| /// This is a namespace for runtime functions related to management |
There was a problem hiding this comment.
/// is used for doc comments only.
| init() | ||
|
|
||
| /// Dump the current state of all counters. | ||
| func dump(skipUnchanged: Bool) |
There was a problem hiding this comment.
How about this?
protocol _RuntimeFunctionCountersStats {
func dump<T : TextOutputStream>(skipUnchanged: Bool, to: inout T)
func dumpDiff<T : TextOutputStream>(_ after: Self, skipUnchanged: Bool, to: inout T)
}And then:
extension _RuntimeFunctionCounterStats {
public func dump(skipUnchanged: Bool) {
dump(skipUnchanged: skipUnchanged, to: &_StdOut)
}
}
extension _RuntimeFunctionCountersStats : CustomDebugStringConvertible {
public var debugDescription: String {
var result = ""
dump(skipUnchanged: true, to: &result)
return result
}
}| ) | ||
|
|
||
| // Use counter name as index. | ||
| subscript(_ index: String) -> UInt32 { |
There was a problem hiding this comment.
index is misleading here. Just call it counterName =)
| public // @testable | ||
| func enableRuntimeFunctionCountersUpdates( | ||
| mode: (globalMode: Bool, perObjectMode: Bool) = (true, true)) { | ||
| _ = _RuntimeFunctionCounters.setGlobalRuntimeFunctionCountersMode( |
There was a problem hiding this comment.
_ = should become unnecessary once the function is marked with a @discardableResult attribute.
| /// counters related to a given object. | ||
| public // @testable | ||
| func _measureRuntimeFunctionCountersDiffs( | ||
| objects: [UnsafeRawPointer], _ f: () -> Void) -> |
There was a problem hiding this comment.
f is not very descriptive. body perhaps? At least it has some part of the overall meaning...
| f() | ||
| // Disable counters updates. | ||
| _RuntimeFunctionCounters.enableRuntimeFunctionCountersUpdates( | ||
| mode: (globalMode: false, perObjectMode: false)) |
|
@swift-ci please smoke test |
|
I don't see any thread-safety here. Are these counters supposed to be atomic? |
|
@gparker42 I've put into the comments that we may want to make those counters atomic. But in principle, the main motivation for them is to write some tests and perform some performance investigations to track e.g. the number of ARC operations being performed during the execution of a given stdlib call like |
|
|
||
| namespace swift { | ||
| struct HeapObject; | ||
| } |
There was a problem hiding this comment.
} // end anonymous namespace
| #if !defined(NDEBUG) | ||
|
|
||
| /// The name of a helper function for tracking the calls of a runtime function. | ||
| #define SWIFT_RT_TRACK_INVOCATION_NAME(RT_FUNCTION, OBJ) \ |
There was a problem hiding this comment.
On a bunch of the parameters here, you have the OBJ parameter but you don't use it. Why is it still here?
|
|
||
| // Define counters used for tracking the total number of invocations of runtime | ||
| // functions. | ||
| struct RuntimeFunctionCountersState { |
There was a problem hiding this comment.
Is your intention here to have a c++ or a c API. If you want this to be a truly portable c API, there are a bunch of changes that would be necessary to achieve that.
| /// Get the runtime object state associated with an object and store it | ||
| /// into the result. | ||
| extern "C" void | ||
| getObjectRuntimeFunctionCounters(swift::HeapObject *object, |
There was a problem hiding this comment.
Why are you using a c++ reference with an extern "C". Seems weird.
There was a problem hiding this comment.
This is better, but you are still using C++ namespaces. If this is intended to be a C API, these should all be typedefs of void * or of an anonymous C struct.
| RT_FUNCTION_TO_TRACK(object, swift_allocObject) \ | ||
| RT_FUNCTION_TO_TRACK(object, swift_deallocObject) \ | ||
| RT_FUNCTION_TO_TRACK(object, swift_initStackObject) \ | ||
| RT_FUNCTION_TO_TRACK(object, swift_initStaticObject) \ |
There was a problem hiding this comment.
Don't we have some sort of .def file that you could just include?
| struct HeapObject; | ||
| } | ||
|
|
||
| /// Instrument the runtime functions only if it is a build with |
There was a problem hiding this comment.
"Instrument the runtime functions only if we are building with assertions enabled."
|
|
||
| /// The name of the counter for tracking the calls of a runtime function. | ||
| #define SWIFT_RT_FUNCTION_INVOCATION_COUNTER_NAME(RT_FUNCTION_NAME) \ | ||
| invocationCounter_##RT_FUNCTION_NAME |
There was a problem hiding this comment.
why is this exposed in the header. This seems like an implementation detail.
| /// The object state cache mapping objects to the collected state associated with | ||
| /// them. | ||
| /// TODO: Do we need to make it thread-safe? | ||
| static llvm::DenseMap<HeapObject *, RuntimeFunctionCountersState> RuntimeObjectStateCache; |
There was a problem hiding this comment.
My suggestion: just put in a lock.
| static llvm::DenseMap<HeapObject *, RuntimeFunctionCountersState> RuntimeObjectStateCache; | ||
|
|
||
| /// Define names of runtime functions. | ||
| #undef RT_FUNCTION_TO_TRACK |
There was a problem hiding this comment.
Why are you undefing this? This would be much simpler with a .def file.
|
@gottesmm Thanks for the review, Michael! I pushed a new version where I addressed most of your comments. |
|
@swift-ci please smoke test |
|
@swift-ci please smoke test |
…built with assertions
|
@swift-ci please smoke test |
1 similar comment
|
@swift-ci please smoke test |
|
@gottesmm Michael, is there anything still missing before I merge it? |
|
I am re-reading now. |
| } // end namespace swift | ||
|
|
||
| /// The name of a helper function for tracking the calls of a runtime function. | ||
| #define SWIFT_RT_TRACK_INVOCATION_NAME(RT_FUNCTION) \ |
There was a problem hiding this comment.
Why is this outside the #if !defined(NDEBUG)
There was a problem hiding this comment.
Because it is used also by the RuntimeInvocationsTracking.cpp, which is compiled always, even if NDEBUG is defined.
| /// Get the runtime object state associated with an object and store it | ||
| /// into the result. | ||
| extern "C" void | ||
| getObjectRuntimeFunctionCounters(swift::HeapObject *object, |
There was a problem hiding this comment.
This is better, but you are still using C++ namespaces. If this is intended to be a C API, these should all be typedefs of void * or of an anonymous C struct.
| /// FUNCTION_TO_TRACK(Id) | ||
| /// Id is the name of the runtime function to be tracked. | ||
|
|
||
| /// Set of runtime functions that whose invocations need to be tracked. |
There was a problem hiding this comment.
This makes all of the code look much nicer! Looks really nice! One more suggestion: there is no reason why one should include this if one doesn't define FUNCTION_TO_TRACK. I would add:
#ifndef FUNCTION_TO_TRACK
#error "Must define FUNCTION_TO_TRACK to include RuntimeInvocationsTracking.def"
#endif
| } | ||
|
|
||
| /// Define how to dump the counter for a given runtime function. | ||
| #define FUNCTION_TO_TRACK(RT_FUNCTION) \ |
There was a problem hiding this comment.
We generally put this /right/ next to the #include.
So I would put it like so:
static void dumpRuntimeCounters(...) {
uint32_t tmp = 0;
#define FUNCTION_TO_TRACK(RT_FUNCTION) \
... \
...
#include "RuntimeInvocationsTracking.def"
}
| /// Return the names of the runtime functions being tracked. | ||
| /// Their order is the same as the order of the counters in the | ||
| /// RuntimeObjectState structure. | ||
| SWIFT_RT_ENTRY_VISIBILITY |
There was a problem hiding this comment.
Are these null terminated? If so, you should document that.
| GlobalRuntimeFunctionCountersUpdateHandler; | ||
|
|
||
| /// Define offset for each function being tracked. | ||
| #define FUNCTION_TO_TRACK(RT_FUNCTION) \ |
There was a problem hiding this comment.
Can you move this next to the include of RuntimeInvocationsTracking.def in the function itself.
…they can be used from C
…InvocationsTracking.def"
… builds No runtime function calls will be tracked in this case, but the APIs for reading them are still available. This way there is no need to recompile Swift clients if they are linked against different builds of the standard library and runtime library.
|
@swift-ci please smoke test |
|
LGTM |
|
Big thanks to @gottesmm for his thorough review! |
|
@swift-ci please test |
1 similar comment
|
@swift-ci please test |
- Use SWIFT_RUNTIME_EXPORT instead of SWIFT_RT_ENTRY_VISIBILITY for exposed functions - Use `_swift_` prefixes on the names of exposed functions - Make the global counters and per-object counters cache thread-safe by using locks
Address post-commit review comments on PR #11910
It allows for collecting the state of runtime function counters, which are used to determine how many times a given runtime function was called.
It is possible to get the global counters, which represent the total number of invocations, or per-object counters, which represent the number of runtime functions calls for a specific object.
The APIs are implemented in the runtime library and are exposed as Swift APIs via the standard library. This way it is possible to write tests and benchmarks which collect very precise data about the amount of runtime functions invocations (e.g. swift_retain/swift_release) for an object or for a given code snippet.