OrcV1 removal

Hi All,

The time has finally come to remove OrcV1. I expect to remove it some time after the 14th of September. This will remove all the legacy layers, legacy utilities, the old Orc C bindings, and OrcMCJITReplacement. ExecutionEngine and MCJIT will not be affected by this.

I had hoped to have removable code enabled before deleting OrcV1, but it turns out that implementing removable code in OrcV2 without simultaneously breaking it in OrcV1 is difficult. Instead my plan is to delete OrcV1 and implement removable code in OrcV2 as quickly as possible. I think this is the fastest path to where we want to be.

If you’re on llvm master, still using the legacy layers, and you don’t need removable code support, then I would encourage you to switch over as soon as you’re able. If you do need removable code support then you may have to wait a few weeks for it to land.

If you have any questions about the removal please let me know!

Kind Regards,
Lang.

Hi,

The time has finally come to remove OrcV1. I expect to remove it some time
after the 14th of September. This will remove all the legacy layers, legacy
utilities, the old Orc C bindings, and OrcMCJITReplacement. ExecutionEngine
and MCJIT will *not* be affected by this.

I had hoped to have removable code enabled before deleting OrcV1, but it
turns out that implementing removable code in OrcV2 without simultaneously
breaking it in OrcV1 is difficult. Instead my plan is to delete OrcV1 and
implement removable code in OrcV2 as quickly as possible. I think this is
the fastest path to where we want to be.

If you're on llvm master, still using the legacy layers, and you *don't*
need removable code support, then I would encourage you to switch over as
soon as you're able. If you *do* need removable code support then you may
have to wait a few weeks for it to land.

If you have any questions about the removal please let me know!

Postgres uses removable code support and Orcv1. I does make me quite
worried to see a phase where there'll be no viable way of using both in
llvm. Why isn't the right answer here to at lest develop the
replacement as a set of patches / as a branch that then can be merged as
a whole / shortly after each other, rather than just starting to develop
a replacement after the removal.

Greetings,

Andres Freund

Hi Andres,

Postgres uses removable code support and Orcv1. I does make me quite
worried to see a phase where there’ll be no viable way of using both in
llvm. Why isn’t the right answer here to at lest develop the
replacement as a set of patches / as a branch that then can be merged as
a whole / shortly after each other, rather than just starting to develop
a replacement after the removal.

That sounds good to me. I’ll create a branch called ‘orcv1-removal’ for this shortly.

Regards,
Lang.

Hi Andres,

The orcv1-removal branch is now available (with OrcV1 removed) at https://github.com/lhames/llvm-project/tree/orcv1-removal. I’ll get to work on the removable code feature – hopefully I’ll have something for you to test soon.

Regards,
Lang.

Hi All,

I’ve updated the orcv1 removal branch (https://github.com/lhames/llvm-project/tree/orcv1-removal) with an initial patch for removable code. If anyone wants to follow along with the development or share thoughts on the design you’re very welcome to.

I’ll be adding tests and comments this week, but for anyone who wants to take an early look the main elements are defined in Core.h:

ResourceTracker – Your handle to remove code from a JITDylib. Also allows tracking to be merged onto another tracker (reducing the administrative overhead required for tracking).
ResourceKey – An opaque key associated with each tracker.
ResourceManager – A listener interface to be notified when resources associated with a given key should be removed.

Each JITDylib will have a default tracker (accessible via JITDylib::getDefaultResourceTracker), and allow creation of new trackers (via JITDylib::createResourceTracker). When adding a MaterializationUnit to a JITDylib (with JITDylib::define, or a Layer’s add method) you can optionally specify a tracker to associate with that unit. If no tracker is specified the default tracker for the target JITDylib will be used. A single tracker can be associated with multiple units the remove and transferTo operations (see below) will apply to all associated units.

You can call ResourceTracker::remove at any time to remove all symbols and resources associated with a tracker. Any active compiles associated with the tracker will receive an error when they try to update the JIT state via their MaterializationResponsibility, and will not be able to associate resources with the tracker’s associated ResourceKey.

You can call ResourceTracker::transferTo at any time. This will transfer tracking of all associated symbols and resources to the destination tracker. Any active compiles associated with the tracker will be reassociated with the destination tracker, and all future resources will be associated with the destination tracker. Merging trackers can reduce administrative overhead, especially when merging onto the default tracker for the JITDylib, which has a more compact representation for ownership of symbols.

Calling JITDylib::clear() will call remove on all trackers created by the JITDylib (including the default one).

ResourceTrackers have shared ownership, but the ExecutionSession and JITDylib do not retain ownership (except for the default tracker). If you release all pointers to a tracker its resources will be automatically transferred (via transferTo) to the default tracker for the JITDylib.

ResourceManagers (usually Layers) can call MaterializationResponsibility::withKeyDo(…) to associate a ResourceKey with JIT resources (e.g. allocated memory) in a way that is safe even if remove/transferTo is called on the same tracker from other threads.

A note for those of you who’ve been following along with the ORC Weekly emails: I had previously convinced myself that we’d only be able to do JITDylib at a time removal. This scheme is more flexible than that – I’m hoping it ends up being a nice compromise. If you ignore tracking in your API it is effectively JITDylib-at-a-time (through the default tracker), but if you do want to do fine grained removal you can, you just assume responsibility for properly handling dependencies (i.e. not removing anything that the JIT program might still be using).

I’ll be re-hashing this in more detail in this week’s ORC Weekly update, hopefully with examples and cleaner code.

Regards,
Lang.

Hi,

I've updated the orcv1 removal branch (
https://github.com/lhames/llvm-project/tree/orcv1-removal) with an initial
patch for removable code. If anyone wants to follow along with the
development or share thoughts on the design you're very welcome to.

Cool!I assume this works on "non-native" jitlink platforms as well? Or
just mach?

Looks like there's not yet a C API yet - not a problem, just checking.

Currently it looks like only the main jit dylib is exposed via C, right?
We can't really do the same for resource trackers, if I understand this
correctly. Sketching out what I'd need to do to test postgres, so I
don't waste time going in the wrong direction:

1) Add API for management (create, destroy) a non-default resource tracker
   (i.e. JITDylib::createResourceTracker)
3) Add JITDylib::clear() wrapper
2) LLVMOrcLLJITAdd{LLVMIRModule,ObjectFile} would need a new argument
   (defaulting to NULL for the default resource tracker?) to specify the
   resource tracker
4) LLJIT would need to grow the underlying support methods

Does that sound roughly right?

Independent questions about the current C API:
- Would be nice to document [non-]mangling behaviour of
  LLVMOrcLLJITLookup()
- Do I understand correctly that there's no way to keep a memory buffer
  with an object file alive after adding it with
  LLVMOrcLLJITAddObjectFile()? But that it's fine to create multiple
  buffers for the same memory with LLVMCreateMemoryBufferWithMemoryRange()?

You can call ResourceTracker::remove at any time to remove all symbols and
resources associated with a tracker. Any active compiles associated with
the tracker will receive an error when they try to update the JIT state via
their MaterializationResponsibility, and will not be able to associate
resources with the tracker's associated ResourceKey.

You can call ResourceTracker::transferTo at any time. This will transfer
tracking of all associated symbols and resources to the destination
tracker. Any active compiles associated with the tracker will be
reassociated with the destination tracker, and all future resources will be
associated with the destination tracker. Merging trackers can reduce
administrative overhead, especially when merging onto the default tracker
for the JITDylib, which has a more compact representation for ownership of
symbols.

Calling JITDylib::clear() will call remove on all trackers created by the
JITDylib (including the default one).

ResourceTrackers have shared ownership, but the ExecutionSession and
JITDylib do not retain ownership (except for the default tracker). If you
release all pointers to a tracker its resources will be automatically
transferred (via transferTo) to the default tracker for the JITDylib.

I assume it is not legal to use the same resource tracker with two
different JITDylib's?

Greetings,

Andres Freund

Hi Andres,

Cool!I assume this works on “non-native” jitlink platforms as well? Or
just mach?

This framework is totally materializer agnostic – It works for ObjectLinkingLayer (JITLink), RTDyldObjectLinkingLayer (RuntimeDyld), and even materializers that aren’t emitted via a linker (e.g. stubs and callbacks).

Looks like there’s not yet a C API yet - not a problem, just checking.

No C API yet, but I’ll add one as soon as the C++ API settles down (probably in a week or so).

Currently it looks like only the main jit dylib is exposed via C, right?
We can’t really do the same for resource trackers, if I understand this
correctly. Sketching out what I’d need to do to test postgres, so I
don’t waste time going in the wrong direction:

Currently it looks like only the main jit dylib is exposed via C, right?

That was true – I just hadn’t gotten around to it. Your question seems as good an excuse as any to take a minute out and do that, so I’ve added them in 9a0d1b66730. :slight_smile:

We can’t really do the same for resource trackers, if I understand this
correctly. Sketching out what I’d need to do to test postgres, so I
don’t waste time going in the wrong direction:

  1. Add API for management (create, destroy) a non-default resource tracker
    (i.e. JITDylib::createResourceTracker)
  2. Add JITDylib::clear() wrapper
  3. LLVMOrcLLJITAdd{LLVMIRModule,ObjectFile} would need a new argument
    (defaulting to NULL for the default resource tracker?) to specify the
    resource tracker
  4. LLJIT would need to grow the underlying support methods

That’s all spot-on. It should not require much code either: Both the C API and LLJIT itself are pretty thin wrappers around the core APIs – each of these wrappers should just be a few lines.

I assume it is not legal to use the same resource tracker with two different JITDylib’s?

That’s correct – ResourceTrackers are bound to the JITDylib they’re created from. They should assert if you use them with the wrong JITDylib, though I’m not sure how good the coverage on those asserts are yet.

I think this system could be generalized to enable cross-dylib tracking at the cost of a few dozen extra bytes per tracker. Do you have a use-case for this?

Regards,
Lang.

Hi,

Hi Andres,

I was wondering about it because the function comments didn’t mention it
(or at least I didn’t see it), and not “implicitly” prevent via type
system. I don’t really have a use case.

I tried to think of a nice way to do this with the type system but everything I tried came out awkward.

One use case I can think of for cross-dylib tracking is the current system for CompileOnDemandLayer: It creates a “.impl” JITDylib for each JITDylib you add code to, installs the code in the “.impl” JITDylib and builds stubs in the original JITDylib, treating it as a facade. I haven’t implemented removal properly for this yet, but my plan was to create new ResourceTrackers as needed and treat them as resources themselves in CompileOnDemandLayer: If you add a module with tracker T then CompileOnDemandLayer would create a tracker T’ and record the association key(T) → T’. Then on removal of T we would extract and trigger removal of T’. If ResourceTrackers worked across JITDylib boundaries we could avoid this, but it’s not particularly complicated anyway.

I’m going to leave the current solution in place for now, but if anyone does come up with a compelling use-case for cross-dylib tracking please let me know. :slight_smile:

– Lang.

Hi All,

I’ll be re-hashing this in more detail in this week’s ORC Weekly update, hopefully with examples and cleaner code.

Time got away from me last week unfortunately. I’m still landing unit tests and cleanups on the OrcV1 removal branch, but I hope to put out an ORC Weekly update on Friday covering the latest developments. In the meantime you can find some very basic example usage in llvm/unittests/ExecutionEngine/Orc/ResourceTrackerTest.cpp (https://github.com/lhames/llvm-project/blob/orcv1-removal/llvm/unittests/ExecutionEngine/Orc/ResourceTrackerTest.cpp).

– Lang.

Hi All,

The Kaleidoscope tutorials have now been updated on the orcv1-removal branch. I will try to summarise the state of the work and provide some examples in the ORC JIT Weekly mailout tomorrow. The short version is that I think this is ready to land on the mainline.

If anyone wants to check out the OrcV1 removal branch and provide feedback now is the time. Otherwise I will aim to land the work in the mainline early next week.

– Lang.

Hi,

Hi,

> If anyone wants to check out the OrcV1 removal branch and provide feedback
> now is the time. Otherwise I will aim to land the work in the mainline
> early next week.

I'm trying to get it to work with postgres. Unfortunately this week was a bit less
productive than I had hoped... Hope to get there Mon/Tues.

Have the basics working. Should I open a PR to your branch for what I
got so far?

There's currently two things I am still missing from the C API.

The first is to expose more of symbol resolution. Postgres can be
extended via dynamically loaded libraries, and we need to be careful to
look up symbols in the correct library. In the generated JIT IR
references to functions in such libraries use a distinguishable symbol
name (see example below), indicating that they should not be looked up
in the global namespace.

Previously we used the symbol resolution callback to
LLVMOrcAddEagerlyCompiledIR to implement this. If a symbol name was
scoped to a library, we only searched inside that, rather than globally:

/*
* Attempt to resolve symbol, so LLVM can emit a reference to it.
*/
static uint64_t
llvm_resolve_symbol(const char *symname, void *ctx)
{
...
  llvm_split_symbol_name(symname, &modname, &funcname);
...

  if (modname)
    addr = (uintptr_t) load_external_function(modname, funcname,
               true, NULL);
  else
    addr = (uintptr_t) LLVMSearchForAddressOfSymbol(symname);
...
  return (uint64_t) addr;
}

I don't think that can be implemented with the current C API.

Related to that, when hitting this case I currently get, on stderr:
JIT session error: Symbols not found: [ pgextern./home/andres/build/postgres/dev-assert/vpath/src/test/regress/regress.so.pt_in_widget ]

but currently that error is not bubbled up to the C API. There I just
get:
ERROR: XX000: failed to look up symbol "evalexpr_0_0": Failed to materialize symbols: { (main, { evalexpr_0_0 }) }

(obviously the first part is postgres code)

Do you have thoughts on what that should look like?

The other part that I am still missing is replacment for

LLVMCreateGDBRegistrationListener();
LLVMCreatePerfJITEventListener();
LLVMOrcRegisterJITEventListener(llvm_opt0_orc, l);

I haven't yet looked at whether there is replacement infrastructure for
this already. Is there? Will look after the next few meetings...

Greetings,

Andres Freund

Hi Andres,

Have the basics working. Should I open a PR to your branch for what I
got so far?

Sounds good to me.

There’s currently two things I am still missing from the C API.

The first is to expose more of symbol resolution. Postgres can be
extended via dynamically loaded libraries, and we need to be careful to
look up symbols in the correct library. In the generated JIT IR
references to functions in such libraries use a distinguishable symbol
name (see example below), indicating that they should not be looked up
in the global namespace.

From the example code I guess the namespacing for symbols from external libraries is something like: “.” ?

This is what definition generators are for. We can add an API for attaching generators and provide facilities to define a generator in C.

Here you’d want a generator with a map of module names to dylib handles. First you’d search for a handle for (loading it if it didn’t already exist) then you’d dlsym in that handle.

Related to that, when hitting this case I currently get, on stderr:
JIT session error: Symbols not found: [ pgextern./home/andres/build/postgres/dev-assert/vpath/src/test/regress/regress.so.pt_in_widget ]
but currently that error is not bubbled up to the C API. There I just
get:
ERROR: XX000: failed to look up symbol “evalexpr_0_0”: Failed to materialize symbols: { (main, { evalexpr_0_0 }) }

(obviously the first part is postgres code)
Do you have thoughts on what that should look like?

We need an API to install an error reporter on the session: That’s where the SymbolsNotFound error will be delivered. I’ll add that in a minute.

The other part that I am still missing is replacment for

LLVMCreateGDBRegistrationListener();
LLVMCreatePerfJITEventListener();
LLVMOrcRegisterJITEventListener(llvm_opt0_orc, l);
I haven’t yet looked at whether there is replacement infrastructure for
this already. Is there? Will look after the next few meetings…

You can continue to use LLVMCreateGDBRegistrationListener and LLVMCreatePerfJITEventListener from ExecutionEngine.h for now. We just need to add an OrcV2 equivalent of LLVMOrcRegisterJITEventListener. I’ll do that today.

– Lang.

Hi Andres,

… I’ll add that in a minute.
… I’ll do that today.

That may have been overly optimistic – I had to jump on to another bug for a bit. I’ll try to get these done in the next day or so though.

– Lang.

Hi,

> Have the basics working. Should I open a PR to your branch for what I
> got so far?

Sounds good to me.

Done, Orc removal changes required to make postgres mostly work by anarazel · Pull Request #2 · lhames/llvm-project · GitHub .

The first is to expose more of symbol resolution. Postgres can be
> extended via dynamically loaded libraries, and we need to be careful to
> look up symbols in the correct library. In the generated JIT IR
> references to functions in such libraries use a distinguishable symbol
> name (see example below), indicating that they should not be looked up
> in the global namespace.

From the example code I guess the namespacing for symbols from external
libraries is something like: "<modulename>.<symbolname>" ?

It's currently pgextern.<path_to_library>.<symbolname>

This is what definition generators are for. We can add an API for attaching
generators and provide facilities to define a generator in C.
Here you'd want a generator with a map of module names to dylib
handles.
First you'd search for a handle for <modulename> (loading it if it didn't
already exist) then you'd dlsym <symbolname> in that handle.

Ok, cool.

Related to that, when hitting this case I currently get, on stderr:
> JIT session error: Symbols not found: [
> pgextern./home/andres/build/postgres/dev-assert/vpath/src/test/regress/regress.so.pt_in_widget
> ]
> but currently that error is not bubbled up to the C API. There I just
> get:
> ERROR: XX000: failed to look up symbol "evalexpr_0_0": Failed to
> materialize symbols: { (main, { evalexpr_0_0 }) }

(obviously the first part is postgres code)
> Do you have thoughts on what that should look like?

We need an API to install an error reporter on the session: That's where
the SymbolsNotFound error will be delivered. I'll add that in a minute.

Sounds good.

> The other part that I am still missing is replacment for
> LLVMCreateGDBRegistrationListener();
> LLVMCreatePerfJITEventListener();
> LLVMOrcRegisterJITEventListener(llvm_opt0_orc, l);
> I haven't yet looked at whether there is replacement infrastructure for
> this already. Is there? Will look after the next few meetings...

You can continue to use LLVMCreateGDBRegistrationListener and
LLVMCreatePerfJITEventListener from ExecutionEngine.h for now. We just need
to add an OrcV2 equivalent of LLVMOrcRegisterJITEventListener. I'll do that
today.

Cool.

Greetings,

Andres Freund

Hi Andres,

I’ve just realised that we’re going to need a change to the definition generator API in the long term: Right now it is called under the session lock, but we want to shift to calling it outside the lock and passing a lookup-continuation. This would allow definition discovery to take an arbitrarily long time without blocking forward progress on other JIT work.

I’m guessing your objective for now is to minimize churn, right? If that’s the case I’ll focus on moving to the continuation passing API on the orcv1-removal branch before I update the C API.

Alternatively if you don’t mind some minor churn before LLVM 12 I’ll focus on landing the C API in its current form, then landing orcv1-removal on the mainline, then I can work on the continuation passing update in the mainline.

– Lang.

Hi,

I've just realised that we're going to need a change to the definition
generator API in the long term: Right now it is called under the session
lock, but we want to shift to calling it outside the lock and passing a
lookup-continuation. This would allow definition discovery to take an
arbitrarily long time without blocking forward progress on other JIT work.

Sounds useful.

I'm guessing your objective for now is to minimize churn, right? If that's
the case I'll focus on moving to the continuation passing API on the
orcv1-removal branch before I update the C API.

Yes, I do prefer less churn. It's not a deal breaker, but I try to keep
PG compiling against trunk LLVM, and there's multiple stable branches -
so it's somewhat noisy to do that repeatedly (including syncing up that
all the LLVM trunk test machines update at the same time).

I see a memory leak with OrcV2 that I didn't see with V1. I'm not yet
sure where exactly they're coming from. Possible that I'm just missing a
step somewhere. Or something around the removable code support doesn't
yet fully work.

Greetings,

Andres Freund

Hi Andres,

Yes, I do prefer less churn. It’s not a deal breaker, but I try to keep
PG compiling against trunk LLVM, and there’s multiple stable branches -
so it’s somewhat noisy to do that repeatedly (including syncing up that
all the LLVM trunk test machines update at the same time).

Ok – I’ll make getting the new lookup API working a priority then.

I see a memory leak with OrcV2 that I didn’t see with V1. I’m not yet
sure where exactly they’re coming from. Possible that I’m just missing a
step somewhere. Or something around the removable code support doesn’t
yet fully work.

I just checked and I’ve migrated ObjectLinkingLayer to support ResourceTracker, but not RTDyldObjectLinkingLayer yet. If you’re testing on Linux there’s a good chance that’s the source of your leak. I’ll get that updated tonight.

– Lang.

Hi,

Ok -- I'll make getting the new lookup API working a priority then.

Cool.

> I see a memory leak with OrcV2 that I didn't see with V1. I'm not yet
> sure where exactly they're coming from. Possible that I'm just missing a
> step somewhere. Or something around the removable code support doesn't
> yet fully work.

I just checked and I've migrated ObjectLinkingLayer to support
ResourceTracker, but not RTDyldObjectLinkingLayer yet. If you're testing on
Linux there's a good chance that's the source of your leak. I'll get that
updated tonight.

I indeed am testing on linux... Thanks.

Regards,

Andres