Re: First-time contributor -- speeding up `std::unique_ptr` compilation

[Jonathan Wakely <jwakely.gcc@gmail.com>]

On Fri, 24 Jun 2022, 07:20 Jonathan Wakely, <jwakely.gcc@gmail.com> wrote:

>
>
> On Fri, 24 Jun 2022, 01:19 , <mail@vittorioromeo.com> wrote:
>
>> Hello everyone,
>> Hope you are all doing well.
>>
>> I would like to begin by saying that I am terribly unfamiliar with
>> mailing lists and I've always found them cumbersome to use -- I tend to do
>> most of my development via GitHub PRs and Discord. Therefore, please
>> forgive me if this is not the right place to discuss "how to contribute",
>> and also forgive me for not being able to create a proper patch file.
>>
>> My goal is to generally improve the compilation speed of C++ programs
>> using the standard library. In personal projects and open-source projects I
>> maintain, I've achieved good results by benchmarking compilation via
>> ClangBuildAnalyzer, figuring out the bottlenecks, and optimizing them.
>> While working on the SFML project, I noticed that the `std::unique_ptr`
>> template was taking a lot of time to be instantiated using the latest
>> version of libstdc++.
>>
>> I investigated a bit, and figured out that the `<tuple>` include and
>> `std::tuple` instantiation were the culprit. I then replaced them with a
>> handmade workaround in my system libraries, recompiled SFML, and noticed
>> that `std::unique_ptr`'s instantiation time became negligible as a result
>> of my changes. As a side benefit, we also avoid including the '<tuple>'
>> header altogether, reducing the risk of unintended transitive includes. I
>> created a PR on the GCC GitHub mirror as a way to observe the diff in a
>> nice interface and to discuss my changes, and to see if the maintainers
>> generally think that this is a good idea. You can see the diff here:
>> https://github.com/gcc-mirror/gcc/pull/67/files?diff=split&w=1
>>
>> In terms of rough measurements, the average instantiation time of
>> `std::unique_ptr` went from 22ms to 4ms. This was measured over the entire
>> SFML codebase using clang++ 14.x with the `-ftime-trace` flag, plus the
>> aforementioned ClangBuildAnalyzer tool.
>>
>> A few questions:
>>
>>   1.  Is this improvement something that the libstdc++ maintainers would
>> want to merge in? Of course, the changes need to be cleaned up and adapted
>> to GCC's coding style.
>>
>
> No, it would be a ABI break. It would change the layout for a deleter with
> the 'final' specifier.
>
> It also looks like quite a maintenance headache "just" for improved
> compilation time. The need for two implementations (with and without the
> attribute) that both need to be kept ABI compatible with std::tuple doesn't
> seem like a good trade off and a good use of maintainer effort.
>

I think a more maintainable (and ABI compatible) approach would be to move
the _Head_base templates to a new header and include that in
bits/unique_ptr.h, without the rest of <tuple>. That will also allow the
gdb pretty printers to continue working, whereas your suggestion would mean
rewriting them.

I know you are concerned about build times, but correctness, ABI
compatibility, and to a lesser extent maintainability all matter more.
Build times for microbenchmarks can be worth checking, but bear in mind
that your changes actually mean *more* code needs to be compiled for a
program that already uses std::tuple anyway. You're not removing any code
from the library or making the existing tuple code faster to compile, only
adding more code.




>
>   2.  If so, what is the easiest way to create a patch in the format that
>> you are used to and test it against libstdc++'s test suite? I have never
>> contributed to libstdc++ or GCC before.
>>
>
You need to test it first, before you create a patch. Build your modified
sources as per https://gcc.gnu.org/wiki/InstallingGCC (but starting with
your git clone instead of a release tarball).

You can use --enable-languages=c++ to avoid building more of the compiler
then you need for libstdc++ testing.

In the libstdc++ build directory (e.g. x86_64-pc-linux-gnu/libstdc++-v3)
run make check to run the tests, adding -j8 or a suitable option for your
number of cores.

See https://gcc.gnu.org/onlinedocs/libstdc++/manual/test.html#test.run for
testing docs.

Once you're happy with the results see https://gcc.gnu.org/contribute.html
for patch submission.


>
>