Improve frame storage #74
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
This commit changes a few things, first, it gets rid of the random IDs used to identify frames, as the birthday paradox, and production, shows that we'll quickly find duplicates using 32 bits. This now uses a global array. In BPF arrays are zero initialised, so we don't need to do that explicitly. As we are using shared memory across multiple threads, we read + increment the IDs atomically. In addition to these changes that should reduce the chances of collisions and hence of lost frames, also bump the number of frames we can store and reduce the amount of processes we can profile at once, as it's a very high number. We can tune these later on. Notes ===== Other approaches that I've checked for ID generation are to use a per CPU counter and then offset the CPU number. This works but care has to be taken to make this work in systems with an "unbounded" number of CPUs. Not using an atomic get+increment is cheaper, but we would have to call the CPU id helper, so perf wise it's probably in the same ballpark. Another avenue we could explore is to have a generic symbol table to store both method names and paths. This would result in a reduction of stored strings as we wouldn't need to have the combination of path names x directories, but we would waste more space as these two string buffers have different sizes. This commit doesn't fully fix the issue of collisions. A better fix will come later by processing the samples in a streaming fashion. Signed-off-by: Francisco Javier Honduvilla Coto <javierhonduco@gmail.com>
|
@manuelfelipe This should help with #70, let me know if that's not the case 😄 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This commit changes a few things, first, it gets rid of the random IDs used to identify frames, as the birthday paradox, and production, shows that we'll quickly find duplicates using 32 bits. This now uses a global array. In BPF arrays are zero initialised, so we don't need to do that explicitly. As we are using shared memory across multiple threads, we read + increment the IDs atomically.
In addition to these changes that should reduce the chances of collisions and hence of lost frames, also bump the number of frames we can store and reduce the number of processes we can profile at once, as it's a very high number. We can tune these later on.
Notes
Other approaches that I've checked for ID generation are to use a per CPU counter and then offset the CPU number. This works but care has to be taken to make this work in systems with an "unbounded" number of CPUs. Not using an atomic get+increment is cheaper, but we would have to call the CPU id helper, so perf-wise it's probably in the same ballpark.
Another avenue we could explore is to have a generic symbol table to store both method names and paths. This would result in a reduction of stored strings as we wouldn't need to have the combination of path names x directories, but we would waste more space as these two string buffers have different sizes.
This commit doesn't fully fix the issue of collisions. A better fix will come later by processing the samples in a streaming fashion.
Signed-off-by: Francisco Javier Honduvilla Coto javierhonduco@gmail.com