Conversation
| // 1. Calculate elapsed since the Timestamp recorded in anchor. | ||
| // 2. Add adjustment - the difference between the anchor wall-clock time and the profile start wall-clock time. | ||
| #[cfg(target_os = "macos")] | ||
| let to_profile_time = { |
There was a problem hiding this comment.
why not make this into a separate function?
mythical-fred
left a comment
There was a problem hiding this comment.
Clean fix for the MacOS marker-timestamp mismatch. The mechanical change is right: Apple's samply emits marker timestamps relative to meta.startTime (a wall-clock Date.now() value), Linux samply emits absolute monotonic ns; the to_profile_time closure rebases via the captured (monotonic, wall_clock) anchor pair. The cfg(target_os = "macos") partitioning is tidy and the Linux path stays byte-equivalent to before. Mihai already approved.
A few non-blocking notes inline. No regression risk on Linux.
On the missing-tests rule: this is a no-CI-coverage MacOS-only path and the rest of crates/samply is similarly untested. PR description says it was validated on real MacOS pipelines, which is the honest answer for clock-conversion code. Tracking a "samply: add cross-platform clock-conversion unit test" issue would be nice (you can mock both halves of the anchor with a fixture).
| let (numer, denom) = *NANOS_PER_TICK.get_or_init(|| { | ||
| let mut info = mach_timebase_info_data_t { numer: 0, denom: 0 }; | ||
| unsafe { | ||
| mach_timebase_info(&mut info); |
There was a problem hiding this comment.
Tiny correctness nit: mach_timebase_info returns kern_return_t; you're ignoring it and silently falling back to (1, 1) if denom==0. The fallback is fine in practice (the call essentially can't fail on a real Mach kernel), but a brief comment saying "call cannot realistically fail; (1,1) is a safe identity if it ever does" would document the intent.
Also: ticks * u64::from(numer) is unchecked. On Apple Silicon numer/denom is 1/1, on x86 Macs it's typically 125/3; with ticks ~ 10^17 for a multi-day profile this can theoretically overflow u64. Profiling sessions are short enough that it doesn't matter, but worth a one-liner comment acknowledging the assumption.
| #[cfg(not(target_os = "macos"))] | ||
| { | ||
| // SAFETY: On Unix, `Instant` is implemented using CLOCK_MONOTONIC, | ||
| // which is the clock that we need to use for the profiler, but the Rust |
There was a problem hiding this comment.
The MacOS branch reads Instant::now() and mach_absolute_time_nanos() back-to-back and treats them as the same instant. They aren't — there's a sub-µs gap between the two syscalls. For profile markers in the ms range it's invisible noise, but a comment saying "anchored within sub-µs; acceptable for profile resolution" would prevent a future reader from worrying.
| unparker, | ||
| request_exit, | ||
| #[cfg(target_os = "macos")] | ||
| anchor: (Timestamp::now(), unix_epoch_nanos()), |
There was a problem hiding this comment.
Same drift caveat applies to (Timestamp::now(), unix_epoch_nanos()) — two non-atomic clock reads sampled as one anchor point. Fine for the use case; a one-line comment would document it.
| // (anchor_monotonic_ns, anchor_wall_clock_ns) profile_start_wall_clock_ms timestamp | ||
| // ``` | ||
| // | ||
| // To convert a timestamp to relative nanoseconds expected by samply, we need to: |
There was a problem hiding this comment.
profile_start_time_ms * 1_000_000.0 as i64: epoch ms today is ~1.7e12, ×1e6 = 1.7e18, well within i64 — fine. The lossy as i64 cast would silently saturate around year 2262; that's not a real risk but the cast deserves a as i64 // truncates fractional ns comment so it's obvious it's intentional.
| let now = Instant::now(); | ||
| let mach_now = mach_absolute_time_nanos(); | ||
| Self(if value <= now { | ||
| mach_now - now.duration_since(value).as_nanos() as i64 | ||
| } else { | ||
| mach_now + value.duration_since(now).as_nanos() as i64 | ||
| }) |
There was a problem hiding this comment.
I think now - value is a fixed offset. If so, it might be better to compute it once instead of recomputing it each time.
There was a problem hiding this comment.
Good point. After going back and forth with this, it turns out that the same implementation of this function that works for linux, works on MacOS as well. So this problem is now solved.
| log = "0.4.20" | ||
| md-5 = "0.10.6" | ||
| memory-stats = { version = "1.2.0", features = ["always_use_statm"] } | ||
| mach2 = "0.4.3" |
There was a problem hiding this comment.
The latest version is 0.6.0, is there a reason to prefer 0.4.3?
mythical-fred
left a comment
There was a problem hiding this comment.
LGTM. The new to_profile_time closure and the (anchor_monotonic_ns, anchor_wall_clock_ns) destructuring against self.anchor: (Timestamp, i64) match field order — no swap. Marker start/end and memory sample times all flow through the conversion.
Samply uses a different representation for marker timestamps on MacOS vs Linux. On Linux timestamps represent absolute values of the monotonic clock in nanoseconds. On MacOS timestamps are relative to the start time of the profile stored in the profile's metadata. This commit adds support for the latter format. Signed-off-by: Leonid Ryzhyk <ryzhyk@gmail.com>
Signed-off-by: feldera-bot <feldera-bot@feldera.com>
mythical-fred
left a comment
There was a problem hiding this comment.
Re-APPROVE on 4e997f6. Both prior inline concerns resolved: From<Instant> no longer reads two clocks back-to-back on macOS (the zero-Instant trick works there too because std::time::Instant on macOS goes through mach_timebase_info inside Duration arithmetic, so as_nanos yields the same units as mach_absolute_time_nanos). And feldera-samply now has a description field. Plus the cleaner version-bump pickup and SLT fix on the base. Ship it. 🐟
Fixes #6323 .
Samply uses a different representation for marker timestamps on MacOS vs Linux. On Linux timestamps represent absolute values of the monotonic clock in nanoseconds. On MacOS timestamps are relative to the start time of the profile stored in the profile's metadata. This commit adds support for the latter format.
Describe Manual Test Plan
Tested on several pipelines running on MacOS.
Checklist
Breaking Changes?
Mark if you think the answer is yes for any of these components:
Describe Incompatible Changes