New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Native engine is a stripped cdylib #5557

Merged
merged 1 commit into from Mar 7, 2018

Conversation

Projects
None yet
4 participants
@illicitonion
Copy link
Contributor

illicitonion commented Mar 6, 2018

How

  1. Make our output type a cdylib not a dylib.
  2. Rename the cffi-provided initnative_engine function to
    wrapped_initnative_engine, and create initnative_engine in Rust,
    which calls the C version. This is required because otherwise the
    linker will strip initnative_engine out of the binary.

Why

  1. This is the supported path in rust. The fact that making a dylib
    happened to work before isn't particularly well defined behavior in
    Rust, but was a nice accident.

  2. Binary size. When building a cdylib, the linker can strip dead code,
    and perform LTO. Note that LTO is not enabled in this commit, but
    will be a seprate PR. The following size improvements were observed
    in release mode:
    OSX:

    • dylib (before this change): 16M
    • cdylib (with this change): 2.9M

    Linux:

    • dylib (before this change): 66M
    • cdylib (with this change): 51M
  3. Performance. This allows the toolchain to be much more aggressive
    with optimisations (particularly when we enable LTO in the future).
    Time to list a large repo (2nd invocation, without daemon):
    OSX:

    • Before: 4m29
    • After: 4m12

    Linux:

    • Before: 4m11
    • After: 4m7

Drawbacks

Slows down release mode compilation.
OSX:

  • Before: 323s
  • After: 348s

Linux (on travis, which has somewhat variable performance):

  • Before: 436s
  • After: 556s
@illicitonion

This comment has been minimized.

Copy link
Contributor

illicitonion commented Mar 6, 2018

Depends on #5553

@illicitonion illicitonion referenced this pull request Mar 6, 2018

Merged

Engine is a workspace #5555

@stuhood

stuhood approved these changes Mar 7, 2018

Copy link
Member

stuhood left a comment

Awesome!

@@ -257,33 +257,58 @@ def get_build_cflags():

def bootstrap_c_source(output_dir, module_name=NATIVE_ENGINE_MODULE):
"""Bootstrap an external CFFI C source file."""

tempdir = safe_mkdtemp()

This comment has been minimized.

@stuhood

stuhood Mar 7, 2018

Member

Might consider using src/python/pants/util/contextutil.py def temporary_dir.

This comment has been minimized.

@kwlzn

This comment has been minimized.

@illicitonion

illicitonion Mar 7, 2018

Contributor

Done in dependent PR

@kwlzn

kwlzn approved these changes Mar 7, 2018

# This is an optimistic heuristic for renaming the initnative_engine symbol and nothing else.
if line.startswith(b'init{}'.format(module_name)):
lines[i] = 'wrapped_' + line
file_content = ''.join(lines)

This comment has been minimized.

@kwlzn

kwlzn Mar 7, 2018

Member

how about using a generator inline here to mutate on the fly? e.g.

def transform_file(f):
  with open(f, 'rb') as fh:
    for line in fh:
      if line.startswith(b'init{}'.format(module_name)):
        yield 'wrapped_' + line
      else:
        yield line

file_content = ''.join(transform_file(temp_c_file))

This comment has been minimized.

@illicitonion

illicitonion Mar 7, 2018

Contributor

Done :)

@@ -257,33 +257,58 @@ def get_build_cflags():

def bootstrap_c_source(output_dir, module_name=NATIVE_ENGINE_MODULE):
"""Bootstrap an external CFFI C source file."""

tempdir = safe_mkdtemp()

This comment has been minimized.

@kwlzn
#[no_mangle]
pub extern "C" fn initnative_engine() {
unsafe { wrapped_initnative_engine() }
}

This comment has been minimized.

@kwlzn

kwlzn Mar 7, 2018

Member

clever.. I like it.

@baroquebobcat
Copy link
Contributor

baroquebobcat left a comment

Nice.

LTO is link time optimization, right?

@illicitonion

This comment has been minimized.

Copy link
Contributor

illicitonion commented Mar 7, 2018

LTO is indeed link time optimisation :)

(See #5560 for the PR which enables much more aggressive LTO, which we're probably not actually going to merge)

Native engine is a stripped cdylib
*How*

1. Make our output type a cdylib not a dylib.
2. Rename the cffi-provided `initnative_engine` function to
   `wrapped_initnative_engine`, and create `initnative_engine` in Rust,
   which calls the C version. This is required because otherwise the
   linker will strip `initnative_engine` out of the binary.

*Why*

1. This is the supported path in rust. The fact that making a `dylib`
   happened to work before isn't particularly well defined behavior in
   Rust, but was a nice accident.
2. Binary size. When building a cdylib, the linker can strip dead code,
   and perform LTO. Note that LTO is not enabled in this commit, but
   will be a seprate PR. The following size improvements were observed
   in release mode:
   OSX:
    dylib (before this change): 16M
    cdylib (with this change): 2.9M
   Linux:
    dylib (before this change): 66M
    cdylib (with this change): 51M
3. Performance. This allows the toolchain to be much more aggressive
   with optimisations (particularly when we enable LTO in the future).
   Time to list a large repo (2nd invocation, without daemon):
   OSX:
    Before: 4m29
    After: 4m12
   Linux:
    Before: 4m11
    After: 4m7

*Drawbacks*

Slows down release mode compilation.
OSX:
 Before: 323s
 After: 348s
Linux (on travis, which has somewhat variable performance):
 Before: 436s
 After: 556s

@illicitonion illicitonion force-pushed the twitter:dwagnerhall/cdylib-1 branch from c1ace85 to d7773c3 Mar 7, 2018

@illicitonion illicitonion merged commit c9d56a2 into pantsbuild:master Mar 7, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@illicitonion illicitonion deleted the twitter:dwagnerhall/cdylib-1 branch Apr 27, 2018

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