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

Rewrite Row_printTime() with various improvements #1325

Merged
merged 1 commit into from
Nov 10, 2023

Conversation

Explorer09
Copy link
Contributor

  • Change variable types in the function from signed to unsigned.
  • Change variable naming for consistency: totalMinutes, totalHours, totalDays for total values and minutes, hours, days, etc. for remainder values.
  • Use unsigned long long type for years value to prevent a potential overflow.
  • The time units are now evaluated from small to large, in the hopes that small values would print faster. (There is no performance test for this claim.)
  • The days unit is now printed when total hours is at least 24. (Issue Feature request: formats for long cpu times #1317) There is room in the new code to revert to the old threshold (totalHours < 2400).

@Explorer09
Copy link
Contributor Author

Examples of all formats:

 0:00.00 - 59:59.99
 1h00:00 - 23h59:59
1d 0h 0m - 9d23h59m
  10d 0h -  364d23h
  1y  0d - 999y364d
   1000y - 9999999y
eternity

Copy link
Member

@BenBE BenBE left a comment

Choose a reason for hiding this comment

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

Some suggestions. No perf testing done re mod/div vs. div & mul+sub.

How much of this code could we potentially re-use for #1288 time printing

Row.c Outdated Show resolved Hide resolved
Row.c Outdated Show resolved Hide resolved
Row.c Outdated Show resolved Hide resolved
Row.c Outdated Show resolved Hide resolved
Row.c Outdated Show resolved Hide resolved
Row.c Outdated Show resolved Hide resolved
@Explorer09
Copy link
Contributor Author

With zero padding the formats will now look like this:

 0:00.00 - 59:59.99
 1h00:00 - 23h59:59
1d00h00m - 9d23h59m
  10d00h -  364d23h
  1y000d - 999y364d
   1000y - 9999999y
eternity - eternity

How much of this code could we potentially re-use for #1288 time printing

Unfortunately I don't think there is potential for code reuse. The time factoring logic would be slightly different between two functions, but at least we can keep variable naming consistent.

@BenBE BenBE added the enhancement Extension or improvement to existing feature label Nov 9, 2023
@BenBE BenBE added this to the 3.3.0 milestone Nov 9, 2023
* Change variable types of time values from signed to unsigned.
* Change variable naming for consistency: `totalMinutes`, `totalHours`,
  `totalDays` for total values and `minutes`, `hours`, `days`, etc. for
  remainder values. (`years` remains an exception as there is no larger
  unit than years)
* Use `unsigned long long` type for `years` value to prevent a
  potential overflow.
* The time units are now evaluated from small to large, in the hopes
  that small values would print faster. (There is no performance test
  for this claim.)
* The days unit is now printed when `totalHours` is at least 24.
  (Issue htop-dev#1317) There is room in the new code to revert to the
  old threshold (totalHours < 2400).

Signed-off-by: Kang-Che Sung <explorer09@gmail.com>
Copy link
Member

@natoscott natoscott left a comment

Choose a reason for hiding this comment

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

Looks good, though I think that final branch and 'eternity' string is a bit over the top :P (could've just gone up to 9999999y and used that for anything above 1million years).

@natoscott natoscott merged commit b2029fb into htop-dev:main Nov 10, 2023
12 checks passed
@Explorer09
Copy link
Contributor Author

@natoscott Just to say the "eternity" idea was not mine. It was there since commit ec809b7. Credit @BenBE for that one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Extension or improvement to existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants