[GitHub] [arrow] westonpace commented on issue #36090: [C++] Excessive increase in debug build times

["westonpace (via GitHub)" <gi...@apache.org>]

westonpace commented on issue #36090:
URL: https://github.com/apache/arrow/issues/36090#issuecomment-1595586144

   I investigated this today and I think I figured out the problem.  When we split out acero into it's own module we ended up introducing `test_util_internal.cc`  I believe this had originally been a part of the arrow testing library but I didn't dig too much into where it came from.
   
   To make it available to all the tests we created an `add_arrow_acero_test` function which had...
   
   ```
     add_arrow_test(${REL_TEST_NAME}
                    EXTRA_LINK_LIBS
                    ${ARROW_ACERO_TEST_LINK_LIBS}
                    PREFIX
                    ${PREFIX}
                    SOURCES
                    test_util_internal.cc
                    LABELS
                    ${LABELS}
                    ${ARG_UNPARSED_ARGUMENTS})
   ```
   
   We also added `test_util_internal.cc` to all of the dataset tests in a similar fashion.
   
   Unfortunately, adding this common utility in this way actually means that the file gets compiled again for every single test.  Since this file is fairly complex it ended up adding quite a bit to the overall compilation time.
   
   The fix is straightforward but I may need some cmake help to figure out the best way to do it in our existing infrastructure.  I'd like to create a small test library for acero that contains `src/arrow/acero/dummy_nodes.cc` and `src/arrow/acero/test_util_internal.cc`.  Also, I'd like to create a small test library for datasets that contains `src/arrow/acero/test_util_internal.cc` and `src/arrow/dataset/test_util_internal.cc`.
   
   I don't believe we want to export these libraries (like we do with arrow_testing).  I think they can just be small static archives that get added as dependencies to all of the tests.
   
   As a hack to test this out I added `test_util_internal.cc` to the list of acero sources.  That appears to restore the build time to what I got in 11.0.0.
   
   I did attempt to use ClangBuildAnalyzer to further reduce compile times.  I didn't find any clear wins.  However, I was able to tune precompiled header files to drop build times by an additional 35%.  In other words, on my system:
   
   Current Build Time: 2100
   After fixing duplicate test files: 1900
   With tuned precompiled headers: 1300
   
   The files I needed to add to the precompiled headers were:
   
    * datum.h - Since Datum is generally passed around by value this is an inevitable include for anything remotely involved with compute.
    * scalar.h - This is another header that was fairly ubiquitous.  However, we may be able to reduce this one with better forward declarations since scalars are typically passed around by reference/pointer.
    * compute/exec.h - This is needed for ExecBatch.  Similar to Datum I think this one is inevitable because ExecBatch is usually passed by value
    * compute/expression.h - Same as the Datum and ExecBatch as Expression is typically passed by value
    * compute/function.h
    * compute/kernel.h
   
   Adding datum/exec/expression to the pch for acero and datasets may be sufficient.  I don't think we need it in the top-level pch.
   
   These last two are not actually included excessively.  However, they are needed by all of the compute kernel files and they are very expensive includes (kernel.h in particular).  We could probably leave them out or, if we really want them, find a way to make a dedicated pch file for just compute kernels.
   
   For the testing pch I needed to add the gmock headers.  gtest/gtest.h isn't actually too bad but the gmock includes are very expensive.
   
   Action items:
   
    - [ ] Figure out how to make internal "common testing" libraries for acero & dataset (maybe @kou can help?)
    - [ ] See if we can reduce the number of times we include scalar.h (@westonpace)
    - [ ] Get `kernel.h` out of the compute api_xyz.h files (these only need FunctionOptions which we can put in its own file) @westonpace 
    - [ ] Consider enabling precompiled headers on builds that don't seem to be getting much benefit from ccache (I can look into this but I don't know much about how well ccache is or isn't working on these CI jobs so I'd appreciate any input) 
   
   I've prototyped some of these tasks in https://github.com/westonpace/arrow/tree/experiment/build-times 
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org