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

Move Libdl into Base #35628

Merged
merged 1 commit into from
Jun 30, 2020
Merged

Move Libdl into Base #35628

merged 1 commit into from
Jun 30, 2020

Conversation

staticfloat
Copy link
Member

We need to dlopen() things in Base a lot more due to using JLL
packages within Base and the standard library. We move Libdl into
Base.Libc, but continue to carry a shell Libdl stdlib that
re-exports all relevant pieces of functionality, for
backwards-compatibility.

@staticfloat staticfloat added the building Build system, or building Julia or its dependencies label Apr 28, 2020
base/Base.jl Outdated Show resolved Hide resolved
@JeffBezanson
Copy link
Member

Could this be done more surgically --- i.e. move just dlopen to Base and leave more of the code in stdlib/Libdl/src/Libdl.jl?

@staticfloat
Copy link
Member Author

I don't think there's much to leave; we will need both dlopen() and dlpath() in Base. The only thing we don't need that is exported is find_libraries() (which I don't think anyone uses anyway).

Is the reason you want to leave it in the stdlib to reduce code size?

base/Base.jl Outdated Show resolved Hide resolved
@JeffBezanson
Copy link
Member

Yes, it's nice to be able to reduce code size just by excluding stdlibs from the build. But if the majority of the Libdl code is needed in Base anyway, I suppose it doesn't matter.

@vtjnash
Copy link
Member

vtjnash commented Apr 30, 2020

It's also a really hefty dependency in terms of libc support, since we're going far beyond the posix spec here. Though realistically, we expect it to exist on most targets.

@staticfloat
Copy link
Member Author

It really doesn't seem like that much code to me. SLOC isn't really a useful measure (113 lines in this case), but the code itself is pretty straightforward; over half of it is just simple ccall() wrappers.

The one thing I think we can do to reduce it is to remove the find_library function from Base and keep that in stdlib. There are 32 packages in existence that use it. Looking through them, I think that most of them should use JLLs instead, but it does mean that we can't just drop it, unfortunately.

@JeffBezanson
Copy link
Member

Is this needed in 1.5?

@staticfloat
Copy link
Member Author

No, we can keep it for later.

@staticfloat staticfloat force-pushed the sf/libdl_in_base branch 3 times, most recently from b2365ff to 7e26f3e Compare May 21, 2020 03:08
@vtjnash
Copy link
Member

vtjnash commented May 21, 2020

It really doesn't seem like that much code to me

Only if you don't count the demands you're placing on libdl, but since codegen/ccall already does use dlopen and doesn't (yet?) define static linking, it's not a new issue.

My main concern is just that we may want to avoid using this anywhere in the stdlib, as it has been a known performance issue in the past, since some libraries (for example openblas and libgit2) are known to have very high costs to load them, and by default right now we manage to defer that to their first usage.

@staticfloat
Copy link
Member Author

since some libraries (for example openblas and libgit2) are known to have very high costs to load them

Ah, yes, I can see why you would want to keep using ccall() with sonames instead of dlopen()/ccall() with a handle. That's something that will be getting even worse as we use JLLs, since those will explicitly dlopen() within their __init__() methods. This is a baked-in assumption of the current JLL interface, but I think we could work around this with slightly more clever tooling.

Right now, in order to dlopen() something that has dependencies, we make use of the fact that we can dlopen("libfoo.so") which depends on libbar.so by first loading libbar.so and relying on libc's behavior of not bothering to look for libraries that are already loaded (as identified by SONAME). To enforce the load order and to maintain a simple API, we dlopen() all libraries within a JLL module at __init__() time, then export the handles from that module for other modules to use in ccall()/dlsym().

We could build a system more similar to the "implicit ccall() method" by altering the JLL interface somewhat; if we had the graph of library dependencies per-library, and we could execute some code before each ccall(), we could lazily-load all of our dependent libraries just-in-time.

We already have the graph of library dependencies available to us in BB (Although the information is not yet saved/exported anywhere, but we can bake it into the JLL packages), and to hook some code in before ccall(), my preference would be to change the JLL API contract from exporting const libfoo = "libfoo.so" to instead exporting something like:

function libfoo()
    if !libfoo_is_loaded
        load_libfoo()
        libfoo_is_loaded = true
    end
    return "libfoo.so"
end

Then, users would use ccall(LibFoo_jll.libfoo(), ...) instead of ccall(LibFoo_jll.libfoo, ....), and we would all get what we want.

This is all tangential to this PR of course, it's merely setting proper expectations for how we want this module to be used. Thoughts?

We need to `dlopen()` things in Base a lot more due to using JLL
packages within `Base` and the standard library.  We move `Libdl` into
`Base.Libc`, but continue to carry a shell `Libdl` stdlib that
re-exports all relevant pieces of functionality, for
backwards-compatibility.
@staticfloat
Copy link
Member Author

I believe I have addressed everyone's comments, so I'm going to merge unless someone speaks up soon.

@staticfloat staticfloat merged commit f935125 into master Jun 30, 2020
@staticfloat staticfloat deleted the sf/libdl_in_base branch June 30, 2020 17:03
simeonschaub pushed a commit to simeonschaub/julia that referenced this pull request Aug 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
building Build system, or building Julia or its dependencies
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants