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

add support for planar images #136

Merged
merged 3 commits into from
Dec 30, 2023
Merged

Conversation

chrstphrbrns
Copy link
Contributor

Fixes #125

@tlnagy
Copy link
Owner

tlnagy commented Dec 6, 2023

What's the rational for the explicit SIMD calls and the dependence on SIMD.jl?

@chrstphrbrns
Copy link
Contributor Author

Better performance

@tlnagy
Copy link
Owner

tlnagy commented Dec 7, 2023

How significant of an impact does it make? Do you have any comparisons with and without?

I prefer to avoid adding dependencies if the difference is relatively minor.

@chrstphrbrns
Copy link
Contributor Author

For one particular 5120x2880 image, it's roughly 130ms vs 80ms (results are similar for compressed and uncompressed versions of the image)

@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (bda25ff) 92.20% compared to head (a920e3e) 92.76%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #136      +/-   ##
==========================================
+ Coverage   92.20%   92.76%   +0.56%     
==========================================
  Files          13       13              
  Lines         924      982      +58     
==========================================
+ Hits          852      911      +59     
+ Misses         72       71       -1     

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

@tlnagy
Copy link
Owner

tlnagy commented Dec 8, 2023

I hacked up a quick script to measure the timings:

Script
module TestMain
    println(@__MODULE__)
    using Pkg
    Pkg.activate(; temp = true)
    Pkg.add("TiffImages")

    a = @elapsed Base.compilecache(Base.identify_package("TiffImages"))

    b = @elapsed using TiffImages

    open("timings.txt", "w") do f
        write(f, join([string(@__MODULE__), a, b], "\t"), "\n")
    end
end

module TestPR136
    println(@__MODULE__)

    using Pkg
    Pkg.activate(; temp = true)
    Pkg.develop("TiffImages")

    a = @elapsed Base.compilecache(Base.identify_package("TiffImages"))

    b = @elapsed using TiffImages

    open("timings.txt", "a") do f
        write(f, join([string(@__MODULE__), a, b], "\t"), "\n")
    end
end

It doesn't quite work for proper using measurements so I switched the module orders to get accurate first timings for the main branch vs this PR.

Which gave the following:

Condition Precompilation (s) Using (s) Using Delta (%)
Main.TestMain 38.981615158 0.869952215 1.0
Main.TestPR136 39.184539139 1.031776947 1.19

So this PR results in approximately the same precompilation time, but almost 20% reduction in speed when loading. I'm not sure if the slight reduction in planar image loading speed is worth a 20% reduction in speed for all users.

EDIT: this is with commit a920e3e

@chrstphrbrns
Copy link
Contributor Author

For me, a 40% per-image win easily justifies an imperceptible (or barely perceptible) increase in one-time load cost

FWIW, the PR for #58 also has a SIMD path, which gives somewhat better improvements for "unusual" bit depths

Twice as fast is nothing to sneeze at

@tlnagy
Copy link
Owner

tlnagy commented Dec 9, 2023

The SIMD path would only accelerate the speed of loading planar images, no? The 20% increase in speed hits everyone, while the increase in speed would only benefit folks loading the planar images (which is likely a small subset of folks). I'm not totally against including SIMD as a dependency, I just want to make sure that it's justified.

@chrstphrbrns
Copy link
Contributor Author

The additional delay is fixed and imperceptible, so I round it down to zero, whereas the optimization is per-image and the benefits therefore unbounded. One might argue that few will process enough images to appreciate the benefits, but then, again, the cost is ~zero

Do you prefer to keep the code simple for now and defer optimization until later?

@tlnagy tlnagy merged commit 6ff5952 into tlnagy:master Dec 30, 2023
9 of 11 checks passed
@tlnagy
Copy link
Owner

tlnagy commented Dec 30, 2023

Sorry for the delay with this, but I decided that the delay (particularly with the advances in Julia v1.10 on that front) should be fine. I think it likely makes sense to let this stew on main for a bit and then tag v0.9 to release it.

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.

Add support for "planar" images
3 participants