[REFACTOR][RUNTIME] Phase out profiling.h heavy types, rename to timer.h#19455
Conversation
There was a problem hiding this comment.
Code Review
This pull request removes the runtime profiling infrastructure, including the Profiler class, the Report object, and per-operator profiling in the Relax Virtual Machine. Core timing primitives, Timer and WrapTimeEvaluator, are moved to a new timer module, and FFI names are updated to the runtime.timer namespace. Feedback suggests marking global variables in timer.cc as static to prevent symbol collisions and restoring the FFI registration for the Timer::Start dispatcher.
| std::set<DLDeviceType> seen_devices; | ||
| std::mutex seen_devices_lock; |
There was a problem hiding this comment.
The global variables seen_devices and seen_devices_lock are defined in the tvm::runtime namespace without being marked static. This can lead to symbol collisions during linking if other translation units define variables with the same names in the same namespace. Please mark them as static or move them into an anonymous namespace.
| std::set<DLDeviceType> seen_devices; | |
| std::mutex seen_devices_lock; | |
| static std::set<DLDeviceType> seen_devices; | |
| static std::mutex seen_devices_lock; |
There was a problem hiding this comment.
Done — went one step further: moved both statics inside Timer::Start as function-local statics. They're only used there, so function scope is the tightest possible. C++11 magic statics give thread-safe lazy init, so the mutex is still correct. See commit 45a3075c0b.
| t->Start(); | ||
| return t; | ||
| } | ||
| } |
There was a problem hiding this comment.
The FFI registration for Timer::Start (previously profiling.start_timer) is missing in the new timer.cc. While device-specific timers are registered, the generic dispatcher Timer::Start is a useful entry point for language bindings. Consider registering it under the runtime.timer namespace.
}
TVM_FFI_STATIC_INIT_BLOCK() {
namespace refl = tvm::ffi::reflection;
refl::GlobalDef().def("runtime.timer.start", Timer::Start);
}|
Fixed in commit de3f078. Wrapped seen_devices and seen_devices_lock in an anonymous namespace to ensure internal linkage and prevent ODR collisions. This is the idiomatic C++ pattern for file-local helpers and matches the codebase conventions. |
This PR removes the unused heavy profiling types (Profiler, Report, MetricCollector, Duration/Percent/Count/Ratio, ShapeString, DeviceWrapper, ProfileFunction) from runtime/profiling.h and renames the header to runtime/timer.h. Only Timer, TimerNode, and WrapTimeEvaluator are retained. Main changes: - Rename include/tvm/runtime/profiling.h → include/tvm/runtime/timer.h - Rename src/runtime/profiling.cc → src/runtime/timer.cc - Move WrapTimeEvaluator from tvm::runtime::profiling to tvm::runtime - Rename FFI keys profiling.timer.<device> → runtime.timer.<device> - Delete vm.profile() and VirtualMachineProfiler (TVM_VM_ENABLE_PROFILER) - Delete json_runtime RunProfile/CanDebug/_debug dispatch path - Delete CLML RunProfile override and dead MetricCollector declaration - Delete python/tvm/runtime/profiling/ package entirely
These are file-local helpers used only by Timer::Start; non-static namespace-scope globals risk ODR collision with another TU defining the same names. Wrap in anonymous namespace to ensure internal linkage and match idiomatic C++ patterns for file-scoped variables.
…unction-local statics Function-local statics give thread-safe lazy init via C++11 magic statics and limit visibility to the only consumer. Replaces the anonymous-namespace file-scope statics.
wasm_runtime.cc directly includes source files rather than linking. profiling.cc was removed in the profiling.h phase-out; replace with timer.cc which now carries the Timer and WrapTimeEvaluator implementations.
cc93787 to
04ac1bd
Compare
Summary
Phase out all heavy types from
include/tvm/runtime/profiling.hexceptTimer/TimerNode/WrapTimeEvaluator. Rename header toinclude/tvm/runtime/timer.hand corresponding source tosrc/runtime/timer.cc. Deletes unused symbols (Profiler,Report,MetricCollector,DeviceWrapper,ProfileFunction,vm.profile(),Duration/Percent/Count/Ratio,ShapeString) and updates all 13 include sites. Per user approval, FFI keys renamed toruntime.timer.<device>.Rationale: most of the cases we will switch to native profiling and relax vm have the hook mechanism to do so. So it is better to simplify things when possible