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

Fix _unsafe_trunc to reduce the likelihood of arbitrary values #291

Merged
merged 1 commit into from
Apr 30, 2024

Conversation

kimikage
Copy link
Collaborator

This fixes (mitigates) #288.

Copy link

codecov bot commented Apr 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.93%. Comparing base (2604b5a) to head (c7081b5).

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #291      +/-   ##
==========================================
- Coverage   97.05%   96.93%   -0.13%     
==========================================
  Files           7        7              
  Lines         782      784       +2     
==========================================
+ Hits          759      760       +1     
- Misses         23       24       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@kimikage
Copy link
Collaborator Author

Benchmark (x86-64 Windows)

As for x86-64, there is no major performance regression.
There was about a 2x regression between v1.6.7 and v1.10.2 for 32-bit values, but this does not seem to be related to this change.

using FixedPointNumbers
using BenchmarkTools

mat_f64 = rand(Float64, 1000, 1000) .* 2 .- 1;
mat_f32 = rand(Float32, 1000, 1000) .* 2 .- 1;

@btime $mat_f64 .% N0f8;
@btime $mat_f64 .% N0f16;
@btime $mat_f64 .% N16f16;
@btime $mat_f32 .% N0f8;
@btime $mat_f32 .% N0f16;
@btime $mat_f32 .% N16f16;
@btime $mat_f64 .% Q0f7;
@btime $mat_f64 .% Q0f15;
@btime $mat_f64 .% Q15f16;
@btime $mat_f32 .% Q0f7;
@btime $mat_f32 .% Q0f15;
@btime $mat_f32 .% Q15f16;
julia> versioninfo()
Julia Version 1.10.2
Commit bd47eca2c8 (2024-03-01 10:14 UTC)
Build Info:
  Official https://julialang.org/ release
Platform Info:
  OS: Windows (x86_64-w64-mingw32)
  CPU: 8 × 11th Gen Intel(R) Core(TM) i7-1165G7 @ 2.80GHz
  WORD_SIZE: 64
  LIBM: libopenlibm
  LLVM: libLLVM-15.0.7 (ORCJIT, tigerlake)
Threads: 1 default, 0 interactive, 1 GC (on 8 virtual cores)

before

julia> @btime $mat_f64 .% N0f8;
  214.200 μs (2 allocations: 976.67 KiB)

julia> @btime $mat_f64 .% N0f16;
  428.900 μs (2 allocations: 1.91 MiB)

julia> @btime $mat_f64 .% N16f16;
  1.118 ms (2 allocations: 3.81 MiB)

julia> @btime $mat_f32 .% N0f8;
  119.000 μs (2 allocations: 976.67 KiB)

julia> @btime $mat_f32 .% N0f16;
  327.400 μs (2 allocations: 1.91 MiB)

julia> @btime $mat_f32 .% N16f16;
  1.095 ms (2 allocations: 3.81 MiB)

julia> @btime $mat_f64 .% Q0f7;
  216.300 μs (2 allocations: 976.67 KiB)

julia> @btime $mat_f64 .% Q0f15;
  436.200 μs (2 allocations: 1.91 MiB)

julia> @btime $mat_f64 .% Q15f16;
  1.123 ms (2 allocations: 3.81 MiB)

julia> @btime $mat_f32 .% Q0f7;
  116.900 μs (2 allocations: 976.67 KiB)

julia> @btime $mat_f32 .% Q0f15;
  288.300 μs (2 allocations: 1.91 MiB)

julia> @btime $mat_f32 .% Q15f16;
  1.122 ms (2 allocations: 3.81 MiB)

after

julia> @btime $mat_f64 .% N0f8;
  202.500 μs (2 allocations: 976.67 KiB)

julia> @btime $mat_f64 .% N0f16;
  415.000 μs (2 allocations: 1.91 MiB)

julia> @btime $mat_f64 .% N16f16;
  1.403 ms (2 allocations: 3.81 MiB)

julia> @btime $mat_f32 .% N0f8;
  115.600 μs (2 allocations: 976.67 KiB)

julia> @btime $mat_f32 .% N0f16;
  322.200 μs (2 allocations: 1.91 MiB)

julia> @btime $mat_f32 .% N16f16;
  1.383 ms (2 allocations: 3.81 MiB)

julia> @btime $mat_f64 .% Q0f7;
  205.500 μs (2 allocations: 976.67 KiB)

julia> @btime $mat_f64 .% Q0f15;
  417.700 μs (2 allocations: 1.91 MiB)

julia> @btime $mat_f64 .% Q15f16;
  1.120 ms (2 allocations: 3.81 MiB)

julia> @btime $mat_f32 .% Q0f7;
  116.200 μs (2 allocations: 976.67 KiB)

julia> @btime $mat_f32 .% Q0f15;
  329.200 μs (2 allocations: 1.91 MiB)

julia> @btime $mat_f32 .% Q15f16;
  1.175 ms (2 allocations: 3.81 MiB)

@kimikage
Copy link
Collaborator Author

We are having problems testing ColorTypes.jl. So, I am going to backport this to v0.8.
However, since v0.8.4, the files have been rewritten a lot. Any thoughts?

@kimikage
Copy link
Collaborator Author

In any case, this PR is a bug fix, so I will merge it first.
Theoretically, there should be no major performance regression, and even if there is, it should be tackled in another PR.

@kimikage kimikage merged commit 3a4346c into JuliaMath:master Apr 30, 2024
18 of 22 checks passed
@kimikage kimikage deleted the issue288 branch April 30, 2024 01:24
kimikage added a commit to kimikage/FixedPointNumbers.jl that referenced this pull request Apr 30, 2024
kimikage added a commit to kimikage/FixedPointNumbers.jl that referenced this pull request Apr 30, 2024
kimikage added a commit to kimikage/FixedPointNumbers.jl that referenced this pull request May 1, 2024
kimikage added a commit to kimikage/FixedPointNumbers.jl that referenced this pull request May 1, 2024
kimikage added a commit to kimikage/FixedPointNumbers.jl that referenced this pull request May 1, 2024
kimikage added a commit to kimikage/FixedPointNumbers.jl that referenced this pull request May 1, 2024
kimikage added a commit to kimikage/FixedPointNumbers.jl that referenced this pull request May 1, 2024
kimikage added a commit to kimikage/FixedPointNumbers.jl that referenced this pull request May 1, 2024
kimikage added a commit to kimikage/FixedPointNumbers.jl that referenced this pull request May 13, 2024
kimikage added a commit to kimikage/FixedPointNumbers.jl that referenced this pull request May 13, 2024
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.

1 participant