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

Write optimization via caching #33

Merged
merged 5 commits into from
Feb 24, 2021

Conversation

IanButterworth
Copy link
Contributor

Gives a slight increase in write speed. Benchmarks over in JuliaIO/FileIO.jl#290

@tlnagy
Copy link
Owner

tlnagy commented Feb 22, 2021

Huh. I guess eager evaluation is a negative here.

@IanButterworth IanButterworth changed the title Use PermutedDimsArray during write Use PermutedDimsArray then collect before sending to write Feb 22, 2021
@IanButterworth
Copy link
Contributor Author

IanButterworth commented Feb 22, 2021

Turns out a lot of the slowness was julia's write handler unfolding the reinterpret array type sent to write.
Doing a collect after the views greatly improves speed.

This PR now takes a 1000x1000 image save time from 70ms to 2ms (results here)

@IanButterworth
Copy link
Contributor Author

@tlnagy Cache now set up, and performs well JuliaIO/FileIO.jl#290 (comment)

Copy link
Owner

@tlnagy tlnagy left a comment

Choose a reason for hiding this comment

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

Hi @IanButterworth, thanks for implementing the cache idea! I think it's unnecessary to build the write-cache on construction of the DenseTaggedImage type because many TIFFs will only be read and never written. I think it's better to construct this on write.

src/types/dense.jl Outdated Show resolved Hide resolved
src/types/dense.jl Show resolved Hide resolved
@codecov-io
Copy link

codecov-io commented Feb 24, 2021

Codecov Report

Merging #33 (2005b07) into master (c974bf9) will decrease coverage by 15.30%.
The diff coverage is 2.54%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master      #33       +/-   ##
===========================================
- Coverage   87.80%   72.49%   -15.31%     
===========================================
  Files          12       13        +1     
  Lines         541      658      +117     
===========================================
+ Hits          475      477        +2     
- Misses         66      181      +115     
Impacted Files Coverage Δ
src/TiffImages.jl 100.00% <ø> (ø)
src/precompile.jl 0.00% <0.00%> (ø)
src/types/dense.jl 97.05% <100.00%> (+0.08%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c974bf9...2005b07. Read the comment docs.

@IanButterworth
Copy link
Contributor Author

Good point! Moved the cache to write time.
Also set up the SnoopCompile tooling to auto-generate and install precomp statements. Results again over in JuliaIO/FileIO.jl#290 (comment)

@IanButterworth IanButterworth changed the title Use PermutedDimsArray then collect before sending to write Write optimization via caching. Add SnoopCompile precompile directives Feb 24, 2021
@tlnagy
Copy link
Owner

tlnagy commented Feb 24, 2021

I think this looks good to be merged from my end!

I'm not sure if the snooping makes a difference in its current form, but perhaps that's because of latency higher up in the stack.

@tlnagy
Copy link
Owner

tlnagy commented Feb 24, 2021

Could we move the Snoopcompile stuff into a separate PR and I can go ahead and merge the cache-collect portion of this?

@IanButterworth
Copy link
Contributor Author

IanButterworth commented Feb 24, 2021

Could we move the Snoopcompile stuff into a separate PR

👍🏻 #35

@tlnagy tlnagy merged commit 5f7e640 into tlnagy:master Feb 24, 2021
@tlnagy
Copy link
Owner

tlnagy commented Feb 24, 2021

Awesome, thanks for figuring this out!

@tlnagy tlnagy changed the title Write optimization via caching. Add SnoopCompile precompile directives Write optimization via caching Feb 24, 2021
@IanButterworth IanButterworth deleted the ib/permuteddimsarray branch February 24, 2021 21:38
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.

3 participants