Skip to content

Fix #563: filesizeformat with proper accuracy#568

Merged
Kijewski merged 1 commit intoaskama-rs:masterfrom
seijikun:mr-filter-humansize
Aug 13, 2025
Merged

Fix #563: filesizeformat with proper accuracy#568
Kijewski merged 1 commit intoaskama-rs:masterfrom
seijikun:mr-filter-humansize

Conversation

@seijikun
Copy link
Copy Markdown
Contributor

@seijikun seijikun commented Aug 12, 2025

I refactored the current humansize / filesize implementation from f32 to u128 and added more unit-tests.
Additionally, the new refactoring now allows changing the requested precision (not yet plumbed on the code generation side).

u128 is required to handle the SI Prefixes:

    ((AsciiChar::new(b'Z'), 1e-19), 1e24),
    ((AsciiChar::new(b'Y'), 1e-22), 1e27),
    ((AsciiChar::new(b'R'), 1e-25), 1e30),
    ((AsciiChar::new(b'Q'), 1e-28), 1e33),

Getting something as performant as the original version is quite challenging. Here are my benchmark results for the current solution:

Benchmarks (time for 250000000 format & display invocations):

  • Original (f32): 1.9542559s
  • This MR, but with u64: 2.638588188s
  • This MR (u128): 3.29419516s

@seijikun seijikun force-pushed the mr-filter-humansize branch 2 times, most recently from f989c7c to 92d705c Compare August 12, 2025 13:37
Kijewski
Kijewski reviewed Aug 12, 2025
View reviewed changes
Comment thread askama/src/filters/humansize.rs Outdated
Kijewski
Kijewski reviewed Aug 12, 2025
View reviewed changes
Comment thread askama/src/filters/humansize.rs Outdated
Kijewski
Kijewski reviewed Aug 12, 2025
View reviewed changes
Comment thread askama/src/filters/humansize.rs Outdated
@Kijewski
Copy link
Copy Markdown
Member

Kijewski commented Aug 12, 2025

Working on u128 would probably be faster than working on f32. I guess formatting directly into the output stream is what makes the new code slower.

@GuillaumeGomez
Copy link
Copy Markdown
Collaborator

GuillaumeGomez commented Aug 12, 2025

Working on u128 would probably be faster than working on f32. I guess formatting directly into the output stream is what makes the new code slower.

Nope, if floats and integers are of the same size, the floats are always slower, however floats are always faster than bigger integers (so u64 > f32 > u32).

Also, what kind of numbers would we need above 18 exabytes (u64::MAX)? O.o (I think u64 is more than enough)

@seijikun seijikun force-pushed the mr-filter-humansize branch from 92d705c to 17ef961 Compare August 12, 2025 13:54
@seijikun
Copy link
Copy Markdown
Contributor Author

seijikun commented Aug 12, 2025 •
edited
Loading

I knew binary search was probably slower than linear search in this case, but I didn't think it was noticeable...
But holy cow...

Now I'm at 1.832858975s with u128 - faster than the current implementation :)

@seijikun
Copy link
Copy Markdown
Contributor Author

seijikun commented Aug 12, 2025

Also, what kind of numbers would we need above 18 exabytes (u64::MAX)? O.o (I think u64 is more than enough)

I tried to port it and couldn't represent the previous functionality, because the set of SI_PREFIXES in the current implementation overflowed the u64, so it was either ... delete them or u128 🤷

GuillaumeGomez
GuillaumeGomez reviewed Aug 12, 2025
View reviewed changes
Comment thread askama/src/filters/humansize.rs Outdated
@seijikun seijikun force-pushed the mr-filter-humansize branch 2 times, most recently from 3a563a9 to 7b1e266 Compare August 12, 2025 14:46
@seijikun
Copy link
Copy Markdown
Contributor Author

seijikun commented Aug 12, 2025

Just some final formatting improvements.
Could one of you plumb the precision with an optional parameter, please?
I wasn't able to get that to work.

@seijikun seijikun force-pushed the mr-filter-humansize branch from 7b1e266 to 39278c9 Compare August 12, 2025 14:48
@GuillaumeGomez
Copy link
Copy Markdown
Collaborator

GuillaumeGomez commented Aug 12, 2025

Might be better if you did so you can learn: what did you try?

There are a few things needed to make it work: the filesizeformat function needs to take all args and the optional ones need to use Option. Then in askama_derive, depending on the arguments, you call the function with Some or None for the optional args.

@seijikun seijikun force-pushed the mr-filter-humansize branch 2 times, most recently from dcedaf9 to 74474ff Compare August 12, 2025 15:35
@seijikun
Copy link
Copy Markdown
Contributor Author

seijikun commented Aug 12, 2025

Might be better if you did so you can learn: what did you try?

There are a few things needed to make it work: the filesizeformat function needs to take all args and the optional ones need to use Option. Then in askama_derive, depending on the arguments, you call the function with Some or None for the optional args.

I had trouble understanding how the new argument parsing stuff works (how to fill the default_value field). But I think I got it to work now

github-advanced-security[bot]
github-advanced-security AI found potential problems Aug 12, 2025
View reviewed changes
Comment thread fuzzing/fuzz/src/filters.rs Fixed
@seijikun seijikun force-pushed the mr-filter-humansize branch from 74474ff to c8740f1 Compare August 12, 2025 20:04
Kijewski
Kijewski reviewed Aug 12, 2025
View reviewed changes
Comment thread askama/src/filters/humansize.rs Outdated
@seijikun seijikun force-pushed the mr-filter-humansize branch 2 times, most recently from 3aed30a to 1706904 Compare August 12, 2025 20:38
@seijikun
Copy link
Copy Markdown
Contributor Author

seijikun commented Aug 12, 2025 •
edited
Loading

One more improvement. Instead of just stupidly limiting the precision to the completely arbitrary number of 15 due to numerical problems, I sat down and did some thinking:

Allowing a larger precision than the chosen si-prefix's exponent doesn't make any sense, because except for the super-edgecase of more than 1000 quetta bytes, the additional digits will all just be 0s, which are stripped anyway.
This way, I can guarantee that this is always true: scale <= unit.1.
Furthermore, both numbers are 10-based, so this is also always true: (unit.1 % scale) == 0.
This allowed me to change the fractional calculation from:

let mut fractional = remainder.saturating_mul(scale) / unit.1;

to:

let mut fractional = remainder / (unit.1 / scale);

which effectively avoids overflow due to the multiplication, while allowing much higher precision.
(The affect on runtime performance is negligible - to my own surprise)

GuillaumeGomez
GuillaumeGomez reviewed Aug 12, 2025
View reviewed changes
Comment thread askama/src/filters/humansize.rs Outdated
GuillaumeGomez
GuillaumeGomez reviewed Aug 12, 2025
View reviewed changes
Comment thread askama/src/filters/humansize.rs Outdated
GuillaumeGomez
GuillaumeGomez reviewed Aug 12, 2025
View reviewed changes
Comment thread askama/src/filters/humansize.rs Outdated
Kijewski
Kijewski reviewed Aug 12, 2025
View reviewed changes
Comment thread askama/src/filters/humansize.rs Outdated
GuillaumeGomez
GuillaumeGomez reviewed Aug 12, 2025
View reviewed changes
Comment thread askama/src/filters/humansize.rs Outdated
Comment on lines +81 to +83
' '.write_into(dest, values)?;
unit.0.write_into(dest, values)?;
'B'.write_into(dest, values)?;
Copy link
Copy Markdown
Collaborator

@GuillaumeGomez GuillaumeGomez Aug 12, 2025

Choose a reason for hiding this comment

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

Suggested change
' '.write_into(dest, values)?;
unit.0.write_into(dest, values)?;
'B'.write_into(dest, values)?;
write!(dest, " {}B", unit..0)?;

Copy link
Copy Markdown
Contributor Author

@seijikun seijikun Aug 12, 2025

Choose a reason for hiding this comment

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

I can do this change, but I would strongly advise against it.
This will outright destroy performance.
(I came from there 😅)

Copy link
Copy Markdown
Collaborator

@GuillaumeGomez GuillaumeGomez Aug 12, 2025

Choose a reason for hiding this comment

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

Wait really? I'm surprised, generally the I/O calls are slow so we try to do as little of them as possible.

Copy link
Copy Markdown
Contributor Author

@seijikun seijikun Aug 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

The std calls are SUPER slow. That's why I use askama's FastWritable::write_into() everywhere.
I can do 100s of calls of FastWritable::write_into() and will still be faster than one write!().
Inlining sure can work wonders :)

GuillaumeGomez
GuillaumeGomez reviewed Aug 12, 2025
View reviewed changes
Comment thread askama/src/filters/humansize.rs Outdated
@seijikun seijikun force-pushed the mr-filter-humansize branch from 1706904 to 968a1d4 Compare August 12, 2025 21:47
@seijikun seijikun force-pushed the mr-filter-humansize branch from 968a1d4 to 767ae9e Compare August 13, 2025 09:57
@GuillaumeGomez
Copy link
Copy Markdown
Collaborator

GuillaumeGomez commented Aug 13, 2025

Looks all good to me, great work!

Kijewski
Kijewski approved these changes Aug 13, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@Kijewski Kijewski left a comment

Choose a reason for hiding this comment

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

Thank you!

@Kijewski Kijewski merged commit 59a9820 into askama-rs:master Aug 13, 2025
42 checks passed
@Kijewski Kijewski mentioned this pull request Aug 13, 2025
Closed
@seijikun
Copy link
Copy Markdown
Contributor Author

seijikun commented Aug 13, 2025

Thank you two!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Kijewski Kijewski Kijewski approved these changes

@GuillaumeGomez GuillaumeGomez GuillaumeGomez approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@seijikun @Kijewski @GuillaumeGomez @github-advanced-security