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

Track a distinct compile cache for julia-debug. #33669

Closed
wants to merge 1 commit into from

Conversation

twadleigh
Copy link
Contributor

Addresses #33668.

@@ -633,7 +633,7 @@ end

cache_file_entry(pkg::PkgId) = joinpath(
"compiled",
"v$(VERSION.major).$(VERSION.minor)",
"v$(VERSION.major).$(VERSION.minor)" * (JLOptions().debug_level > 1 ? "-debug" : ""),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

JLOptions().debug_level > 1 is my best guess for the most robust way to test for julia-debug, but others much more knowledgeable may have a different opinion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Specifically, I chose to prefer it to something like:

endswith(basename(unsafe_string(Base.JLOptions().julia_bin)),"-debug")

so as to be insensitive to a name change of the executable.

Copy link
Member

Choose a reason for hiding this comment

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

JLOptions().debug_level corresponds to the -g option and not whether the binary was build with debug information embedded

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the test should be ccall(:jl_is_debugbuild, Cint, ()) > 0.

@tkf
Copy link
Member

tkf commented Oct 24, 2019

FYI an alternative implementation may be to include JLOptions().julia_bin in package_slug. I implemented this in #29914:

julia/base/loading.jl

Lines 136 to 141 in fa5f1b1

function package_slug(uuid::UUID, p::Int=5)
crc = _crc32c(uuid)
crc = _crc32c(unsafe_string(JLOptions().image_file), crc)
crc = _crc32c(unsafe_string(JLOptions().julia_bin), crc)
return slug(crc, p)
end

A major advantage might be that using JLOptions().julia_bin (and .image_file) would support differentiating arbitrary binaries which are not precompile-cache -compatible. A disadvantage is that renaming the binary would create a different set of precompile-cache files. However, this can be avoided by symlinking julia/julia-debug binary.

@twadleigh
Copy link
Contributor Author

@tkf, different system images similarly clobber the compile cache? Tweaking the slug is one way to go. You might also consider qualifying the path, like I did here. It might be a more palatable solution to the maintainers, as it is guaranteed not to interact with the existing implementation.

@tkf
Copy link
Member

tkf commented Oct 24, 2019

different system images similarly clobber the compile cache?

Yes, especially because you can use PackageCompiler to bundle a different set of packages in the system image.

You might also consider qualifying the path

Yes, using suffix as you do in this PR does have a benefit like refreshing precompilation cache is as easy as rm -r ~/.julia/compiled/v1.x-debug and does not slow down normal julia usage.

@twadleigh
Copy link
Contributor Author

@tkf, I think I'll attempt a modest extension of this solution that would, for instance, keep the caches distinct for distinct build images.

@tkf
Copy link
Member

tkf commented Oct 25, 2019

@twadleigh I actually don't know how much core devs want to play with it. So maybe just wait for a response? I don't want to request a feature that would be rejected by core devs :)

@twadleigh
Copy link
Contributor Author

@tkf, fair enough. I'll propose an extension in a separate PR only if/when this is merged.

@tkf
Copy link
Member

tkf commented Oct 29, 2019

FYI, @KristofferC just merged PR #29914. I'm not sure @KristofferC was aware of this patch. Sorry, I should have mentioned there that an alternative patch was suggested in this PR...

@twadleigh
Copy link
Contributor Author

@tkf, I'll close this. If it turns out master still doesn't have the functionality I'm looking for, I'll reopen.

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