[REVIEW] Jitify versions of binaryops for non-homogeneous types

[devavret]

Work in this PR

This PR carries forward the work of @ironbit from libgdf PR 94.

Closes #593
Closes #1057
Depends on #694

Investigation:

• Jitify overheads
• Global cache implementation

Implementation:

• Move core functionality code
• Move unit tests
• cuDF python bindings

Results of Investigation:

Jitify Overheads

I ran a simple vector add on 10 million elements, multiple times to find the overhead of JIT compiling and the overhead of retrieving and running a cached compiled kernel. The results are in the table below (all times are in ms):

Compile	Execute	API	Existing
157.4500	3.22	292.88	3.53831
0.0467	3.13	3.28	3.40279
0.0151	3.14	3.23	3.40932
0.0137	3.11	3.19	3.40577

The cost of JIT compiling is indeed large but the cost of running a cached kernel (hashing and retrieving) is negligible. So any future effort needs to be in cache persistence.
Since the API cost is almost double the compile cost, I also tried to break down the cost of JIT program creation which was divided into two parts: reading the program string and compiling.

Program init	Compile
143.8870	131.2620
0.1201	0.0325
0.0506	0.0176
0.0483	0.0164

Running different instantiations of kernels (different binop/Types) after the first one, does not incur this Program init cost, only the Compile cost.
I didn't dig deeper into what the cost of reading the program string is all about.

Global cache implementation

There's three levels of cache persistence I considered:

• Thread level
• Process level
• Inter process level

Thread level

This was already implemented in libgdf PR 94

Process level

Jitify has an environment variable JITIFY_THREAD_SAFE that can be enabled to turn on thread safety. The cache was simply stored globally with that option turned on and a CMake option was added to turn this off if you want.
I confirmed that this worked and did provide performance benefit by running a simple script:

from concurrent.futures import ThreadPoolExecutor
import numpy as np

e = ThreadPoolExecutor(100)

gfs = [cudf.DataFrame({'x': (np.arange(3, 10003)*3).astype('int32'),
                       'y': np.arange(10000).astype('int16')}) for _ in range(1)]

def f(gf, i):
    print('.', end="", flush=True)
    return (gf.x + gf.y)

ts = time.time()
list(e.map(f, gfs * 1000, data))
te = time.time()
print(te-ts)

Which when ran with thread local cache, took 23 s and with process level cache, took 0.7282 s

Inter process level

This was investigated but wasn't implemented because the Jitify API to serialize/deserialize is experimental and also requires the user to take care of the caching. Also to limit the scope of this PR.
Possible ways to implement this:

• Stored as a file in temporary folder. Can be done portably using std::filesystem::temp_directory_path but would need .lock files as unix doesn't have exclusive access to files.
• Stored in shared memory.
  In either case, more information is needed about usage scenarios.

[jrhemstad]

Only did a top-level review of the headers.

[harrism]

@devavret Just want to make sure you address my concerns in my review of the original libgdf PR (rapidsai/libgdf#94), as well as any concerns expressed by other reviewers. Thanks!

[devavret]

@harrism I presumed those would've been addressed already. I see they haven't. I'm on it.

[jrhemstad]

Thanks for taking the time to update this PR and get it working.

Several remnants that were inherited from the original PR need to be updated:

1. Parameter names like vax and vay are confusing. Should instead be renamed with lhs and rhs or left and right.
2. Most of the internal implementation files are missing Doxymentation
3. Lots of custom and reimplemented traits that are already present in Jitify or that should be added to Jitify (e.g., std::common_type). Removing these will significantly simplify this code.
4. Non-standard error checking functions that should user GDF_REQUIRE.
5. The null handling in the kernels is unnecessary and inefficient. It can instead be done as a simple memcpy (outside of the kernel).

[eyalroz]

Shouldn't this be gdf_datum, signifying the singular rather than the plural?

gdf_data sounds super- vague, and is also confusing IMHO since it can be interpreted to mean lots of data, or all data in a column etc.

[eyalroz]

Come on... don't do this. Use an array. Or a flat map (if we have one; probably not). Or at worst, an unordered map.

[kkraus14]

Minor changes needed

[kkraus14]

Should we pin this to a specific branch / tag / commit?

[mtjrider]

Yes, please pin to a commit to avoid build issues in the future.

[devavret]

Will this do? Or do I need to specify it in .gitmodules.

[mtjrider]

👍

[kkraus14]

We need to specify it in .gitmodules otherwise a git submodule update --init --recursive --remote will update it to the latest commit of master.

[devavret]

I can't seem to find a way to pin this to a commit/tag in the .gitmodules file. I read this and update seems to be the only option. But details on update say that the only way to do this is to use none. Is that what I should do?

[mtjrider]

Yes, please pin to a commit to avoid build issues in the future.

[mtjrider]

DEPENDS does not have to be a path, it can be a CMake variable.

For example:

add_custom_command(OUTPUT  TYPES_JIT
	                   WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}/include
	                   COMMAND ${CMAKE_BINARY_DIR}/stringify cudf/types.h > ${CMAKE_BINARY_DIR}/include/types.h.jit
                           ...)
...
add_custom_target(stringify_run DEPENDS TYPES_JIT)
...

As long as the MAIN_DEPENDENCY is properly specified to be ${CMAKE_CURRENT_SOURCE_DIR}/include/cudf/types.h, CMake will update jit_types if that file changes.

Note that this is helpful if later you expect your command to have multiple outputs.

I would also consider renaming stringify_run to something more descriptive. I'm not really sure what the purpose of this target is off the cuff, and that obfuscates our build process for new developers, etc.

[devavret]

I tried this but it didn't work. Running make rebuilds launcher.cpp every time.

add_custom_command(OUTPUT STRINGIFIED_HEADERS
                   WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}/include
                   COMMAND ${CMAKE_BINARY_DIR}/stringify cudf/types.h > ${CMAKE_BINARY_DIR}/include/types.h.jit
                   COMMENT "Run stringify on header types.h to convert it to c-str for use in JIT compiled code"
                   DEPENDS stringify
                   MAIN_DEPENDENCY ${CMAKE_CURRENT_SOURCE_DIR}/include/cudf/types.h)

add_custom_target(stringified_headers DEPENDS STRINGIFIED_HEADERS)

add_dependencies(cudf stringified_headers)

[harrism]

Sorry what does the cmake code above have to do with launcher.cpp?

[devavret]

This code is for making and running an executable that comes with Jitify. This executable, called stringify, converts source files into c-strings and writes them to another source file. Now those stringified source files can be used in JIT code compilation. In our specific use case, we wanted to be able to use the definitions in types.h in the JIT kernels. So we use stringify to convert types.h into a string and store it in types.h.jit in the build/include directory. That types.h.jit is included in launcher.cpp. I wanted a way to use CMake to run stringify only when types.h is changed. With @mt-jones ' suggestion, CMake runs it all the time and every time i run make, launcher.cpp is rebuilt because the include is touched.

[kkraus14]

I think the only thing left is the change of the submodule to a fork on the rapidsai project. Once that's done I'll approve.

[GPUtester]

Can one of the admins verify this patch?

[kkraus14]

add to whitelist

[devavret]

I think the only thing left is the change of the submodule to a fork on the rapidsai project. Once that's done I'll approve.

I'm only waiting on the ops ticket to fork jitify. I cannot see the issue right now. (404 for me). So I'm waiting to be added to the rapids org.

Edit: nvm, found the fork.