Dedup Windows CentralProcessor frequency queries; fix JNA numbering#3459
Merged
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
📜 Recent review details⏰ Context from checks skipped due to timeout. (14)
📝 WalkthroughWalkthroughWindows CPU frequency selection is centralized in ChangesWindows frequency querying
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant WindowsCentralProcessor
participant JNA_or_FFM
participant ProcessorInformation
participant NtPowerInformation
WindowsCentralProcessor->>JNA_or_FFM: queryProcessorPerformanceCounters()
JNA_or_FFM->>ProcessorInformation: query processor performance counters
ProcessorInformation-->>JNA_or_FFM: counter instances and values
WindowsCentralProcessor->>JNA_or_FFM: queryFrequencyCounters()
JNA_or_FFM->>ProcessorInformation: query processor frequency counters
ProcessorInformation-->>JNA_or_FFM: counter instances and values
WindowsCentralProcessor->>JNA_or_FFM: queryNTPower(fieldIndex)
JNA_or_FFM->>NtPowerInformation: query native frequency data
NtPowerInformation-->>JNA_or_FFM: processor frequencies
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
oshi-common/src/main/java/oshi/hardware/common/platform/windows/WindowsCentralProcessor.java (1)
224-238: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
isWindows7OrGreater()duplicatesisWindows8OrGreater().Both methods parse
os.versionand compare major/minor against a hard-coded threshold; only the threshold differs. Consider extracting a sharedisWindowsVersionOrGreater(int major, int minor)helper to avoid the duplication.♻️ Suggested consolidation
- private static boolean isWindows8OrGreater() { - // Use system property check that works without native access - String osVersion = System.getProperty("os.version", ""); - String[] parts = osVersion.split("\\."); - if (parts.length >= 2) { - int major = ParseUtil.parseIntOrDefault(parts[0], 0); - int minor = ParseUtil.parseIntOrDefault(parts[1], 0); - return major > 6 || (major == 6 && minor >= 2); - } - return false; - } + private static boolean isWindows8OrGreater() { + return isWindowsVersionOrGreater(6, 2); + } ... - private static boolean isWindows7OrGreater() { - String osVersion = System.getProperty("os.version", ""); - String[] parts = osVersion.split("\\."); - if (parts.length >= 2) { - int major = ParseUtil.parseIntOrDefault(parts[0], 0); - int minor = ParseUtil.parseIntOrDefault(parts[1], 0); - return major > 6 || (major == 6 && minor >= 1); - } - return false; - } + private static boolean isWindows7OrGreater() { + return isWindowsVersionOrGreater(6, 1); + } + + private static boolean isWindowsVersionOrGreater(int reqMajor, int reqMinor) { + String osVersion = System.getProperty("os.version", ""); + String[] parts = osVersion.split("\\."); + if (parts.length >= 2) { + int major = ParseUtil.parseIntOrDefault(parts[0], 0); + int minor = ParseUtil.parseIntOrDefault(parts[1], 0); + return major > reqMajor || (major == reqMajor && minor >= reqMinor); + } + return false; + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@oshi-common/src/main/java/oshi/hardware/common/platform/windows/WindowsCentralProcessor.java` around lines 224 - 238, Extract the shared os.version parsing and comparison logic from isWindows7OrGreater() and isWindows8OrGreater() into an isWindowsVersionOrGreater(int major, int minor) helper. Update both methods to delegate to it with their respective Windows version thresholds, preserving their existing behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@oshi-common/src/main/java/oshi/hardware/common/platform/windows/WindowsCentralProcessor.java`:
- Around line 265-283: Update mapPercentToFreqs to iterate with a source index
and read percentList using that index, since instances and percentList are
parallel. In the NUMA mapping branch, detect unmapped instances and skip them
rather than defaulting to logical processor 0; retain bounds checks and existing
frequency population behavior.
---
Nitpick comments:
In
`@oshi-common/src/main/java/oshi/hardware/common/platform/windows/WindowsCentralProcessor.java`:
- Around line 224-238: Extract the shared os.version parsing and comparison
logic from isWindows7OrGreater() and isWindows8OrGreater() into an
isWindowsVersionOrGreater(int major, int minor) helper. Update both methods to
delegate to it with their respective Windows version thresholds, preserving
their existing behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 536239fa-02bb-41a4-a55b-b0c3f65c7fc2
📒 Files selected for processing (4)
CHANGELOG.mdoshi-common/src/main/java/oshi/hardware/common/platform/windows/WindowsCentralProcessor.javaoshi-core-ffm/src/main/java/oshi/hardware/platform/windows/WindowsCentralProcessorFFM.javaoshi-core/src/main/java/oshi/hardware/platform/windows/WindowsCentralProcessorJNA.java
📜 Review details
⏰ Context from checks skipped due to timeout. (13)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Analyze (java)
- GitHub Check: Test JDK 21, macos-latest
- GitHub Check: Test JDK 25, macos-latest (+ JNA==FFM)
- GitHub Check: Test JDK 26, windows-latest
- GitHub Check: Test JDK 25, windows-latest (+ JNA==FFM)
- GitHub Check: Test JDK 25, ubuntu-latest (+ JNA==FFM)
- GitHub Check: Test JDK 21, windows-latest
- GitHub Check: Test JDK 26, macos-latest
- GitHub Check: Test JDK 25, OpenBSD (+ JNA==FFM)
- GitHub Check: Test JDK 25, FreeBSD (+ JNA==FFM)
- GitHub Check: Lint (Spotless, Checkstyle, ForbiddenAPIs, Javadoc)
- GitHub Check: Test JDK 25, OpenIndiana (+ JNA==FFM)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2026-04-12T19:22:09.716Z
Learnt from: dbwiddis
Repo: oshi/oshi PR: 3149
File: oshi-common/src/main/java/oshi/hardware/common/platform/mac/MacCentralProcessor.java:159-163
Timestamp: 2026-04-12T19:22:09.716Z
Learning: When reviewing OSHI hardware platform code that uses or implements `CentralProcessor`/`PhysicalProcessor.getEfficiency()`, remember the API’s counterintuitive semantics: a *higher* efficiency-class value indicates *greater performance* (lower efficiency). P-cores/“performance cores” should therefore have higher values (e.g., 1) than E-cores/“efficiency cores” (e.g., 0), as documented in the `CentralProcessor` Javadoc. Do not flag code as incorrect solely because ARM_P_CORES or Intel P-cores are assigned higher efficiency values.
Applied to files:
oshi-common/src/main/java/oshi/hardware/common/platform/windows/WindowsCentralProcessor.java
📚 Learning: 2026-04-12T19:34:56.879Z
Learnt from: dbwiddis
Repo: oshi/oshi PR: 3149
File: oshi-common/src/main/java/oshi/hardware/common/platform/mac/MacCentralProcessor.java:159-163
Timestamp: 2026-04-12T19:34:56.879Z
Learning: In OSHI’s Java platform implementations, `PhysicalProcessor.getEfficiency()` / `EfficiencyClass` follow the same convention as Windows `PROCESSOR_RELATIONSHIP.EfficiencyClass`: a *higher* numeric efficiency class value represents *greater performance* and *less efficiency* (typically P-cores higher than E-cores). When reviewing, do not treat cases where P-cores are assigned higher efficiency/class values as a bug—this is intentional cross-platform consistency.
Applied to files:
oshi-common/src/main/java/oshi/hardware/common/platform/windows/WindowsCentralProcessor.java
📚 Learning: 2026-07-06T19:25:59.225Z
Learnt from: dbwiddis
Repo: oshi/oshi PR: 3432
File: oshi-common/src/main/java/oshi/util/ExceptionUtil.java:26-56
Timestamp: 2026-07-06T19:25:59.225Z
Learning: In this repo’s SLF4J 1.x-compatible logging code, do not flag cases where a logger call uses SLF4J 1.6+ implicit-cause handling—e.g., `log.error("...{}", value, t)` (or similar with a single `{}` placeholder) where `t` is passed as the final/trailing argument. SLF4J will attach `t` as the log event’s cause with the full stack trace; this is equivalent to SLF4J 2.x’s fluent `atLevel(...).setCause(t).log(...)` and is intentionally used to preserve SLF4J 1.x runtime compatibility.
Applied to files:
oshi-common/src/main/java/oshi/hardware/common/platform/windows/WindowsCentralProcessor.javaoshi-core/src/main/java/oshi/hardware/platform/windows/WindowsCentralProcessorJNA.javaoshi-core-ffm/src/main/java/oshi/hardware/platform/windows/WindowsCentralProcessorFFM.java
📚 Learning: 2026-04-06T04:45:47.644Z
Learnt from: dbwiddis
Repo: oshi/oshi PR: 3121
File: oshi-core/src/main/java/oshi/software/os/linux/LinuxOperatingSystem.java:53-53
Timestamp: 2026-04-06T04:45:47.644Z
Learning: In the oshi/oshi project, only interfaces and classes in the root packages `oshi`, `oshi.hardware`, and `oshi.software.os` are considered the public API. Code in subpackages that represent specific implementations/platforms (e.g., `oshi.software.os.linux`, `oshi.hardware.platform.linux`, and similar subpackages) is implementation detail. When reviewing, do not treat changes to classes in these platform-specific subpackages as public API breaks (for example: making `LinuxOperatingSystem` abstract, narrowing constructor visibility, or adding/removing abstract methods should not be flagged as breaking public API).
Applied to files:
oshi-core/src/main/java/oshi/hardware/platform/windows/WindowsCentralProcessorJNA.java
🔇 Additional comments (5)
oshi-common/src/main/java/oshi/hardware/common/platform/windows/WindowsCentralProcessor.java (2)
201-222: LGTM on the new abstract contracts.Abstract hooks and javadocs for
queryProcessorPerformanceCounters(),queryFrequencyCounters(), andqueryNTPower()are clear and consistent with the JNA/FFM overrides.
240-263: Fallback chain and max-freq aggregation logic look correct.
queryCurrentFreq()'s Win7+ guard, performance→frequency→native fallback ordering, andqueryMaxFreq()'s use ofArrays.stream(...).max()are all logically sound, contingent on themapPercentToFreqs()fix noted above.Also applies to: 285-289
oshi-core/src/main/java/oshi/hardware/platform/windows/WindowsCentralProcessorJNA.java (1)
182-217: LGTM!oshi-core-ffm/src/main/java/oshi/hardware/platform/windows/WindowsCentralProcessorFFM.java (1)
206-244: LGTM!CHANGELOG.md (1)
9-9: LGTM!
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #3459 +/- ##
============================================
- Coverage 74.80% 74.78% -0.03%
Complexity 76 76
============================================
Files 587 587
Lines 21830 21805 -25
Branches 3656 3648 -8
============================================
- Hits 16331 16306 -25
- Misses 3465 3468 +3
+ Partials 2034 2031 -3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
dbwiddis
force-pushed
the
dedup-windows-centralproc-freq
branch
from
July 14, 2026 04:04
fbc9672 to
b4568ba
Compare
dbwiddis
changed the title
Hoist queryCurrentFreq, mapPercentToFreqs, and queryMaxFreq into the shared WindowsCentralProcessor base, with abstract hooks for the backend-specific native calls (queryProcessorPerformanceCounters, queryFrequencyCounters, queryNTPower) and a native-free isWindows7OrGreater() version check. The JNA mapPercentToFreqs, added in oshi#3233, indexed the parallel percent list by computed CPU number and defaulted unmapped NUMA instances to CPU 0, so on multi-processor-group / NUMA systems it assigned current frequencies to the wrong logical processors. Unify on the positional numbering already used by processTickData (the oshi#1373-hardened usage path) and the FFM frequency code, fixing the JNA path.
dbwiddis
force-pushed
the
dedup-windows-centralproc-freq
branch
from
July 14, 2026 04:14
b4568ba to
2b08493
Compare
This was referenced Jul 14, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
First slice of Item 7 (CentralProcessor) in the cross-OS dedup audit, kept small because it also fixes a bug.
queryCurrentFreq(),mapPercentToFreqs(), andqueryMaxFreq()were duplicated across the Windows JNA and FFMCentralProcessortwins. Hoist them into the sharedWindowsCentralProcessorbase, with abstract hooks for the genuinely backend-specific pieces:queryProcessorPerformanceCounters()/queryFrequencyCounters()— theProcessorInformationJNA/FFMPDH/WMI driversqueryNTPower(int)— thePowrProf/FFMCallNtPowerInformationnative callThe Win7 version check moves to a native-free
isWindows7OrGreater()(system-property based, matching the existingisWindows8OrGreater()).Bug fix (JNA per-processor frequency numbering)
The two twins'
mapPercentToFreqshad diverged. The perfmon query returnsinstancesandpercentListas parallel lists, and the shared usage-tick mapperprocessTickData()(hardened during #1373) reads them by position and maps each instance name to a logical processor via the NUMA map. The FFM frequency code matched that; the JNA version (added in #3233) instead indexedpercentListby the computed CPU number and defaulted unmapped NUMA instances to CPU 0.On ordinary single-group systems (instances
"0".."N-1"in order, no NUMA) the two are identical, which is why it went unnoticed. On multi-processor-group / NUMA systems (>64 logical processors, or multi-socket), the JNA path assigned current frequencies to the wrong logical processors (and could clobber CPU 0).This unifies both backends on the positional numbering used by
processTickDataand the FFM code, fixing the JNA path. CHANGELOG updated.Behavior
FFM is unchanged; the JNA per-processor current-frequency numbering is corrected on NUMA/multi-group systems. Both backends now run identical frequency code, so
NativeComparisonTest.processorFrequencies()compares like-for-like.Testing
install: BUILD SUCCESS, 0 tests failedSummary by CodeRabbit