Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

#1061 Fix transferTime in WindowsDisks by using 1-%Idle #1063

Merged
merged 2 commits into from Dec 17, 2019

Conversation

Space2Man
Copy link
Contributor

Hi,

first try at pull request. It compiles fine and uses 1 - %IdleTime to set the TransferTime.
Can you test this? I switched my system to Linux and don't have a test system at hand right now.

Thanks, Space

@Space2Man Space2Man changed the title Fix transferTime in WindowsDisks by using 1-%Idle #1061 Fix transferTime in WindowsDisks by using 1-%Idle Dec 17, 2019
@coveralls
Copy link

coveralls commented Dec 17, 2019

Coverage Status

Coverage remained the same at 81.825% when pulling bbe1c58 on Space2Man:feature/WindowsTransfertime into 7c809b6 on oshi:master.

@codecov-io
Copy link

codecov-io commented Dec 17, 2019

Codecov Report

Merging #1063 into master will decrease coverage by 0.08%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##             master   #1063      +/-   ##
===========================================
- Coverage     77.58%   77.5%   -0.09%     
  Complexity        2       2              
===========================================
  Files            26      26              
  Lines          1249    1249              
  Branches        159     159              
===========================================
- Hits            969     968       -1     
- Misses          205     206       +1     
  Partials         75      75
Impacted Files Coverage Δ Complexity Δ
.../src/main/java/oshi/hardware/CentralProcessor.java 84.12% <0%> (-1.59%) 0% <0%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7c809b6...bbe1c58. Read the comment docs.

@dbwiddis
Copy link
Member

It seems like it would work, except the default doesn't make sense (using 0 as default idle time means the disk is running 100%, it should be the opposite). But in my testing I can't see why it's wrong the other way. The idle and disk time always sum up to the same value, so the original method should work.

Is it WMI or performance counters giving you the odd readings in the old code?

@Space2Man
Copy link
Contributor Author

How can I find out which method is used? What logging do I need to enable?

@dbwiddis
Copy link
Member

Doesn't look like it's logged. You'd have to instrument the classes yourself. I'll try it tomorrow... manual WMI queries look sane, so it may be the performance counters don't read like I think they read

@Space2Man
Copy link
Contributor Author

I tried to do some further investigations but I failed. On the system where I see the transferTime == timeStamp in my app both Get-Counter and Get-CIMInstance return value of 0 for PercentDiskTime and 100 for PercentIdleTime in PowerShell ... how do you get the PERCENTDISKTIME? Is it maybe mapped incorrectly in JNA?

I am quite lost but I am not that much into Windows :)

@Space2Man
Copy link
Contributor Author

But I think the reason to use 1 - % Idle Time was something like this:

http://support.demandtech.com/index.php?pg=kb.page&id=47

@dbwiddis
Copy link
Member

The "fallback" for percent disk time comes from WMI in the Win32_PerfRawData_PerfDisk_PhysicalDisk class. You can try this on your commandline:
wmic path Win32_PerfRawData_PerfDisk_PhysicalDisk get PercentDiskTime,PercentIdleTime

You'll notice they add up to the same number consistently. The primary choice of data, however, is from the performance counter \\PhysicalDisk(0 C:)\% Disk Time (or whichever drive letter is the instance) which you can look at the processed data with the Perfmon.exe program, and again see that idle and disk time add up to 100%. However, you can't see the raw numbers that go into the counter... that's possibly what OSHI is returning to you, however.

Anyway, the 1-%Idle is probably a good thing to use and I'll accept this PR, but please look at your default as noted in my first comment.

@dbwiddis
Copy link
Member

I just tested the disk time vs. idle time on my hard drive, over 20 seconds, got:

9496222 94963206
9496263 94964174
...
9497756 94980229
9497787 94981199

The numbers sum up to elapsed milliseconds (e.g., each row sum is about 1000 more than the previous). So this confirms subtracting idle time from elapsed time should give the correct result.

@Space2Man
Copy link
Contributor Author

Ok, I set default to stats.timeStamp. Then transferTime should be set to 0 if nothing is returned.

@dbwiddis
Copy link
Member

Looks good, merging. If you want to submit the same PR against the 3.13.X branch I can update that one too.

@dbwiddis dbwiddis merged commit 97da0a4 into oshi:master Dec 17, 2019
@Space2Man Space2Man deleted the feature/WindowsTransfertime branch February 8, 2020 22:52
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.

None yet

4 participants