Skip to content

feat: return nanoseconds from monotonic_clock_stop#1

Open
bikallem wants to merge 2 commits intomoonbitlang:mainfrom
bikallem:bench-nanosecond-resolution
Open

feat: return nanoseconds from monotonic_clock_stop#1
bikallem wants to merge 2 commits intomoonbitlang:mainfrom
bikallem:bench-nanosecond-resolution

Conversation

@bikallem
Copy link
Copy Markdown

@bikallem bikallem commented Apr 1, 2026

Summary

Change moonbit_monotonic_clock_stop to return elapsed time in nanoseconds instead of microseconds. This gives moon bench the resolution needed to measure sub-microsecond operations accurately.

Changes

Windows (QueryPerformanceCounter):

  • Cast tick delta to double before scaling by 1e9 to avoid int64 overflow for intervals longer than ~15 minutes

Linux/macOS (clock_gettime):

  • Compute tv_sec * 1_000_000_000LL + tv_nsec directly, instead of tv_sec * 1_000_000 + tv_nsec / 1000.0
  • This eliminates the lossy division that previously discarded sub-microsecond precision

Merge order

These three PRs must land in this order:

  1. This PR (land first) — runtime returns nanoseconds
  2. feat(bench): use nanosecond resolution for benchmarks core#3406 (land second) — bench library uses ns thresholds and conversion factors. CI native target will fail until this PR lands.
  3. feat(bench): display nanosecond resolution in bench output moon#1588 (land last) — display code adds ns tier to auto_select_unit

Before / After

Before (µs resolution):

fdct   0.01 µs ±  0.00 µs

After (ns resolution):

fdct  153.87 ns ±  4.21 ns

Test plan

  • Verify moon bench shows nanosecond values for fast benchmarks on native target
  • Verify Windows build compiles cleanly (uses double cast to avoid overflow)
  • Verify Linux/macOS clock_gettime path returns correct elapsed nanoseconds

Change `moonbit_monotonic_clock_stop` to return elapsed time in
nanoseconds instead of microseconds. This gives `moon bench` the
resolution needed to measure sub-microsecond operations accurately.

Windows: multiply by 1_000_000_000 instead of 1_000_000.
Linux/macOS: compute `tv_sec * 1_000_000_000 + tv_nsec` directly
instead of converting nanoseconds to microseconds.

Coordinated with:
- moonbitlang/core: update bench library thresholds and conversion
  factors
- moonbitlang/moon: update display code to use ns as base unit
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 64d1a158f7

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

runtime.c Outdated
Comment on lines 2213 to 2214
return (double)((counter.QuadPart - ts->ts.QuadPart) * 1000000000LL) /
freq.QuadPart;
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Avoid int64 overflow in Windows ns conversion

The Windows implementation now computes (counter.QuadPart - ts->ts.QuadPart) * 1000000000LL in 64-bit integer arithmetic before casting to double, which can overflow for ordinary long-running intervals (e.g., around 15 minutes at a 10 MHz performance counter) and produce corrupted elapsed times. This is a regression from the previous microsecond scaling, which had a much larger safe range. Convert to floating-point before scaling (or reorder operations) so the intermediate does not overflow.

Useful? React with 👍 / 👎.

Cast tick delta to double before scaling by 1e9 to prevent integer
overflow for intervals longer than ~15 minutes at typical 10 MHz
performance counter frequencies.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant