Enhances Timer with format options, enable toggle, and GPU sync

[ooctipus]

Description

Adds configurable time format (s/ms/us/ns), global enable/enable_display_output toggles, and wp.synchronize_device() before stopping to ensure accurate GPU timing. Output now shows last/mean/std/n per measurement.

Credit Fully goes to : @AntoineRichard I just copied pasted his solution from dev/newton

Type of change

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (existing functionality will not work without user modification)
• Documentation update

Screenshots

Please attach before and after screenshots of the change if applicable.

Checklist

• I have read and understood the contribution guidelines
• I have run the pre-commit checks with ./isaaclab.sh --format
• I have made corresponding changes to the documentation
• My changes generate no new warnings
• I have added tests that prove my fix is effective or that my feature works
• I have updated the changelog and the corresponding version in the extension's config/extension.toml file
• I have added my name to the CONTRIBUTORS.md or my name already exists there

[greptile-apps]

Greptile Summary

This PR enhances isaaclab.utils.timer.Timer with configurable time units (s/ms/us/ns), global and per-instance enable/disable toggles, a reset() static method, richer context-manager output (last/mean/std/n), and a wp.synchronize() call inside stop() to flush pending GPU kernels before sampling the clock. The Welford accumulator is refactored into a dedicated _welford_state class-level dict, keeping timing_info clean. The test suite is also significantly expanded and no longer requires a full simulation app bootstrap.

Key observations:

• The wp.synchronize() call inside stop() uses the all-device variant instead of wp.synchronize_device(), which was explicitly mentioned in the PR description; this unnecessarily stalls unrelated GPU devices in multi-GPU environments.
• GPU synchronization is unconditional — every stop() call pays the sync cost even when the timer is wrapping pure CPU work, which can inflate measurements and add latency.
• The global Timer.enable class variable is only evaluated at __init__ time; this snap-shot semantics is tested but undocumented, making the flag easier to misuse than a true dynamic kill-switch.
• The sample standard deviation fix (n - 1 denominator) addressed in earlier review threads is correctly applied in the new implementation.

Confidence Score: 3/5

• Safe to merge after resolving the wp.synchronize() vs wp.synchronize_device() mismatch; the other findings are style/documentation level.
• The main logic concern is using wp.synchronize() (all devices) instead of the intended wp.synchronize_device() (current device), which can cause unintended cross-device stalls in multi-GPU environments. This is a genuine functional difference from the stated design. The unconditional GPU sync for CPU-only timers is a performance concern but not a correctness bug. All other changes (Welford refactor, unit formatting, enable toggles, reset method) look correct and are well-tested.
• source/isaaclab/isaaclab/utils/timer.py — specifically the stop() method's wp.synchronize() call.

Important Files Changed

Filename	Overview
source/isaaclab/isaaclab/utils/timer.py	Core Timer rewrite: adds configurable time units, global/instance enable toggles, and a wp.synchronize() call on stop. The synchronise call incorrectly uses wp.synchronize() (all devices) instead of wp.synchronize_device() (current device) as stated in the PR description, which can over-stall multi-GPU setups. The GPU sync is also unconditional, adding overhead to CPU-only timers.
source/isaaclab/test/utils/test_timer.py	Test suite significantly expanded with new tests for enable toggles, time units, reset, and display output. Correctly drops the full simulation app bootstrap in favour of a bare wp.init(). All new test cases look correct and cover the new features well.
source/isaaclab/docs/CHANGELOG.rst	Changelog entry for 4.5.1 added correctly, describing the new Timer features.
source/isaaclab/config/extension.toml	Version bumped from 4.5.0 to 4.5.1, matching the changelog entry.

Sequence Diagram

sequenceDiagram
    participant User
    participant Timer
    participant warp as wp (Warp)
    participant GlobalState as Timer.timing_info / _welford_state

    User->>Timer: Timer(msg, name, enable, time_unit)
    Note over Timer: Captures Timer.enable at init<br/>Validates time_unit

    User->>Timer: start()
    alt _enable is False
        Timer-->>User: return (no-op)
    else _enable is True
        Timer->>Timer: _start_time = perf_counter()
    end

    User->>Timer: stop()
    alt _enable is False
        Timer-->>User: return (no-op)
    else _enable is True
        Timer->>warp: wp.synchronize()  ⚠️ all devices
        warp-->>Timer: GPU work flushed
        Timer->>Timer: _elapsed_time = perf_counter() - _start_time
        alt name is not None
            Timer->>GlobalState: _update_welford(elapsed)
            GlobalState-->>Timer: timing_info[name] updated
        end
    end

    opt Context manager __exit__
        Timer->>User: print(msg, Last/Mean/Std/N)
    end

Loading

_{Last reviewed commit: 804830e}

[greptile-apps]

_{4 files reviewed, 7 comments}

_{Edit Code Review Agent Settings | Greptile}

[greptile-apps]

Additional Comments (1)

source/isaaclab/test/utils/test_timer.py
Missing test coverage for new features: format parameter (s/ms/us/ns), enable parameter, enable_display_output toggle, and GPU synchronization behavior.

[AntoineRichard]

LGTM. Need to change to (n-1) though like the greptile says.

[AntoineRichard]

@greptile

[greptile-apps]

wp.synchronize() syncs all devices, not just the current one

The PR description explicitly states that wp.synchronize_device() should be used "before stopping to ensure accurate GPU timing", but the implementation calls wp.synchronize() instead. These are meaningfully different:

• wp.synchronize() – blocks until all CUDA devices finish their work.
• wp.synchronize_device() – blocks until only the current (active) device finishes its work.

In a multi-GPU setup this causes unnecessarily broad synchronisation, stalling work on unrelated devices and inflating the measured time. The more targeted call matches the stated intent and should be used:

Suggested change

	wp.synchronize()
	wp.synchronize_device()

[greptile-apps]

Unconditional GPU sync adds overhead to CPU-only timers

wp.synchronize() is called on every stop() regardless of whether any GPU kernels were submitted during the timed interval. For timers used around pure CPU work (e.g., data loading, configuration, bookkeeping) this adds a non-trivial, unnecessary stall.

Consider making GPU synchronization opt-in via a parameter (e.g., sync_cuda: bool = True) so that callers who know they are timing CPU-only paths can avoid the extra cost:

def __init__(self, ..., sync_cuda: bool = True):
    ...
    self._sync_cuda = sync_cuda

def stop(self):
    if not self._enable:
        return
    if self._start_time is None:
        raise TimerError("Timer is not running. Use .start() to start it")
    if self._sync_cuda:
        wp.synchronize_device()
    ...

[greptile-apps]

Global Timer.enable is only evaluated at instantiation

self._enable is captured once during __init__. If Timer.enable is later set to False, existing timer instances are unaffected and will continue to record timing data. Conversely, setting Timer.enable = False after instance creation has no effect on live timers.

This behaviour is implicitly tested (the test sets Timer.enable = False before creating the instance), but the docstring for enable does not clarify this. Users who expect a global kill-switch that affects already-created timers will be surprised. Consider adding a note to the enable docstring:

enable: ClassVar[bool] = True
"""Whether to enable newly created timers.

Note:
    This flag is evaluated at instantiation time. Changing it after a
    :class:`Timer` has been created does not affect that instance.
"""