Logger and timer macros improvements

[pemeliya]

Summary

This PR hardens the logging subsystem (include/mori/utils/mori_log.hpp) on two fronts:

1. Fixes a data race in ModuleLogger's internal maps that, under the
   multithreaded RDMA workload, could hand back an empty shared_ptr that
   callers then cached forever.
2. Removes ScopedTimer overhead on the hot path so timers compile down to
   a true no-op unless DEBUG logging is actually enabled for the module.

It also adds a dedicated, TSAN-friendly concurrency test and silences a
deprecated-literal warning coming from the bundled fmt/spdlog.

Motivation

ModuleLogger guarded its loggers_ / envOverrides_ maps inconsistently and
relied on a recursive_mutex, while several methods called each other and
re-locked. Concurrent first-touch of a module (or resolution of an unknown
module) raced the on-demand insert, occasionally returning a null logger that a
call-site cached permanently. Separately, ScopedTimer always copied strings,
read the clock twice, and did a per-call GetLogger() hashtable lookup even when
DEBUG was off — measurable on hot paths such as a per-batch MORI_IO_TRACE.

Changes

ModuleLogger thread-safety

• Eagerly create every known module logger in the singleton constructor, while
  still single-threaded, so loggers_ is fully populated before any
  GetLogger() can touch the map.
• Split the public API from *Locked helpers (GetLoggerLocked,
  InitModuleLocked, HasEnvOverrideLocked) so methods no longer re-lock via
  each other.
• Replace the recursive_mutex with a plain std::mutex.
• GetLogger() never returns null: unknown modules are created on demand, and a
  hard failure logs a fatal message and abort()s instead of silently dropping
  logs.
• SetModuleLevel() now emits the env-override warning via the pre-resolved
  logger while holding the lock (no re-entrant GetLogger).

ScopedTimer hot-path optimization

• Resolve and cache the (module -> logger) lookup once per call-site in a
  function-local static (MORI_TIMER / MORI_FUNCTION_TIMER).
• Constructors early-out when the level is disabled: no string copy, no clock
  read, no allocation. The enabled path is marked MORI_UNLIKELY.
• Unified name handling under std::string_view, copying into an owned string
  only on the enabled path (safe for temporaries).
• Dropped the redundant enabled_/module_ members; logger_ != nullptr now
  serves as the "enabled" sentinel.

Supporting changes

• include/mori/core/utils/utils.hpp: add MORI_LIKELY / MORI_UNLIKELY
  branch-hint macros and scope the warpSize macro to device compilation.
• CMakeLists.txt: silence -Wdeprecated-literal-operator from bundled fmt
  for both the spdlog build and mori_logging consumers (guarded by a
  compiler-flag check).

Tests

• New tests/cpp/utils/test_logging.cpp with multi-threaded stress tests:
  • ResolveNeverReturnsNull — concurrent resolution of known + unknown modules.
  • ConcurrentFirstTouchSameModule — 64 threads first-touch the same new
    module; all must observe one stable, non-null instance.
  • LogWhileReconfiguring — logging while levels are reconfigured concurrently.
• Wired into tests/cpp/CMakeLists.txt as test_logging (builds independently
  of BUILD_IO since core logging is header-only). Intended to be run under
  ThreadSanitizer to prove map access is synchronized.

[Copilot]

Pull request overview

Hardens ModuleLogger concurrency behavior and reduces hot-path overhead for logging/timing macros, adding a dedicated multithreaded regression test and minor build-system tweaks to quiet a bundled warning.

Changes:

• Makes ModuleLogger map access consistently mutex-protected and prevents GetLogger() from returning null by eager init + on-demand creation with hard-fail fallback.
• Optimizes ScopedTimer and logging macros to avoid unnecessary work when DEBUG is disabled (call-site caching, early-outs, string_view handling).
• Adds a new TSAN-friendly concurrency test and wires it into CTest; adds branch-hint macros and suppresses a deprecated literal-operator warning for bundled fmt/spdlog.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
tests/cpp/utils/test_logging.cpp	Adds multithreaded stress tests targeting logger resolution/creation races and concurrent reconfiguration.
tests/cpp/CMakeLists.txt	Builds and registers the new test_logging gtest binary independently of BUILD_IO.
include/mori/utils/mori_log.hpp	Refactors ModuleLogger locking/initialization and updates logging/timer macros for hot-path performance.
include/mori/core/utils/utils.hpp	Adds MORI_LIKELY/MORI_UNLIKELY and scopes warpSize macro to device compilation.
CMakeLists.txt	Conditionally applies -Wno-deprecated-literal-operator to spdlog and mori_logging consumers.

Suppressed comments (2)

include/mori/utils/mori_log.hpp:424

• ScopedTimer::ElapsedSeconds() computes Clock::now() - start_ even when the timer is disabled (logger_ == nullptr), but start_ is left default-initialized in that case. That yields a meaningless large duration (time since epoch) if ElapsedSeconds() is called on a disabled timer.

  double ElapsedSeconds() const {
    return std::chrono::duration<double>(Clock::now() - start_).count();
  }

include/mori/utils/mori_log.hpp:445

• MORI_FUNCTION_TIMER also expands to multiple statements, with the same single-statement-context pitfalls as MORI_TIMER. Converting it to a single declaration expression keeps the scope-based RAII behavior while remaining safe in if/else without braces.

#define MORI_FUNCTION_TIMER(module)                                 \
  static const std::shared_ptr<spdlog::logger> _mori_timer_logger = \
      ::mori::ModuleLogger::GetInstance().GetLogger(module);        \
  ::mori::ScopedTimer timer_instance(__PRETTY_FUNCTION__, _mori_timer_logger.get())

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[pemeliya]

Hi @maning00, you mentioned that includng utils.hpp from mori_log.hpp breaks some device module compilations?

Can you point me to the relevant module? I do not see any compile errors locally. The reason I did it is to have MORI_LIKELY/UNLIKELY macros to be defined in one central place..