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

Setup header header struct. #5

Closed
wants to merge 5 commits into from

Conversation

james-bird
Copy link
Contributor

Very slow to construct/read into. I think it's to do with the use of static arrays, but haven't found a fix yet. Not yet tested the validity of data.

@stillyslalom
Copy link
Owner

Current header compilation time:

julia> t = time(); CineFiles.CineHeader("test/data/8bpp.cine"); time() - t
0.07999992370605469

After this PR:

julia> t = time(); CineFiles.CineHeader("test/data/8bpp.cine"); time() - t
46.08899998664856

Yeah, that's pretty painful. I think the main culprit is Description::SVector{4096,Char}, since compilation time drops to ~7.5 seconds if I comment that line out of the struct definition. Julia's compiler has issues with large tuples (JuliaLang/julia#35619), and the solution probably involves restructuring the setup header into a non-isbits type with Strings replacing each NTuple{N,Char}. I'll take a shot at it.

@james-bird
Copy link
Contributor Author

The issue could be related to the issue discussed here:
https://discourse.julialang.org/t/long-parameter-for-type-seems-very-slow/63290

@james-bird
Copy link
Contributor Author

I have made some changes which make the reading much more tolerable. The data looks sensible too, based on my limited testing.

Copy link
Owner

@stillyslalom stillyslalom left a comment

Choose a reason for hiding this comment

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

Looks mostly good - I've also considered iteratively reading the setup struct as a OrderedDict{Symbol,Any}, which might fractionally increase runtime, but would sharply decrease compilation time. That would also make it easier to handle the branches necessary to read older Setup header formats.

src/CineFiles.jl Show resolved Hide resolved
src/CineFiles.jl Outdated Show resolved Hide resolved
src/CineFiles.jl Outdated Show resolved Hide resolved
@james-bird james-bird closed this Jan 20, 2022
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.

2 participants