Better integration of "foreign types"?

[fingolfin]

Two years ago, our PR was accepted which among other things added jl_new_foreign_type to the Julia kernel. We've been relying on that work since then in GAP.jl.

However, it has one annoying limitation: these foreign types are not visible to precompilation. We worked around this by adding an abstract supertype GapObj. Instead of the actual foreign type, we then use GapObj throughout our Julia code. That was fine until we started to write higher level Julia types which contain GapObj members: As we discovered, by having x::GapObj instead of x::ActualConcreteType, various optimization are disabled. I am now looking into working around this by parametrizing all our structs (not sure whether that will work out). But I've also wondered if it might be possible to enhance precompilation to deal with our foreign type. Thinking extremely naively, perhaps one could add a syntax extension foreign type MyForeignType end or some other means (say, some special macro/function call/whatever) to inform the precompiler about the foreign type? It could then parse code referencing this, and would trust that the __init__ section of the package in which it occurs will actually provide such an external type. However, I don't even know whether that is enough information about the type for the precompiler, perhaps it needs much more; perhaps some of it could be added, perhaps others make the whole idea impossible. I clearly don't know enough about how precompilation works. Before I try to dig deeper and find out, I thought I could ask the experts whether this sounds even remotely realistic to achieve? Or perhaps I am overlooking a much simpler solution?

UPDATE 17. June 2022: Just to say, things have evolved quite a bit since I opened this issue. In particular, the foreign type is now initialized in GAP_jll, and this seems enough to enable precompilation in GAP.jl, which uses GAP_jll. So all is good from that point of view. It would still be nice to be able to implement more, e.g. having some kind of interface to enable handling (de)serialization of instances of this type. I realize it'll be up to us to provide one.

[vtjnash]

Since this is supposed to be a "fake" type, that just lets the GC read the layout field, its should be fine to teach precompile just to make an actual copy of the object whenever it encounters one of these DataTypes.

[fingolfin]

@vtjnash apologies, but I don't quite follow: "make an actual copy of the object" -> what does "the object" refer to here?

As far I understand things, there are no instances of the foreign type during precompilation -- indeed, I assume the foreign type does not even exist during precompilation, as __init__ has not yet run, and hence jl_new_foreign_type has not yet been called. Am I wrong?

Assuming I am right, it seems I need a way to tell Julia earlier about the to-be-created foreign type, by inserting something into the code which informs it that MyForeignType, if encountered, refers to a foreign type. My guess is that this might then also affect things like the layout of structs? My (again, naive) idea would be that any MyForeignType member would be stored like an Any, i.e. as a pointer to the actual object (as nothing can be said about the size of instances).

To illustrate the concrete issue: Right now, we have code like this, where GapObj is the supertype of the foreign type:

import GAP
import GAP: GapObj
struct Group
  x::GapObj 
end
foo(G::Group)::Bool = GAP.Groups.IsAbelian(G.x)
G = Group(GAP.Globals.CyclicGroup(10))
# very silly example
function bar(G::Group)
  x = true
  for i = 1:10000
    x = x && foo(G)
  end
end

Unfortunately, just to be able to set GapObj as supertype for our foreign type requires some hacking in some scenarios (namely in the one where Julia is embedded into GAP; then we have to create our foreign type before GAP.jl is loaded, and hence I had to resort to a nasty hack to change its supertype later on, once GAP.jl is loaded; see also https://discourse.julialang.org/t/swizzling-the-super-type-of-a-foreign-julia-type-or-how-evil-must-i-be/33733).

I also wonder whether if we were able to directly write MyForeignType instead of GapObj in those structs, some code could be more performant as it would enable better optimizations.... Though perhaps that's wrong; or perhaps even if it is right, using parametrized types can help mitigate this. I am still experimenting and running tons of benchmarks

[timholy]

Looks like we may have to solve this if we want #43990. By "make a copy" I assume you mean an opaque buffer? How do you figure out how big the object is? I don't see that in the arguments to jl_new_foreign_type.

[fingolfin]

@timholy I don't understand what "make a copy" refers to? @vtjnash brought it up, I asked for clarification, but so far got none.

In general, we can of course extend the foreign type API to cover new needs by Julia; if you can tell me what is needed, I can help come up with a way to provide it.

Note that the description of how things work in Oscar resp. GAP.jl is no longer fully accurate: we've worked around many of the issues we had in the past by moving the initialization of the foreign type into GAP_jll. Then GAP.jl refers to it, which has the advantage that the foreign type is already known to Julia, and GAP.jl and other packages using it can now use the foreign type GapObj directly inside of structs, method arguments etc. The only thing that doesn't work is doing something like const myGapObj = func_that_returns_GapObj() at the top level, because there is currently no way to serialize such an object (new kernel APIs would have to be added for that, I think). Anyway, we are not really unhappy about that right now.

Actually, there is also some hackery going on: for aesthetic reasons, we modify the foreign type in GAP.jl to changes its parent module from GAP_jll to GAP; this is mostly to ensure users are less confused; we tried to do with a custom show method instead in the past, but then e.g. Vector{GapObj} still was printed as Vector{GAP_jll.GapObj}, and we eventually gave up. If this hack is a problem, we can remove it, though (and I'd appreciate any pointers on "how to do it properly" if there are any...)

Internally, there are actually three foreign types, but only one (GapObj) is ever directly interacted with; it is basically a glorified Ref which only stores a pointer to the actual underlying foreign objects, and nothing else.

I could explain more, but that's probably besides the point. I had a look at PR #43990 but could not determine from that what the exact problem is there resp. how it relates to Oscar.jl / GAP.jl / GAP_jll / foreign types. But we are certainly happy to discuss ways to move this forward

[timholy]

Here's the error:

julia> using Oscar
fatal: error thrown and no exception handler available.
ErrorException("Cannot serialize instances of foreign datatypes")
ijl_error at /home/tim/src/julia-master/src/rtutils.c:41
jl_serialize_value_ at /home/tim/src/julia-master/src/dump.c:978
jl_serialize_value_ at /home/tim/src/julia-master/src/dump.c:605 [inlined]
jl_serialize_value_ at /home/tim/src/julia-master/src/dump.c:778
jl_serialize_value_ at /home/tim/src/julia-master/src/dump.c:605 [inlined]
jl_serialize_value_ at /home/tim/src/julia-master/src/dump.c:829
jl_serialize_value_ at /home/tim/src/julia-master/src/dump.c:605 [inlined]
jl_serialize_value_ at /home/tim/src/julia-master/src/dump.c:660
jl_serialize_value_ at /home/tim/src/julia-master/src/dump.c:605 [inlined]
jl_serialize_value_ at /home/tim/src/julia-master/src/dump.c:850
jl_serialize_value_ at /home/tim/src/julia-master/src/dump.c:605 [inlined]
jl_serialize_value_ at /home/tim/src/julia-master/src/dump.c:660
jl_serialize_value_ at /home/tim/src/julia-master/src/dump.c:605 [inlined]
jl_serialize_value_ at /home/tim/src/julia-master/src/dump.c:850
jl_serialize_value_ at /home/tim/src/julia-master/src/dump.c:605 [inlined]
serialize_htable_keys at /home/tim/src/julia-master/src/dump.c:1033 [inlined]
ijl_save_incremental at /home/tim/src/julia-master/src/dump.c:2578
jl_write_compiler_output at /home/tim/src/julia-master/src/precompile.c:65
ijl_atexit_hook at /home/tim/src/julia-master/src/init.c:207
jl_repl_entrypoint at /home/tim/src/julia-master/src/jlapi.c:707
main at /home/tim/src/julia-master/cli/loader_exe.c:59
__libc_start_main at /lib/x86_64-linux-gnu/libc.so.6 (unknown line)
_start at /home/tim/src/julia-master/usr/bin/julia (unknown line)

Debugging statements show this occurs while serializing GET_FUNC_PTR(::GAP.GapObj, ::Int64), which gets compiled when Oscar is built. Current versions of Julia can't serialize it because it's precompiled in Oscar but owned by GAP, and that inability to serialize external methods is what saves you from hitting the fact that Julia can't serialize foreign types. Were you to force compilation of that method in GAP (e.g., add precompile(GET_FUNC_PTR, (GapObj, Int)) to GAP's source code), you'd hit the same error on current versions of Julia. The point of #43990 is to be able to serialize virtually any precompiled code regardless of ownership, which is why we hit the bug.

[timholy]

It seems one could handle this by adding two new fields to jl_fielddescdyn_t, serialize and deserialize that would act similarly to jl_serialize_value and jl_deserialize_value. Thoughts?

[fingolfin]

So why is serializing GAP_FUNC_PTR a problem then? Looking at the code:

function ADDR_OBJ(obj::GapObj)
    mptr = Ptr{Ptr{Csize_t}}(pointer_from_objref(obj))
    return unsafe_load(mptr)
end
...
function GET_FUNC_PTR(obj::GapObj, narg::Int)
    #@assert TNUM_OBJ(obj) == T_FUNCTION
    #@assert 0 <= narg && narg <= 7
    bag_ptr = Ptr{Ptr{Nothing}}(ADDR_OBJ(obj))
    unsafe_load(bag_ptr, narg + 1)
end

So why does serializing this involve an instance of type GapObj? Or what else is the problem?

It would be really good to know which objects it tries to serialize -- because only for some will serialization be possible, I am afraid...

[fingolfin]

I'd like to debug this (in particular, it would be good to know TNUM_OBJ(obj) for whatever GapObj objects it is trying to serialize) but am not sure how to go about that using gdb/lldb. I am not even sure how to find out the name of the function being serialize (GET_FUNC_PTR). I'll try to find out tonight/next week, but if there are any hints or tricks you could quickly point me at...

[fingolfin]

Oh wait, or is just serializing the type what causes problems, as opposed to serializing instances? Sorry for the clearly naive questiojns

[timholy]

Not naive questions at all.

Here's what's happening: Methods contain a roots field that stores items needed in codegen or code-compression. (The encoding of these roots changed recently, see #43793 and #43881.) The error gets triggered when trying to store a new root for (snapshot of debugging output)

New root for (file /home/tim/.julia/packages/GAP/wF2n6/src/wrappers.jl, line 61) String(Any)
    GAP.GapObj()

So it seems calling https://github.com/oscar-system/GAP.jl/blob/68938062694bc18e01e44f6574a6fcecffd08663/src/wrappers.jl#L61is the problem: calling it forces compilation, which adds a new root to the method, and serializing the new root is what triggers the error.

EDIT: the particular specialization is GAP.Wrappers.String(::GAP.FFE).

[timholy]

Yes, so in this case the error is not in serializing an instance but merely the root

MethodInstance for GAP._call_gap_func(::GAP.GapObj, ::GAP.FFE)

which only involves types.
EDIT: sorry, off-by-one bug (C starts counting at 0). It's the root

julia> r = m.roots[4]
GAP: <Attribute "String">

julia> typeof(r)
GAP.GapObj

of GAP.Wrappers.String

I'm no longer sure of the connection to GET_FUNC_PTR. Keep in mind that serialization order is not deterministic, so the error varies from run-to-run.

[fingolfin]

Unfortunately that is a GAP function object and as such will be difficult to serialize. That said, I still think it would be good to add a (de)serialize code interface for foreign types, even if initially it only supports a few types -- at least we'd then have something to build on. Even better would be if these (de)serialize functions would be allowed to raise an exception to help debugging.

In this particular code, GAP.Wrapper.String, defined via @wrap String(x::Any)::Any, is a @generated function, which is meant to effect an optimization. I can turn off this optimization, then resulting function would become more or less

IsString(x) = GAP.Globals.IsString(x)

The @generated variant instead looks up the value GAP.Globals.IsString, and then emits code which directly uses that object.

Perhaps I could also do something like this:

const _IsString = Ref{GapObj}()
function _IsString(x)
  isassigned(_IsString) || _IsString[] = GAP.Globals.IsString
  return _IsString[](x)
end

but I'll have to benchmark how that performs (and whether it works at all 😂)

[timholy]

I see. So even if we modified jl_new_foreign_type it might not actually fix Oscar?

Another option is to add __precompile__(false) to the top of Oscar. Is precompilation that important to you? Given that it's dancing around an error, It seems like a poster child for why you want to be able to turn off precompilation.