Deliver libddprof_ffi as a shared library #22

nsavoire · 2022-02-08T13:35:23Z

What does this PR do?

Deliver libddprof_ffi as a shared library
Activate LTO and reduce number of codegen units to 1
Add a simple exporter example executable

Motivation

Some clients prefer having a shared library rather than a static one.

r1viollet · 2022-02-09T10:11:01Z

ddprof-ffi/Cargo.toml

@@ -8,7 +8,9 @@ edition = "2018"
 license = "Apache-2.0"

 [lib]
-crate-type = ["lib", "staticlib"]
+# LTO is ignored if "lib" is added as crate type
+# cf. https://github.com/rust-lang/rust/issues/51009


nice referencing this

r1viollet · 2022-02-09T10:16:47Z

ffi-build.sh

@@ -30,6 +31,7 @@ case "$target" in
    "x86_64-apple-darwin")
        expected_native_static_libs=" -framework Security -liconv -lSystem -lresolv -lc -lm -liconv"
        native_static_libs="${expected_native_static_libs}"
+        shared_library_suffix=".dylib"


Thanks for checking the mac os builds !

examples/ffi/CMakeLists.txt

r1viollet · 2022-02-09T12:25:33Z

Cargo.toml

@@ -14,3 +14,5 @@ debug = 2 # full debug info

 [profile.release]
 debug = 1 # line tables only
+lto = true
+codegen-units = 1


Is this something we want to put in the cargo config, or could we not make sure to have it as a command line argument in the CI ?
I'm thinking we dan't want to slow all of our build times. Only the build time to generate the github release.
WDYT ?

As discussed, locally we will use the develop profile. We can leave the release profile optimised for size.

morrisonlevi · 2022-02-09T17:10:25Z

ffi-build.sh

@@ -46,6 +48,10 @@ sed < ddprof_ffi.pc.in "s/@DDProf_FFI_VERSION@/${version}/g" \
    | sed "s/@DDProf_FFI_LIBRARIES@/${native_static_libs}/g" \
    > "$destdir/lib/pkgconfig/ddprof_ffi.pc"


The concept of "native static libs" doesn't apply to a shared object. There will be dependencies, of course, but this isn't how we should be populating them for the .so.

Thanks for pointing this out. I did put them in Libs.private when I had still hope to be able to make a single pkg-config file for both shared and static library, but since I could not do it, it's better to completely remove them for the shared library.

r1viollet

LGTM !

morrisonlevi · 2022-02-11T17:53:24Z

The shared library artifact for alpine is suspicious, particularly the runpath:

# readelf -d ./libddprof-x86_64-alpine-linux-musl/lib/libddprof_ffi.so

Dynamic section at offset 0x4cf8f8 contains 24 entries:
  Tag        Type                         Name/Value
 0x0000000000000001 (NEEDED)             Shared library: [libgcc_s.so.1]
 0x0000000000000001 (NEEDED)             Shared library: [libc.musl-x86_64.so.1]
 0x000000000000001d (RUNPATH)            Library runpath: [$ORIGIN/:$ORIGIN/../../../../../../../../../../usr/lib/rustlib/x86_64-alpine-linux-musl/lib:/usr/lib/rustlib/x86_64-alpine-linux-musl/lib]

nsavoire · 2022-02-11T20:37:23Z

The shared library artifact for alpine is suspicious, particularly the runpath:

# readelf -d ./libddprof-x86_64-alpine-linux-musl/lib/libddprof_ffi.so

Dynamic section at offset 0x4cf8f8 contains 24 entries:
  Tag        Type                         Name/Value
 0x0000000000000001 (NEEDED)             Shared library: [libgcc_s.so.1]
 0x0000000000000001 (NEEDED)             Shared library: [libc.musl-x86_64.so.1]
 0x000000000000001d (RUNPATH)            Library runpath: [$ORIGIN/:$ORIGIN/../../../../../../../../../../usr/lib/rustlib/x86_64-alpine-linux-musl/lib:/usr/lib/rustlib/x86_64-alpine-linux-musl/lib]

Hum, indeed this is strange. But I don't expect this to be an issue, the runpath here is not useful to find dependencies since libddprof_ffi.so depends only on libgcc/libc.
Adding rpath/runpath to cdylib seems specific to alpine-musl:

We could remove runpath from the lib just to be extra cautious. WDYT @morrisonlevi ?

* Reduce number of codegen units to further reduce ouput library size. * Deliver shared library alongside static library

Create ddprof_ffi-static.pc for linking with static library

…ddprof_ffi-static.pc for static linking

…sary for dynamic linking

Rust adds some weird RPATH/RUNPATH to cdylibs on alpine-musl (cf. https://git.alpinelinux.org/aports/tree/community/rust/alpine-target.patch,https://git.alpinelinux.org/aports/tree/community/rust/need-rpath.patch).

ivoanjo

Thanks for this! I left two tiny notes, and I'll try to wire this up in Ruby and report back with the experience :)

ivoanjo · 2022-02-14T12:22:52Z

examples/ffi/exporter.cpp

+static ddprof_ffi_ByteSlice to_byteslice(const char *s)
+{
+    return {.ptr = (uint8_t *)s,
+            .len = strlen(s)};
+}
+
+static ddprof_ffi_Slice_c_char to_slice_c_char(const char *s)
+{
+    return {.ptr = s,
+            .len = strlen(s)};
+}


Not related to this PR, but this looks to me as a weird sharp edge in the libddprof API.

It's weird to have both ddprof_ffi_ByteSlice and ddprof_ffi_Slice_c_char when they are effectively the same (?).

Also, I had to write similar helpers for using libddprof, so perhaps it would make sense to export these (possibly as macros?) in ffi.h as well :)

You usually don't want to encourage people to transport strings without an actual size associated to it. strlen is also not something you want encourage, so I don't know how I feel about exposing these types of APIs.
I guess we can also mention this :-)

Hmmm... I'm guessing the "don't want to encourage" may be related to performance -- is that the case?

If so, would it make sense to call it something like to_slice_slow? I guess for a bunch of hardcoded strings ("ruby", "wall-clock", "nanoseconds", "service", ...) we just use them once so the cost is trivial so we could have the ease of use instead :)

I agree there is room for improvements concerning the APIs, we could provide some helpers function like to_slice_xxx on top of FFI headers. Maybe even a more friendly C++ interface.

It's strange that slices for type/unit/filename/build_id/name/system_name/... are different from slices used for /tags/filenames.

But this would be subject of another pull request ;)

I don't feel super strongly about this, but I would usually avoid relying on C style null terminated strings. Though it is not clear how we could avoid these conversions at boundaries or how to have a common types.

ffi-build.sh

morrisonlevi

Did we check Alpine builds? I remember the compiler telling me it can't make shared libs on Alpine.

nsavoire · 2022-02-15T16:38:54Z

Yes, there is a test in CI that try linking why shared library.
x86_64-unknown-linux-musl refuses to create cdylibs but x86_64-alpine-linux-musl has specific patches to allow it.

nsavoire requested review from morrisonlevi and r1viollet February 8, 2022 13:35

r1viollet reviewed Feb 9, 2022

View reviewed changes

examples/ffi/CMakeLists.txt Show resolved Hide resolved

r1viollet reviewed Feb 9, 2022

View reviewed changes

morrisonlevi reviewed Feb 9, 2022

View reviewed changes

r1viollet approved these changes Feb 10, 2022

View reviewed changes

nsavoire added 7 commits February 11, 2022 21:42

Build ddprof-ffi as a shared library

0366c81

Add exporter example

1d3b254

Activate LTO

d0575dd

* Reduce number of codegen units to further reduce ouput library size. * Deliver shared library alongside static library

Update license file

9962404

Make ddprof_ffi.pc link with shared library by default

aa16b91

Create ddprof_ffi-static.pc for linking with static library

Make dynamic linking the default for libddprof_ffi.pc and provide lib…

6041f3d

…ddprof_ffi-static.pc for static linking

Remove native static libs from ddprof_ffi.pc since they are not neces…

e1542ff

…sary for dynamic linking

nsavoire force-pushed the nsavoire/deliver_shared_library branch 2 times, most recently from b386c52 to a3e8035 Compare February 14, 2022 10:14

nsavoire added 2 commits February 14, 2022 11:35

Optimize for size instead of speed

282da24

Remove RPATH/RUNPATH from cdylib on alpine-musl

06174b3

Rust adds some weird RPATH/RUNPATH to cdylibs on alpine-musl (cf. https://git.alpinelinux.org/aports/tree/community/rust/alpine-target.patch,https://git.alpinelinux.org/aports/tree/community/rust/need-rpath.patch).

nsavoire force-pushed the nsavoire/deliver_shared_library branch from a3e8035 to 06174b3 Compare February 14, 2022 10:36

ivoanjo reviewed Feb 14, 2022

View reviewed changes

Fix OSX -> macOS

68b2c75

morrisonlevi approved these changes Feb 15, 2022

View reviewed changes

nsavoire merged commit ea737a1 into main Feb 15, 2022

nsavoire deleted the nsavoire/deliver_shared_library branch February 15, 2022 16:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deliver libddprof_ffi as a shared library #22

Deliver libddprof_ffi as a shared library #22

nsavoire commented Feb 8, 2022 •

edited by ivoanjo

Loading

r1viollet Feb 9, 2022

r1viollet Feb 9, 2022

r1viollet Feb 9, 2022

r1viollet Feb 10, 2022

morrisonlevi Feb 9, 2022

nsavoire Feb 9, 2022

r1viollet left a comment

morrisonlevi commented Feb 11, 2022

nsavoire commented Feb 11, 2022

ivoanjo left a comment

ivoanjo Feb 14, 2022

r1viollet Feb 14, 2022

ivoanjo Feb 14, 2022

nsavoire Feb 15, 2022

r1viollet Feb 15, 2022

morrisonlevi left a comment

nsavoire commented Feb 15, 2022

		@@ -46,6 +48,10 @@ sed < ddprof_ffi.pc.in "s/@DDProf_FFI_VERSION@/${version}/g" \
		\| sed "s/@DDProf_FFI_LIBRARIES@/${native_static_libs}/g" \
		> "$destdir/lib/pkgconfig/ddprof_ffi.pc"

Deliver libddprof_ffi as a shared library #22

Deliver libddprof_ffi as a shared library #22

Conversation

nsavoire commented Feb 8, 2022 • edited by ivoanjo Loading

What does this PR do?

Motivation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

r1viollet left a comment

Choose a reason for hiding this comment

morrisonlevi commented Feb 11, 2022

nsavoire commented Feb 11, 2022

ivoanjo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

morrisonlevi left a comment

Choose a reason for hiding this comment

nsavoire commented Feb 15, 2022

nsavoire commented Feb 8, 2022 •

edited by ivoanjo

Loading