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

[loading]: Rework preferences loading #38044

Merged
merged 1 commit into from
Oct 27, 2020
Merged

[loading]: Rework preferences loading #38044

merged 1 commit into from
Oct 27, 2020

Conversation

staticfloat
Copy link
Member

Implements the Preferences loading framework as outlined in 0. The
most drastic change is that the list of compile-time preferences is no
longer sequestered within its own dictionary, but is instead
autodetected at compile-time and communicated back to the compiler.
This list of compile-time preferences is now embedded as an array of
strings that the loader must load, then index into the preferences
dictionary with that list to check the preferences hash.

In a somewhat bizarre turn of events, because we want the .ji filename
to incorporate the preferences hash, and because we can't know how to
generate the hash until after we've precompiled, I had to move the .ji
filename generation step to after we precompile the .ji file.

@staticfloat staticfloat force-pushed the sf/preferences2 branch 2 times, most recently from 71a65e3 to f5552f0 Compare October 15, 2020 20:07
@staticfloat
Copy link
Member Author

@Keno analyzegc is angry at me, I believe because prefs_list could be GC'ed:

https://github.com/JuliaLang/julia/pull/38044/files#diff-f9e2818fcf6de95dcaa432fde9cea4e28109ddef7bc2fbc11849f4d6565b68d8R1143

I don't think I can use JL_GC_PUSH*() here because of the usage of prefs_list in two different scopes, unless perhaps I can JL_GC_PUSH() while prefs_list is still NULL. Is that the case?

@Keno
Copy link
Member

Keno commented Oct 15, 2020

Yes, the GC push is for a stack location, not a value, so you can initialize the location to null and then push it. The error is correct. You need to root that value

base/loading.jl Outdated Show resolved Hide resolved
base/loading.jl Outdated Show resolved Hide resolved
base/loading.jl Outdated Show resolved Hide resolved
base/loading.jl Outdated Show resolved Hide resolved
@staticfloat
Copy link
Member Author

@vtjnash you are probably interested in this from the perspective of communication happening between the code being compiled and the compiler.

In short; when we call @load_preference("backend") in the code being compiled, Preferences calls jl_generating_output(), notes that "backend" was requested during compilation, and pushes that name onto Base. COMPILETIME_PREFERENCES[uuid]. At the end of compilation, as part of the header of the .ji file, the list of accessed top-level preferences gets serialized along with the hash of all preferences listed. This allows loading to attend only to the preferences that are included in that header list, allowing us to store compile-time preferences and non-compile-time preferences in the same dict, and not requiring the user to specifically denote which are or are not compile-time preferences. In effect, the preferences are known as compile-time purely by the virtue of them being accessed at compile-time.

This should not make it any harder or easier to disallow parsing of TOML files off of the disk at compile time. Most likely what we'd do for that is change Base.get_preferences() to look up in a global Dict whether we already have a mapping for that UUID, and if so, use it instead of trying to hit the disk. Then, if we're launching a remote compile job, we are responsible for loading preferences for all UUIDs being compiled beforehand and transmitting that to the remote compile job, such that it can pre-fill the global Dict.

@vtjnash
Copy link
Member

vtjnash commented Oct 19, 2020

Yep, I was somewhat following along and that sounded like what I expected

Implements the `Preferences` loading framework as outlined in [0]. The
most drastic change is that the list of compile-time preferences is no
longer sequestered within its own dictionary, but is instead
autodetected at compile-time and communicated back to the compiler.
This list of compile-time preferences is now embedded as an array of
strings that the loader must load, then index into the preferences
dictionary with that list to check the preferences hash.

In a somewhat bizarre turn of events, because we want the `.ji` filename
to incorporate the preferences hash, and because we can't know how to
generate the hash until after we've precompiled, I had to move the `.ji`
filename generation step to _after_ we precompile the `.ji` file.

[0]: #37791 (comment)
@staticfloat
Copy link
Member Author

Great, I will merge this when green then.

@vtjnash
Copy link
Member

vtjnash commented Oct 19, 2020

Ah, wait, I don’t understand how you can look up something correctly by content hash with this scheme

@staticfloat
Copy link
Member Author

Ah, wait, I don’t understand how you can look up something correctly by content hash with this scheme

I'm not sure what you mean; what is content-hashed?

@staticfloat
Copy link
Member Author

@vtjnash Was your comment related to this: #38105

@vtjnash
Copy link
Member

vtjnash commented Oct 26, 2020

Yeah, that's where I noticed that lookup won't work anymore. That function maybe should be replaced with randstring for clarity?

@staticfloat
Copy link
Member Author

Which function should be randstring? In general we want deterministic results so that we don't have to recompile a .ji file after flip-flopping preferences.

@staticfloat
Copy link
Member Author

Spoke with Jameson, he's okay with this overall, so I'm going to merge.

@staticfloat staticfloat merged commit bd304ef into master Oct 27, 2020
@staticfloat staticfloat deleted the sf/preferences2 branch October 27, 2020 17:46
staticfloat added a commit that referenced this pull request Nov 15, 2024
When preferences were first added, we originally did not have any
preference merging across load path [0].  We later added that [1], but
retained the requirement that for each individual element in the load
path, preferences must have an entry in `Project.toml` listing the
relevant package.  This was partly on purpose (immediately binding the
name to the UUID prevents confusion when a UUID<->name binding is not
readily apparent) and partly just inheriting the way things worked back
when preferences was written with just a single Project.toml in mind.

This PR breaks this assumption to specifically allow an entry in the
Julia load path that contains only a single `LocalPreferences.toml`
file that sets preferences for packages that may or may not be in the
current environment stack.  The usecase and desire for this is
well-motivated in [2], but basically boils down to "system admin
provided preferences that should be used globally".  In fact, one such
convenient method that now works is to drop a `LocalPreferences.toml`
file in the `stdlib` directory of your Julia distribution, as that is
almost always a part of a Julia process's load path.

[0] #37595
[1] #38044
[2] JuliaPackaging/Preferences.jl#33
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.

4 participants