This repository has been archived by the owner on Jun 28, 2022. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 2
[PROF-4780] Breaking change: Change FFI File struct to contain a Buffer instead of a ByteSlice #33
Merged
Conversation
This file contains 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
…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 <nicolas.savoire@datadoghq.com>
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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_slice
function 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
ByteSlice
into aBuffer
, and then invokelibddprof
with 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_build
it would be copied again.Callers manually needed to clean up the
Buffer
afterwards usingddprof_ffi_Buffer_free
.After discussing this with @morrisonlevi, we decided to go the other way: change the
File
to contain aByteSlice
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.
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.cpp
due 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.