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

Better precompilability #287

Merged
merged 3 commits into from
Jan 7, 2021
Merged

Better precompilability #287

merged 3 commits into from
Jan 7, 2021

Conversation

timholy
Copy link
Member

@timholy timholy commented Dec 31, 2020

These allow more concrete inference and eliminate some weird sources of latency. Very briefly, the first commit compensates for #259 (CC @rofinn, @omus), the last implements the conclusion of discussion in #226 (CC @johnnychen94). All part of a grand scheme to reduce latency (e.g., JuliaGraphics/Gtk.jl#551 (comment)).

After #259, the result of `filename(q)` is not inferrable.
This uses "the kernel trick" to provide a single point where the result
must be inferred, and an `invokelatest` to prevent the abstract signature
from being inferred. It then precompiles the `::String` variant.
Consequently, with respect to latency this should get us back to where
we were before #259.
For reasons I don't fully understand, either the string interpolation or
the error messes up precompilation of `applicable_loaders`. This splits
it out into a separate function and calls it via `invokelatest` to
prevent it from blocking precompilation.
I haven't seen this warning come up in a long time, so I don't think
this code is active. It adds significant latency due to the time
needed to infer the Pkg calls, so let's ditch it.

xref #226.
@coveralls
Copy link

Coverage Status

Coverage increased (+1.4%) to 74.487% when pulling 6f009f0 on teh/better_precompilability into 6fd2c3a on master.

@rofinn
Copy link
Contributor

rofinn commented Jan 1, 2021

The first commit seems fine to me, though I don't fully understand why filename(q) isn't inferrable? Would adding a return type declaration to both the FileIO.filename and FilePathsBase.filename be a better solution as I can imagine this coming up again... and both functions should return a string?

@timholy
Copy link
Member Author

timholy commented Jan 1, 2021

though I don't fully understand why filename(q) isn't inferrable

It's because struct File and struct Stream have no type declaration at all for the filename field, so as far as inference is concerned it's ::Any. As far as I understood the point of #259, it was to allow the filename field to store something besides String...in which case we shouldn't type-assert that an accessor returns a String. Or am I misunderstanding?

@rofinn
Copy link
Contributor

rofinn commented Jan 2, 2021

Oh, I see. It was specifically this change https://github.com/JuliaIO/FileIO.jl/pull/259/files#diff-525588e68b2421901be164965272940effa093de733a3fd36c4b9a4344b8c20cL29 that's causing the problem. The term filename confused me as they mean slightly different things between the two packages. Would parameterizing File and Stream by the type of filename in those structs help julia specialize appropriately?

@timholy
Copy link
Member Author

timholy commented Jan 4, 2021

Parametrization might be undesirable from a latency perspective: any method accepting a MyType{T1,T2} has to be specialized for every combination of T1 and T2, and that adds latency. I think the approach here is probably better because it doesn't increase the amount of specialization except additively in the number of filetype(q) outputs.

@timholy timholy merged commit 1df5033 into master Jan 7, 2021
@timholy timholy deleted the teh/better_precompilability branch January 7, 2021 21:32
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