Skip to content

Commit

Permalink
[engine] Move to new-style CFFI callbacks. (pantsbuild#4324)
Browse files Browse the repository at this point in the history
Fixes pantsbuild#4036

Problem

We currently use "old-style callbacks" with CFFI. These are known to be slower and cause more complicated stack traces during debugging/performance analysis.

Solution

Switch to "new-style callbacks" using the following approach:

Bootstrap CFFI C sources pre-pants execution by way of a simple, venv-backed entrypoint fed by native.py and integrated into bootstrap.sh.
Compile and link the generated CFFI C sources into the rust binary at build time with cargo, by way of a build script + the gcc-rs package.
On OSX, we instruct cargo to use a linker flag (via .cargo/config) that enables weak linking to avoid missing python symbols during linking (this behavior is the default on Linux). The weakly linked symbols (e.g. _PyImport_ImportModule) are then dynamically resolved within the address space of the parent binary (python) at runtime.
Rename the native-engine rust binary to native_engine.so to be python import compatible.
Dynamically load the native_engine.so binary as a python module by injecting the binaryutil dir into sys.path at runtime and initializing the new-style callbacks with the closed-loop ffi object.
This results in a single native-engine binary that can be loaded both as a python module (import) and as a C module (dlopen) and is not statically or dynamically linked to python.

Result

As tested on my Sierra laptop, this is good for a >10% speedup as measured by ./pants --enable-v2-engine list :: in the pants repo.
  • Loading branch information
kwlzn authored Mar 13, 2017
1 parent f574b6f commit 8e24991
Show file tree
Hide file tree
Showing 10 changed files with 541 additions and 387 deletions.
9 changes: 9 additions & 0 deletions .cargo/config
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
# N.B. On OSX, we force weak linking by passing the param `-undefined dynamic_lookup` to
# the underlying linker used by cargo/rustc via RUSTFLAGS. This avoids "missing symbol"
# errors for Python symbols (e.g. `_PyImport_ImportModule`) at build time when bundling
# the CFFI C sources. The missing symbols will instead by dynamically resolved in the
# address space of the parent binary (e.g. `python`) at runtime - obviating a need to
# link to libpython.

[target.x86_64-apple-darwin]
rustflags = ["-C", "link-args=-undefined dynamic_lookup"]
24 changes: 19 additions & 5 deletions build-support/bin/native/bootstrap.sh
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,10 @@ source ${REPO_ROOT}/build-support/common.sh
source ${REPO_ROOT}/build-support/bin/native/utils.sh

readonly NATIVE_ROOT="${REPO_ROOT}/src/rust/engine"
readonly NATIVE_ENGINE_MODULE="native_engine"
readonly NATIVE_ENGINE_BINARY="${NATIVE_ENGINE_MODULE}.so"
readonly NATIVE_ENGINE_VERSION_RESOURCE="${REPO_ROOT}/src/python/pants/engine/subsystem/native_engine_version"
readonly CFFI_BOOTSTRAPPER="${REPO_ROOT}/build-support/native-engine/bootstrap_cffi.py"

# N.B. Set $MODE to "debug" to generate a binary with debugging symbols.
readonly MODE="${MODE:-release}"
Expand All @@ -40,11 +43,18 @@ function calculate_current_hash() {
# sensitive to the CWD, and the `--work-tree` option doesn't seem to resolve that.
(
cd ${REPO_ROOT}
git ls-files -c -o --exclude-standard "${NATIVE_ROOT}" | \
git hash-object -t blob --stdin-paths | fingerprint_data
git ls-files -c -o --exclude-standard \
"${NATIVE_ROOT}" \
"${REPO_ROOT}/src/python/pants/engine/subsystem/native.py" \
| git hash-object -t blob --stdin-paths | fingerprint_data
)
}

function ensure_cffi_sources() {
# N.B. Here we assume that higher level callers have already setup the pants' venv and $PANTS_SRCPATH.
PYTHONPATH="${PANTS_SRCPATH}:${PYTHONPATH}" python "${CFFI_BOOTSTRAPPER}" "$@"
}

function ensure_build_prerequisites() {
# Control a pants-specific rust toolchain, optionally ensuring the given target toolchain is
# installed.
Expand Down Expand Up @@ -95,17 +105,21 @@ function bootstrap_native_code() {
# Bootstraps the native code and overwrites the native_engine_version to the resulting hash
# version if needed.
local native_engine_version="$(calculate_current_hash)"
local target_binary="${CACHE_TARGET_DIR}/${native_engine_version}/native-engine"
local target_binary="${CACHE_TARGET_DIR}/${native_engine_version}/${NATIVE_ENGINE_BINARY}"
local cffi_output_dir="${NATIVE_ROOT}/src/cffi"
local cffi_env_script="${cffi_output_dir}/${NATIVE_ENGINE_MODULE}.sh"
if [ ! -f "${target_binary}" ]
then
ensure_cffi_sources "${cffi_output_dir}"
source "${cffi_env_script}"
local readonly native_binary="$(build_native_code)"

# Pick up Cargo.lock changes if any caused by the `cargo build`.
native_engine_version="$(calculate_current_hash)"
target_binary="${CACHE_TARGET_DIR}/${native_engine_version}/native-engine"
target_binary="${CACHE_TARGET_DIR}/${native_engine_version}/${NATIVE_ENGINE_BINARY}"

mkdir -p "$(dirname ${target_binary})"
cp "${native_binary}" ${target_binary}
cp "${native_binary}" "${target_binary}"

# NB: The resource file emitted/over-written below is used by the `Native` subsystem to default
# the native engine library version used by pants. More info can be read here:
Expand Down
12 changes: 6 additions & 6 deletions build-support/bin/native/generate-bintray-manifest.sh
Original file line number Diff line number Diff line change
Expand Up @@ -66,12 +66,12 @@ function emit_osx_files() {
# It appears to be the case that upload de-dupes on includePattern keys; so we make a unique
# includePattern per uploadPattern via a symlink here per OSX version.
ln -fs \
${CACHE_TARGET_DIR}/${native_engine_version}/native-engine \
${CACHE_TARGET_DIR}/${native_engine_version}/native-engine.10.${version}
${CACHE_TARGET_DIR}/${native_engine_version}/${NATIVE_ENGINE_BINARY} \
${CACHE_TARGET_DIR}/${native_engine_version}/${NATIVE_ENGINE_BINARY}.10.${version}
cat << EOF >> ${REPO_ROOT}/native-engine.bintray.json
{
"includePattern": "${CACHE_TARGET_DIR}/${native_engine_version}/native-engine.10.${version}",
"uploadPattern": "build-support/bin/native-engine/mac/10.${version}/${native_engine_version}/native-engine"
"includePattern": "${CACHE_TARGET_DIR}/${native_engine_version}/${NATIVE_ENGINE_BINARY}.10.${version}",
"uploadPattern": "build-support/bin/native-engine/mac/10.${version}/${native_engine_version}/${NATIVE_ENGINE_BINARY}"
}${sep}
EOF
done
Expand All @@ -83,11 +83,11 @@ function emit_linux_files() {
cat << EOF >> ${REPO_ROOT}/native-engine.bintray.json
{
"includePattern": "${native_engine_32}",
"uploadPattern": "build-support/bin/native-engine/linux/i386/${native_engine_version}/native-engine"
"uploadPattern": "build-support/bin/native-engine/linux/i386/${native_engine_version}/${NATIVE_ENGINE_BINARY}"
},
{
"includePattern": "${native_engine_64}",
"uploadPattern": "build-support/bin/native-engine/linux/x86_64/${native_engine_version}/native-engine"
"uploadPattern": "build-support/bin/native-engine/linux/x86_64/${native_engine_version}/${NATIVE_ENGINE_BINARY}"
}
EOF
}
Expand Down
19 changes: 19 additions & 0 deletions build-support/native-engine/bootstrap_cffi.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
# coding=utf-8
# Copyright 2017 Pants project contributors (see CONTRIBUTORS.md).
# Licensed under the Apache License, Version 2.0 (see LICENSE).

from __future__ import (absolute_import, division, generators, nested_scopes, print_function,
unicode_literals, with_statement)

import sys

from pants.engine.subsystem.native import bootstrap_c_source


if __name__ == '__main__':
try:
output_dir = sys.argv[1]
except Exception:
print('usage: {} <output dir>'.format(sys.argv[0]))

bootstrap_c_source(output_dir)
Loading

0 comments on commit 8e24991

Please sign in to comment.