[NO-TICKET] Trim unneeded headers from gem#5
Conversation
These files are not needed by the `datadog` gem and thus can be removed on our fork.
These headers are not currently needed by the `datadog` gem, and thus we can go ahead and remove them. (We can always re-add them later)
We're not using it ATM :)
This is the script I've used to find which headers I could delete.
Strech
left a comment
Strech
left a comment
There was a problem hiding this comment.
Holy-moly AI is taking our jobs! 🚀
|
Questions:
|
|
We discussed this during our 1:1, but I'll write it down for posterity:
If the profiler compiles it should be correct. We can additionally run the profiler test suite to be extra sure, but given how C is a static compiled language, I'd expect any issue with reducing the headers to show up immediately when the profiler gets compiled.
Ideally, we'd need to manually trim new sets of headers in the same way -- e.g, when the 3.4.0 stable headers come out, we'd do the same change; this is why I left the But such a change is actually optional, so we can also chose to just sync with upstream and not do the clean up immediately after (at the cost of shipping a few more files that we need to). |
|
I'll hold on merging this as @TonyCTHsu wanted to try to add some automation to validate these changes. |
| # `find_includes.rb vm_core.h iseq.h ractor_core.h` we get a rough list of headers that are not needed by the | ||
| # datadog gem. Always remember to validate the result -- this tool isn't perfect (for instance it doesn't detect that | ||
| # thread_pthread.h is in use). |
There was a problem hiding this comment.
removing files that are not actually needed by the dd-trace-rb gem to work
This seems a bit backwards: datadog-ruby_core_source is a dependency of dd-trace-rb, but this introduces a circular dependency back from datadog-ruby_core_source to dd-trace-rb: which headers to pass to the tool is dd-trace-rb knowledge.
The mention that the tool isn't perfect† doesn't bode well either. It is also AIUI non-reversible, not telling you that suddenly you need a header that isn't there.
Combined with the huge resulting diff, I am not confident with a merge.
† Given that C99 preprocessing is sort of Turing complete the only way to surefire know is by running a full-blown C preprocessor, like cc -E...
|
Me and @lloeki discussed this PR vs #6 a bit. For SSI, it makes sense to delete as much as possible, so we could even delete all of the header files, since we don't I've extended the comment on
|
There was a problem hiding this comment.
Thanks for adding the comments, it is much more actionable from an external perspective.
For SSI, it makes sense to delete as much as possible, so we could even delete all of the header files,
Indeed as of today without profiling we can go down to hack datadog's gemspec and remove the dependency, or keep the dependency and simply remove all headers post install.
So with such a change this changeset ultimately only benefits non-SSI installs.
since we don't support profiling in SSI. (And the path towards profiling + SSI is probably not going to go through these headers.)
That may change: I believe the requirement for no-compile+no-write when injection happens can - and arguably should - be addressed, even if it means building the C profiling extension per patch (or patch range) version.
While I find the proposal too complex, circular, and tricky to maintain I can't put forward a cogent argument to not merge, especially as this is a "two-way door".
So feel free to go ahead.
| # `#include THREAD_IMPL_H`, with this `THREAD_IMPL_H` being provided by the auto-generated "ruby/config.h" file. | ||
| # Thus, this detection fails for this file. |
There was a problem hiding this comment.
That's where we would need an actual C preprocessor ;)
| # Currently this script should be used as `find_includes.rb vm_core.h iseq.h ractor_core.h`. These headers are the | ||
| # ones included in | ||
| # https://github.com/DataDog/dd-trace-rb/blob/master/ext/datadog_profiling_native_extension/private_vm_api_access.c . |
There was a problem hiding this comment.
Wouldn't find_includes ext/datadog_profiling_native_extension/private_vm_api_access.c work?
Or shouldn't it be made to? (i.e finding the intersection between private_vm_api_access.c includes and the ones provided in this gem)
There was a problem hiding this comment.
It could indeed be made to work with it, although it'd need to be updated to ignore any headers in dd-trace-rb and handle the different folders correctly.
| # Thus, this detection fails for this file. | ||
| # | ||
| # 3. How to validate that there are no missing headers? To validate a set of headers is enough, build dd-trace-rb | ||
| # with those headers on linux/docker (`bundle exec rake clean compile`). If it passes, it's enough! |
There was a problem hiding this comment.
Sounds like a test in this repo would be useful:
- [CI] clone this repo at the current git SHA
- clone DataDog/dd-trace-rb's
masterin sometmpdir - cd into it, hack the Gemfile to point to this gem's cloned dir, and attempt compilation
Note: this is what I'm doing in libv8-node: cloning and building downstream mini_racer, and running a small set of mini_racer tests.
There was a problem hiding this comment.
@TonyCTHsu mentioned wanted taking a stab at it; TBH it would be nice to have that automation, but I was hoping to not block on it (since as usual, I have many other profiler things to work on)
It will benefit SSI too until we do the above delete change; which we haven't done yet ;)
Supporting SSI customers would be great, and I hope to get to it in the short/medium-term. |
7 participants
What does this PR do?
This PR further reduces the size of the gem:
lib/:master: 8.3 MiBivoanjo/clean-unused-files: 4.3 MiBmaster: 1.2 MiBivoanjo/clean-unused-files: 669 KiBIt does this by removing files that are not actually needed by the dd-trace-rb gem to work. I've used the new
find_includes.rbscript + a bit of manual checking to find files that aren't actually needed.I've also tweaked the
.gemspecto exclude a few files that aren't actually needed from the packaged gem.Motivation:
Reduce shipped + installed size of gem.
Additional Notes:
N/A
How to test the change?
I've manually tested the changes by pointing the dd-trace-rb gem to this gem, and making sure that both
bundle exec rake clean compileandbundle exec rspec spec/datadog/profiling/continue to work fine for all the expected Rubies.(It's also possible to use CI to do this, although to speed up testing I only did it locally)