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

[WIP/RFC] Test and document embedding with dynamically loaded libjulia. #28886

Closed
wants to merge 10 commits into from

Conversation

GunnarFarneback
Copy link
Contributor

@GunnarFarneback GunnarFarneback commented Aug 24, 2018

This is a first attempt to document and test code for embedding with dynamic loading of libjulia, as I requested myself in #28826.

Please point out any misconceptions I may have about how this ought to work and in particular if it is unsafe with respect to memory management or in any other way.

Known things that need more work:

  • Bikeshed the naming of files. embeddingdl is not great and really only a placeholder for something better.
  • Test on Windows. It is based off code that works on both Linux and Windows, but I don't have the infrastructure in place to test Julia on Windows.
  • Add documentation for Mac. I have neither knowledge nor any access to that platform.
  • Flesh out the usage examples.

@GunnarFarneback
Copy link
Contributor Author

CI is finally passing. Linux on Travis used to pass before it broke down. The FreeBSD build has intermittently timed out while running embeddingdl, but I'm not sure how much to make of that.

I still want to add some more tests of usage but the infrastructure and documentation are ready for review.

Copy link
Member

@c42f c42f left a comment

Choose a reason for hiding this comment

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

It's great to have documentation for this 👍

I'm in two minds about the best approach

  • If this is officially supported, I think we could add some portion of the helper infrastructure from your embeddingdl.c to julia.h. Just hide it behind an #ifdef so it doesn't intrude on normal use. This means users just wouldn't need to know about some of the gotchas like the jl_init special case.
  • If it's not officially supported, it feels a bit odd to officially document it :-) But we could just run with what you've got already and clearly state that this type of embedding is experimental.

I prefer the former option but that's just my opinion. To me a lot of the C API exposed in julia.h still seems like a wild west containing semi internal stuff and it's not really clear where the embedding API ends and the internals begin.

close(out.in)
close(err.in)
out_task = @async readlines(out)
err = read(err, String)
Copy link
Member

Choose a reason for hiding this comment

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

The part above looks (almost?) exactly like @testset "embedding example" and could be factored out into a function.

// Extracted from src/support/platform.h
#if defined(_WIN32) || defined(_WIN64)
#define _OS_WINDOWS_
#endif
Copy link
Member

Choose a reason for hiding this comment

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

Why did this need to be added to embedding.c?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Very good question. I don't see that myself now. Backing it out and see if it still passes CI is probably probably the best option.

#else
const char *library_name = "libjulia.so";
#endif
void *libjulia = dlopen(library_name, RTLD_LAZY | RTLD_GLOBAL);
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible to load without RTLD_GLOBAL?

#include <stdio.h>
#include <stdlib.h>
#include <string.h>

Copy link
Member

Choose a reason for hiding this comment

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

A comment which links back to the markdown documentation would be good here. That way people who only read the source code will see the larger picture.

A very brief self contained overview (just a few lines) would be welcome too.

#define jl_get_global (*p_jl_get_global)
#define jl_unbox_voidpointer (*p_jl_unbox_voidpointer)
#define jl_string_ptr (*p_jl_string_ptr)
#define jl_exception_occurred (*p_jl_exception_occurred)
Copy link
Member

Choose a reason for hiding this comment

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

All this looks like useful infrastructure which could potentially go in julia.h. That might be easier for users than having them keep all this code up to date in their own projects.

Roughly speaking, something like the following pattern (in julia.h):

#ifndef JL_DLOPEN_INTERFACE
// Normal content of julia.h
#else
// Insert code for loading function pointers and masquerading, etc.
#endif

Then the embedded users can simply

#define JL_DLOPEN_INTERFACE
#define JL_DLOPEN_DEFINE_INTERFACE // To guard definition of p_jl_init and any other required static vars so it's done only once if the user plugin has multiple source files
#include <julia.h>

// Rest of the code below generally like the non-dlopen version (if with some restrictions)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Any particular advantage to having it in julia.h rather than in a separate file? I actually found it helpful not having to include julia.h as that gave me conflicts with some qt header file.

Copy link
Member

Choose a reason for hiding this comment

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

Only that julia.h is currently the place where the public interface is defined.

The idea of defining the symbol JL_DLOPEN_INTERFACE is that it would strip julia.h down to almost nothing so you shouldn't have any conflicts with other libraries such as Qt. Do you remember what the conflicts were?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, that was too long ago now.

jl_eval_string(command);

if (jl_exception_occurred()) {
const char *p = jl_string_ptr(jl_eval_string("sprint(showerror, ccall(:jl_exception_occurred, Any, ()))"));
Copy link
Member

Choose a reason for hiding this comment

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

I think this is safe from a GC rooting perspective. But it's worth a comment that p must be used immediately; either as you do below, or via a strcpy to a buffer managed on the C side.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, a comment could be useful. The construction is also discussed in the last posts of https://discourse.julialang.org/t/julia-exceptions-in-c/18387/8.

void *libjulia = dlopen(library_name, RTLD_LAZY | RTLD_GLOBAL);
#endif

if (!libjulia) {
Copy link
Member

Choose a reason for hiding this comment

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

Might be nice to also do some version checking here (jl_ver_major, at least?).

Though I don't know whether the embedding API has any ABI guarentees yet?

Copy link
Member

Choose a reason for hiding this comment

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

To clarify what I mean about ABI guarantees — those would define how closely we must match jl_ver_major / jl_ver_minor / jl_ver_patch in order to further use the libjuila that we are dlopening.

Copy link
Member

Choose a reason for hiding this comment

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

Keno comments on slack that "there are no stability guarantees on libjulia".

So naively we should check for a match of JULIA_VERSION_MAJOR == jl_ver_major() and the same for jl_ver_minor at least. Maybe also jl_ver_patch though I hazard a guess that a plugin based on the functions in jlapi.c is likely to keep working between patch versions.

All of this can be put into an inline jl_check_version() function.

// `include` is not available out of the box from the embedding
// environment, so we add it here (issue #28825).
checked_eval_string("include(x) = Base.include(Main, x)",
"Failed to define include: ");
Copy link
Member

Choose a reason for hiding this comment

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

Will be fixed in #32062, but still required for anyone targeting 1.0, unless that's backported.

Copy link
Member

Choose a reason for hiding this comment

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

This is now merged, so you should be able to remove this.

@stevengj
Copy link
Member

stevengj commented Feb 25, 2020

See also #25326 — would be good to have a way to query a julia executable for the path of libjulia.

However, you can do:

Libdl.dlpath("libjulia")

right now.

@GunnarFarneback
Copy link
Contributor Author

Although I still think it would be good to have documentation and tests for this in the Julia repository, this PR is now effectively superseded by the package https://github.com/GunnarFarneback/DynamicallyLoadedEmbedding.jl.

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