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

arcstat: Fix integer division with python3 #12603

Merged
merged 1 commit into from
Oct 8, 2021
Merged

Conversation

Kayvlim
Copy link
Contributor

@Kayvlim Kayvlim commented Oct 3, 2021

Motivation and Context

The arcstat script requests compatibility with python2 and python3.

When using python2, int/int yields an int and calculations are correct.

PEP 238 modified this behavior in python3: int/int yields a float, and results in erroneous output when running under options like arcstat -a 3.
"Erroneous output" includes cases where hits + miss < reads, or hit% is 1.0K instead of 100, or hit%=99 and 0 misses.

Description

This PR replaces instances of / with //, yielding the expected result in both versions of Python.

How Has This Been Tested?

Since this a utility script, it was tested on my machine by running the utility and verifying that its results make sense.
I also inserted debug prints to peek the internal values, but removed them on the commit.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Performance enhancement (non-breaking change which improves efficiency)
  • Code cleanup (non-breaking change which makes code smaller or more readable)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Library ABI change (libzfs, libzfs_core, libnvpair, libuutil and libzfsbootenv)
  • Documentation (a change to man pages or other documentation)

Checklist:

cmd/arcstat/arcstat.in Show resolved Hide resolved
@behlendorf behlendorf requested a review from a user October 4, 2021 19:05
@behlendorf behlendorf added the Status: Code Review Needed Ready for review and testing label Oct 4, 2021
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Making this behave consistently with both Python 2 and 3 will be good, but not all of these values need to be integers. There's a reasonable justification that you can't have half a hit or read half a byte, but half a percent should be perfectly valid.

prettynum() also divides by the scale, and has an explicit format string for floating point values. It seems like whether all integers or some floating point values are preferred, that function needs to be adjusted one way or another as well.

@ghost
Copy link

ghost commented Oct 5, 2021

To clarify, prettynum() looks like it is likely a source of errors, and restoring integer division may merely be working around it.

@Kayvlim
Copy link
Contributor Author

Kayvlim commented Oct 6, 2021

@freqlabs

Making this behave consistently with both Python 2 and 3 will be good, but not all of these values need to be integers. There's a reasonable justification that you can't have half a hit or read half a byte, but half a percent should be perfectly valid.

I agree with you that not all of these values need to be integers. In particular, I really don't like to write hit%=100 and miss=0 when there are 1000 hits and 1 miss.

The idea for this PR was only to fix the wrong values when running under python3 compared to what it writes under python2; under python2 it does not (and will not, as I will detail below) write half a percent either.

prettynum() also divides by the scale, and has an explicit format string for floating point values. It seems like whether all integers or some floating point values are preferred, that function needs to be adjusted one way or another as well.

What you are requesting is a bit of an overhaul - one I agree with but can't write anytime soon - because prettynum() isn't expecting floating point values for percentages. There's an explicit instruction in line 249 to turn any number between 0 and 1 into 0 (so there will never be half a percent), and there's a check in line 256 for the special case where the number isn't divided by the scale, which is the case for percentages. I will need to understand what was the rationale behind these lines before changing them, but they certainly need to go.

I considered printing "miss: <1" and "hit%: >99" for these cases, but print_values() expects numbers only, as does prettynum(), so I can't format it this way. One of my ideas for the future includes giving each column its own formatter.

To clarify, prettynum() looks like it is likely a source of errors

Yep!

and restoring integer division may merely be working around it.

The numbers seem to be correct (to +/- 1 unit) after restoring integer division. We simply can do better in how we present them.

I'd rather fix this immediate python3 issue in this PR and open an issue for the other changes I intend to make before I take the time to write the code. Is this acceptable?

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Sure, this is ok for now.

@behlendorf behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Oct 6, 2021
The arcstat script requests compatibility with python2 and python3, but
PEP 238 modified the / operator and results in erroneous output when
run under python3.

This commit replaces instances of / with //, yielding the expected
result in both versions of Python.

Signed-off-by: Valmiky Arquissandas <foss@kayvlim.com>
@jwk404 jwk404 merged commit 2d02bba into openzfs:master Oct 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Accepted Ready to integrate (reviewed, tested)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants