make collider header-only #871
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #871 +/- ##
==========================================
- Coverage 91.84% 90.05% -1.79%
==========================================
Files 37 64 +27
Lines 4976 9161 +4185
Branches 0 950 +950
==========================================
+ Hits 4570 8250 +3680
- Misses 406 911 +505 ☔ View full report in Codecov by Sentry. |
|
Huh, so the total time is the time for each CPU added together? Seems like wall clock time is what we care about, right? So if this is making total time 15% better, but wall clock time 30-50% worse, is that good? You also mentioned making collider into a header - are you still planning to do that? What difference would it make? I've never heard of a Unity build before, but it looks like it's just concatenating several cpp files for compilation (I sort of thought that would take more, rather than less time, but goes to show how much I know about the compilation process). Still, another reason I like a bunch of separate files is that generally re-compiling is quicker, since I'm often only touching one of those files. How does this change affect re-compilation? |
|
Yes, total time is the time for each CPU added together. I think it depends on what we are optimizing for, either the developer time or CI time, or both (some devs may have less beefy machines with less cores, idk). Unity build is not really meant for development, it is for optimizing one-off compilation. And yes, it makes recompilation pretty slow. I did it just because I want to see how much a different it makes, and I am happy to drop that change. I still plan to make collider into header, will see how it affects compile time. I don't expect there to be a huge increase in compile time because it is still small comparing to things like stl algorithms. We can usually optimize compiler frontend time (parsing) by using PCH. I don't think these functions will be instantiated many times (< 10 across the whole library I think), so backend time should be fine. |
|
Okay, that sounds reasonable. Let's see how it looks without the Unity part. I think I'd rather optimize for dev experience than for the CI. |
|
OK I checked, no measurable compile time difference: 20.2s for header-only collider, 19.5s for the current version. |
|
And I found that we should not expose collider to users: it uses some of our internal APIs like |
|
the cmake error can probably be fixed by the build interface thing, will check later tmr |
|
@elalish should we merge this now? |
Not very sure about this one, I guess this depends on how far we want to go for optimizing compile time.
I wanted to optimize some collider related performance issue, which requires passing functions inside the collider loop. However, I recall that the reason we put collider into
.cppfile instead of using templates in the header function is due to compile time concern, which was pretty painful when we compiled using nvcc. We don't use nvcc now, and I wanted to optimize the build time a bit more so we can have more headroom to make it slower :)Changes: