This repository was archived by the owner on Jun 28, 2022. It is now read-only.
[PROF-4780] Breaking change: Change FFI File struct to contain a Buffer instead of a ByteSlice#33
Merged
Merged
Conversation
…er instead of a ByteSlice In #30 we added the `ddprof_ffi_Buffer_from_byte_slice` function to allow FFI users to provide their own data to be reported using the `ProfileExporterV3` (via `ddprof_ffi_ProfileExporterV3_build`). Thus, if one wanted to report some data, it first converted it from a `ByteSlice` into a `Buffer`, and then invoke `libddprof` with it. Here's a (simplified) example from the Ruby profiler: ```c ddprof_ffi_File files[1]; ddprof_ffi_Slice_file slice_files = {.ptr = files, .len = 1}; ddprof_ffi_Buffer *pprof_buffer = ddprof_ffi_Buffer_from_byte_slice((ddprof_ffi_ByteSlice) { .ptr = /* byte array with data we want to send */, .len = /* ... */ }); files[0] = (ddprof_ffi_File) {.name = /* ... */, .file = pprof_buffer}; ddprof_ffi_Request *request = ddprof_ffi_ProfileExporterV3_build(exporter, start, finish, slice_files, timeout_milliseconds); ddprof_ffi_Buffer_free(pprof_buffer); ``` This approach had a few downsides: 1. It copied the data to be reported twice. It would be first copied into a `Buffer`, and then inside `ddprof_ffi_ProfileExporterV3_build` it would be copied again. 2. **Callers manually needed to clean up the `Buffer` afterwards using `ddprof_ffi_Buffer_free`**. After discussing this with @morrisonlevi, we decided to go the other way: change the `File` to contain a `ByteSlice` directly. This avoids the extra copy in (1.), as well as the caller needing to manage the `Buffer` objects manually in (2.). This is a breaking API change for libddprof FFI users.
nsavoire
reviewed
Mar 9, 2022
Minor cleanup as suggested during review. Co-authored-by: Nicolas Savoire <[email protected]>
ivoanjo
added a commit
to DataDog/dd-trace-rb
that referenced
this pull request
Mar 10, 2022
See DataDog/libddprof#33 for details.
morrisonlevi
approved these changes
Mar 10, 2022
morrisonlevi
left a comment
Collaborator
There was a problem hiding this comment.
Good work! After merging this in, can you see if it causes any conflicts with #32 and fix them for Paul if so? If not, ping me and I can do it.
ivoanjo
added a commit
to DataDog/dd-trace-rb
that referenced
this pull request
May 16, 2022
See DataDog/libddprof#33 for details.
ivoanjo
added a commit
to DataDog/dd-trace-rb
that referenced
this pull request
May 17, 2022
See DataDog/libddprof#33 for details.
ivoanjo
added a commit
to DataDog/dd-trace-rb
that referenced
this pull request
May 19, 2022
See DataDog/libddprof#33 for details.
ivoanjo
added a commit
to DataDog/dd-trace-rb
that referenced
this pull request
May 25, 2022
See DataDog/libddprof#33 for details.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do? + Motivation
In #30 we added the
ddprof_ffi_Buffer_from_byte_slicefunction to allow FFI users to provide their own data to be reported using theProfileExporterV3(viaddprof_ffi_ProfileExporterV3_build).Thus, if one wanted to report some data, it first converted it from a
ByteSliceinto aBuffer, and then invokelibddprofwith it.Here's a (simplified) example from the Ruby profiler:
This approach had a few downsides:
It copied the data to be reported twice. It would be first copied into a
Buffer, and then insideddprof_ffi_ProfileExporterV3_buildit would be copied again.Callers manually needed to clean up the
Bufferafterwards usingddprof_ffi_Buffer_free.After discussing this with @morrisonlevi, we decided to go the other way: change the
Fileto contain aByteSlicedirectly.This avoids the extra copy in (1.), as well as the caller needing to manage the
Bufferobjects manually in (2.).This is a breaking API change for libddprof FFI users.
How to test the change?
Validate that any FFI user (such as
examples/ffi/exporter.cpp) can still successfully report data after the update.I've validated locally with the Ruby profiler (PR incoming at some point);
I could not buildexamples/ffi/exporter.cppdue to unrelated reasons -- I'll tackle it separately from this PR.Update: Pushed a couple of commits to update
exporter.cpp, and confirmed it still works fine.