Skip to content

Add support and example for dynamically linked function#1005

Closed
pedroerp wants to merge 1 commit intofacebookincubator:mainfrom
pedroerp:velox-udf-new
Closed

Add support and example for dynamically linked function#1005
pedroerp wants to merge 1 commit intofacebookincubator:mainfrom
pedroerp:velox-udf-new

Conversation

@pedroerp
Copy link
Copy Markdown
Contributor

@pedroerp pedroerp commented Feb 8, 2022

Adding a DynamicLibraryLoader.h header to help loading and registering functions defined in dynamically loaded library. Also adding a unit test containing an example of a function dynamically linked and registered. Apart from complications related to .so built with different toolsets, this provides the base functionality that works for consistent platforms.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Feb 8, 2022
Copy link
Copy Markdown
Contributor

@kgpai kgpai left a comment

Choose a reason for hiding this comment

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

Looks great @pedroerp . One question, should we warn users when user loaded udfs overwrite velox library provided udfs.


namespace facebook::velox::exec {

static constexpr const char* kSymbolName = "registry";
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I wonder if we should call it something like velox_registry to sort of namespace it ?

// Try to dynamically load the shared library.
void* handler = dlopen(fileName, RTLD_NOW);

if (handler == nullptr) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: just use VELOX_USER_CHECK(handler, "Error...").

# Here's the actual test which will dynamically load the library defined above.
add_executable(velox_function_dynamic_link_test DynamicLinkTest.cpp)

add_test(NAME velox_function_dynamic_link_test
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Neat!

Copy link
Copy Markdown
Contributor

@wenleix wenleix left a comment

Choose a reason for hiding this comment

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

Thanks!


void loadDynamicLibraryFunctions(const char* fileName) {
// Try to dynamically load the shared library.
void* handler = dlopen(fileName, RTLD_NOW);
Copy link
Copy Markdown
Contributor

@wenleix wenleix Feb 10, 2022

Choose a reason for hiding this comment

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

noob question: what will happen if multiple .so is loaded ? (will there be multiple "registry" symbol ?)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I thought the registry name is loaded from file passed so it shouldnt matter if multiple .so are loaded as long as they have different paths. Collision between functions during registrations though can happen.

Copy link
Copy Markdown
Contributor

@wenleix wenleix Feb 10, 2022

Choose a reason for hiding this comment

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

From https://pubs.opengroup.org/onlinepubs/7908799/xsh/dlopen.html, it says

With the exception of the global symbol object obtained via a dlopen() operation on a file of 0, dependency ordering is used by the dlsym() function. Load ordering is used in dlsym() operations upon the global symbol object.

Since we didn't use RTLD_GLOBAL, it should use dependency ordering:

Dependency ordering uses a breadth-first order starting with a given object, then all of its dependencies, then any dependents of those, iterating until all dependencies are satisfied.

which should work since it searches first in the handler .

Copy link
Copy Markdown
Contributor

@kgpai kgpai Feb 10, 2022

Choose a reason for hiding this comment

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

Thanks Wenlei. I can also forsee another problem where say if a .so depends on another .so which has a registry function in that case behavior can be undefined.

@laithsakka laithsakka self-requested a review February 12, 2022 00:58
@laithsakka
Copy link
Copy Markdown
Contributor

laithsakka commented Feb 12, 2022

Thanks @pedroerp for adding this.

shall we move DynamicLibraryLoader from expression to exec/dynamic_loader?

A couple of note for the extension of this work:

  1. We might want to register dynamically loaded functions in a separate registery that supports un-registering also
    and keep tracking of loaded libraries and what functions registered by what libraries, and when unloading a library
    unregister related functions.

  2. We have a general state-full library loader interface to load and manage the life cycle of dynamically
    generated libraries that was used in codegen and might be worth looking at.
    /velox/experimental/codegen/library_loader

The interface might have things that might not be needed such as

  1. Enforcing a function table to be defined in the loaded library that tells what functions are exported and their types hashes (used to verify matched between requested function signature and the one found in the library).

  2. Enforcing every library to have init() and release() that are called upon loading and before unloading.

But those enforcement can be skipped by using the unchecked interface of the loader.

The interface provides several other functionalities such:

  1. Tracking loaded libraries and paths they came from .
  2. Generic getFunction interface that extracts a function from a loaded library.

@mbasmanova
Copy link
Copy Markdown
Contributor

Closing stale PRs. Please, reopen if you'd like to continue working on this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants