Cpu utilization windows native - #1789
Conversation
New cpu_utilization_windows_system provider using GetSystemTimes(), following the timing pattern of cpu_energy_rapl_scaphandre_component. Registers the metric in phase_stats.py and the XGBoost provider, where CPU utilization providers are enumerated as explicit lists.
New cpu_utilization_windows_core provider using NtQuerySystemInformation(SystemProcessorPerformanceInformation) for per-logical-core busy/DPC/interrupt time, complementing the existing system-wide cpu_utilization_windows_system provider. Registers the metric in phase_stats.py using the same allowlist pattern.
- --with-interrupt-dpc CLI flag, off by default - include_interrupt_dpc config option - config.yml.example entry
…aximilianJahns/green-metrics-tool_rapl_metric_provider into cpu-utilization-windows-native # Please enter a commit message to explain why this merge is necessary, # especially if it merges an updated upstream into a topic branch. # # Lines starting with '#' will be ignored, and an empty message aborts # the commit.
…aximilianJahns/green-metrics-tool_rapl_metric_provider into cpu-utilization-windows-native # Please enter a commit message to explain why this merge is necessary, # especially if it merges an updated upstream into a topic branch. # # Lines starting with '#' will be ignored, and an empty message aborts # the commit.
…aximilianJahns/green-metrics-tool_rapl_metric_provider into cpu-utilization-windows-native
- Build new provider binaries in install_windows.ps1 - Fix cold-start zero: sleep after first snapshot, before loop starts - Update psu_energy_ac_sdia_machine to accept cpu_utilization_windows_system - Add METRIC_MAPPINGS entries to config.js.example - Translate German comment to English - Fix tabs -> spaces in phase_stats.py - Add trailing newlines - Remove automated_test/ from .gitignore
Eco CI Output - Old Energy EstimationEco CI Output [RUN-ID: 29943951378]:
🌳 CO2 Data: Total cost of whole PR so far: |
|||||||||||||||||||||||||||||||||||
|
Eco CI Output [RUN-ID: 29944894841]:
🌳 CO2 Data: Total cost of whole PR so far: |
|||||||||||||||||||||||||||||||||||
|
@coderabbitai review |
✅ Action performedReview finished.
|
📝 WalkthroughWalkthroughAdds Windows system-wide and per-core CPU utilization providers with native collectors, Python wrappers, build scripts, and installer integration. Updates Windows configuration templates and frontend metric mappings. Includes the new metrics in phase statistics and system CPU utilization assignment. Extends SDIA and XGBoost PSU providers to recognize Windows CPU utilization configuration and log files. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
install_windows.ps1 (1)
236-244: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDo not let optional provider build failures abort the whole install.
$ErrorActionPreference = "Stop"turns theseWrite-Errorcalls into terminating errors, so any MSVC/compiler failure inInvoke-ScaphandreProviderBuild,Invoke-CpuUtilizationSystemProviderBuild, orInvoke-CpuUtilizationCoreProviderBuildstops before Python requirements and Docker container builds. These providers are optional and disabled/commented-out by default, so useWrite-Warningfor these failures, or wrap the calls at Line 655-657 intry { ... } catch { Write-Warning $_.Exception.Message }before the Docker build step.
🧹 Nitpick comments (1)
install_windows.ps1 (1)
277-373: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract a shared helper to eliminate triple duplication across the three build functions.
Invoke-ScaphandreProviderBuild,Invoke-CpuUtilizationSystemProviderBuild, andInvoke-CpuUtilizationCoreProviderBuildare nearly byte-for-byte identical (cl.exe fast path, vswhere/vcvarsall fallback, error messages), differing only in$providerDir/$sourceFile/label text. This duplication is exactly why theWrite-Error-halts-install issue above got introduced in three places in a single diff — consolidating into one parameterized function (e.g.Invoke-NativeProviderBuild -ProviderDir -SourceFile -OutputBinary -Label) would let a single fix propagate everywhere and prevent future drift between the copies.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5e132824-3a9e-4827-88f0-b75566be26e6
📒 Files selected for processing (13)
.gitignoreconfig.yml.examplefrontend/js/helpers/config.js.exampleinstall_windows.ps1lib/phase_stats.pymetric_providers/cpu/utilization/windows/core/build.batmetric_providers/cpu/utilization/windows/core/provider.pymetric_providers/cpu/utilization/windows/core/source.cmetric_providers/cpu/utilization/windows/system/build.batmetric_providers/cpu/utilization/windows/system/provider.pymetric_providers/cpu/utilization/windows/system/source.cmetric_providers/psu/energy/ac/sdia/machine/provider.pymetric_providers/psu/energy/ac/xgboost/machine/provider.py
ArneTR
left a comment
ArneTR
left a comment
There was a problem hiding this comment.
Thanks for the PR @ribalba and @MaximilianJahns
I put most of my comments in the Review directly attached to code lines.
Here some extras:
- Was this provider tested on low -i ? What is the minimum floor this provider can handle?
- In the Linux ones we introduced a minmum floor from which it makes no sense anymore and can actually lead to wrong averages (bc of the way we calculate averages). Maybe it makes sense to do that also here? Would be nice to have something from the docs we can link or something empirical.
PR for reference: Minumum Floor: be78f8d
| }, | ||
| "cpu_utilization_windows_system": { | ||
| "clean_name": "CPU %", | ||
| "source": "Windows", |
There was a problem hiding this comment.
Here it would be nice to see the actual source in the system. For linux we for instance do not list "linux" as the source, but the subsystem where we capture the metric, for instance "procfs".
There was a problem hiding this comment.
Updated both providers to use the concrete subsystem instead of the OS name — win32 for the system-wide provider (GetSystemTimes(), documented Win32 API) and ntapi for the per-core provider (NtQuerySystemInformation(), undocumented native NT API), consistent with how Linux uses procfs/cgroup rather than linux. Re: the same comment on phase_stats.py — that block only lists the metric name for aggregation purposes, there's no source field there to rename; the name itself (cpu_utilization_windows_system) stays as-is, same as cpu_utilization_procfs_system did.
| 'lmsensors_fan_component', | ||
| 'cpu_utilization_procfs_system', | ||
| 'cpu_utilization_mach_system', | ||
| 'cpu_utilization_windows_system', |
There was a problem hiding this comment.
same argument here: use subsystem if possible
| csv_buffer.write(generate_csv_line(phase['hidden'], run_id, metric, detail_name, f"{idx:03}_{phase['name']}", value_avg, 'MEAN', max_value, min_value, sampling_rate_avg, sampling_rate_max, sampling_rate_95p, unit)) | ||
|
|
||
| if metric in ('cpu_utilization_procfs_system', 'cpu_utilization_mach_system'): | ||
| if metric in ('cpu_utilization_procfs_system', 'cpu_utilization_mach_system', 'cpu_utilization_windows_system'): |
There was a problem hiding this comment.
where is cpu_utilization_windows_core added?
There was a problem hiding this comment.
cpu_utilization_machine is a single scalar used downstream (e.g. by the XGBoost energy provider). cpu_utilization_windows_core emits 8 separate per-core values per tick from a single process call; including it in that same assignment would mean each core's value overwrites the previous one in turn, silently keeping only the last core processed. cpu_utilization_windows_system already provides the single system-wide value that's the correct semantic fit for cpu_utilization_machine. Happy to add a short comment at that line to make this explicit for future readers, if useful.
There was a problem hiding this comment.
I don't understand this. GMT supports having multiple detail_names per metric. WE have this for instance with containers / network interfaces etc. Is that not the same case here too?
Does this mean we currently do not have cpu_utilization_windows_core as phase_stat?
There was a problem hiding this comment.
cpu_utilization_windows_core is present as a phase_stat — it's already in the outer allowlist a few lines above the one in your diff, so it goes through the same detail_name-per-row thing as containers/network interfaces: 8 MEAN rows (core_0...core_7), one per core.
This specific line is just doing something different: cpu_utilization_machine is a single number, used downstream by things like the XGBoost energy provider, which just wants one system-wide utilization value. I left cpu_utilization_windows_core out of this one on purpose — since it produces 8 values per tick instead of 1, stuffing it into this single variable would just mean whichever core gets processed last silently "wins" and overwrites the rest.
One alternative I considered: instead of a single variable, use 8 separate variables (or a dict keyed by core). I didn't go that way because nothing actually consumes per-core values there right now — and the per-core data is already fully available anyway, just query the detail_name rows in phase_stats. Adding 8 parallel variables would just be duplicating data I already have in a cleaner form, without anyone actually using it.
| metric_provider_executable='metric-provider-binary.exe', | ||
| skip_check=skip_check, | ||
| folder=folder, | ||
| disable_buffer=False, |
There was a problem hiding this comment.
why not disabling the buffer? How is windows default buffering? we would need to rewrite the base.py which currently uses stdbuf ... but all in all I see this also helpful for windows
There was a problem hiding this comment.
Currently has no effect for this provider — disable_buffer is only consumed inside BaseMetricProvider.start_profiling() (wraps the command with stdbuf -o0, Linux-only), but both Windows providers override start_profiling() entirely and never call super().start_profiling(). The actual unbuffered-output requirement is instead handled directly in source.c via setvbuf(stdout, NULL, _IONBF, 0), which is portable and doesn't depend on an external tool.
There was a problem hiding this comment.
I foresee this something tricky to debug when code is copied. In the current case if the C code contains a guard that disables buffering it is sufficient.
But if we ever use shell scripts or non-C callers in Windows this will fail.
So my question: Is there a CLI way for windows to disable buffering when redirecting to files?
There was a problem hiding this comment.
stdbuf is Linux-specific — it works via LD_PRELOAD, intercepting setvbuf() in the child process before main() even runs. Windows has no equivalent for arbitrary executables, since it doesn't have that kind of interposition mechanism built into the OS.
I searched a bit and one possible method could be ConPTY (CreatePseudoConsole, Windows Pseudoconsole API, available since Windows 10 1809). It's a pseudo console — making a child process think it's attached to a real terminal instead of a redirected file, which would trigger Windows' default unbuffered behavior automatically, no setvbuf() needed in the child at all.
But it has some real drawbacks:
ConPTY makes the child act like it's on a real terminal, so it can inject VT/ANSI escape sequences (cursor control, colors) into the output — those would need to be stripped before GMT's line parser sees them, risks silently corrupting data if done wrong.
There's a known Microsoft issue where CREATE_NEW_PROCESS_GROUP (already used via host_platform.terminate_process_group() for clean shutdown) doesn't play well with ConPTY-hosted processes — could break the shutdown path that works fine right now.
| # cpu_gpu: False | ||
| # dram: False | ||
| #--- CPU Utilization - System-wide CPU load via GetSystemTimes() | ||
| # cpu_utilization_windows_system: |
There was a problem hiding this comment.
I would activate this by default
| #include <limits.h> | ||
|
|
||
| /* ---- CLI arg parsing (identical pattern to RAPL provider) ---- */ | ||
| static unsigned int parse_int(char *argument) |
There was a problem hiding this comment.
Can windows not import our libraries from lib/c ? Maybe compile them separately as windows importable object files?
There was a problem hiding this comment.
gmt-lib.c/gmt-lib.h use POSIX-only headers (unistd.h, clock_gettime, access()), which aren't available under MSVC. The existing RAPL provider already established the pattern of not using lib/c on Windows and reimplementing the small set of needed helpers directly — we followed that for consistency. There are currently three Windows providers affected (RAPL, and these two). Splitting out a Windows-compatible subset (arg parsing + timing helpers, roughly what we already have as parse_int() and clock_state_t/clock_init()/now_us()) as a shared gmt-lib-windows.c looks like a reasonably contained follow-up — touches all three existing Windows providers though, so we'd suggest it as a separate PR rather than folding it into this one.
| /* ---- CPU time reading (this replaces read_cpu_proc() from procfs) ---- */ | ||
| typedef struct { | ||
| uint64_t idle; | ||
| uint64_t kernel; /* NOTE: kernel time INCLUDES idle time, same caveat as procfs system_time */ |
There was a problem hiding this comment.
can we link a documentation here where it is said that Windows kernel only tracks these three counters.
And what exactly is the caveat that system time includes idle time?
| uint64_t busy_delta = (total_delta > idle_delta) ? (total_delta - idle_delta) : 0; | ||
|
|
||
| uint64_t ts_us = now_us(&clock); | ||
| long value = (total_delta > 0) ? (long)((busy_delta * 10000ULL) / total_delta) : 0; |
There was a problem hiding this comment.
Why is the multiplication with 10,000 ? What is the unit of the delta? I would have expected a multiplication with 1,000 or 1,000,000 but 10,000 is suprising
There was a problem hiding this comment.
This isn't a time-unit conversion — it's a fixed-point precision choice for the ratio, unrelated to the FILETIME 100ns units used elsewhere. busy_delta / total_delta is a value in [0,1]; multiplying by 10000 instead of 100 encodes it as an integer with 0.01% precision (matching the existing cpu_utilization_procfs_system convention on Linux) instead of losing precision to whole-percent integer rounding.
| /* same compute/non_compute split logic as procfs, mapped to Windows fields */ | ||
| uint64_t total_delta = (curr.user - prev.user) + (curr.kernel - prev.kernel); | ||
| uint64_t idle_delta = curr.idle - prev.idle; | ||
| uint64_t busy_delta = (total_delta > idle_delta) ? (total_delta - idle_delta) : 0; |
There was a problem hiding this comment.
I have a hard time understanding this calulation and think this will be hard to maintain.
I would expect:
user+kernel/(idle+kernel+user)
But this seems to user+kernel-idle ...?
There was a problem hiding this comment.
if this is related to the fact that kernel contains idle I think it would be cleaner to create a transient var: "true_idle" or "true_system" or "system_no_idle" or something
There was a problem hiding this comment.
GetSystemTimes() is documented by Microsoft to return exactly three values: idle, kernel, and user time (https://learn.microsoft.com/en-us/windows/win32/api/processthreadsapi/nf-processthreadsapi-getsystemtimes). The "kernel includes idle" caveat: per the same documentation and confirmed against psutil's own Windows implementation (which applies the identical kernel - idle correction), Windows' kernel-mode time accounting counts idle-thread execution as kernel time — there's no separate "system-busy" counter the way Linux's /proc/stat separates system from idle. We've made this explicit in code now via named intermediate variables (kernel_delta, user_delta, idle_delta) plus an inline comment. I changed it to be better readable.
|
Please also have a look at the NitPick comments from Coderabbit. They look useful |
| prev = curr; | ||
|
|
||
| LONGLONG remaining_qpc = deadline - now_qpc(); | ||
| double sleep_ms = (remaining_qpc > 0) ? (remaining_qpc / qpc_ticks_per_ms) : 0.0; |
There was a problem hiding this comment.
I also wanted to flag this. This sleep behaviour is very different to the code we have in the other GMT providers.
What is the reason for implementing it that differently?
The benefits we have from our current version should then somehow guaranteed another way:
- Currently we put fixed sleeps and check resulting total sample times. These are checked to not execeed a 20% overhead. This guarantees for us that the provider itself does not consume excessive CPU ticks and we can safely run quite some in parallel
When calculating time differences in the provider itself we can run into situations where the provider can run continously and almost never sleep. So overhead must be checked inline.
I would argue for simplifying the C code as by requirement we anyway can have no provider that takes longer than 1-5 ms for processing the capture loop anyway. It would create noise on the measurement.
There was a problem hiding this comment.
Good point, agreed — simplified to a fixed Sleep(interval_ms) after each snapshot, same as the other providers, in both source.c files. The deadline-compensation logic was carried over from the RAPL provider pattern without re-evaluating whether it was actually needed here; you're right that it isn't, given the measurement itself is just a syscall (~1-5ms). Verified afterwards that intervals stay within ~2-6ms of the target with the simplified version, well inside the 20% tolerance check.
|
@ribalba If you want, feel free to copy this branch again for a merge... see the comments below for what I changed, hope you agree with the approach |
|
@ArneTR is this good to go? |
4 participants
This is a branch I copied from
https://github.com/MaximilianJahns/green-metrics-tool_rapl_metric_provider/tree/cpu-utilization-windows-native
and this was the PR
#1757
as I couldn't write my fixes to Maxes branch I pushed it here.
@ArneTR this seems to work under windows now and I could test it.
@MaximilianJahns Thank you for all the work
Summary