llvm dev - Dec 2017 - Who wants faster LLVM/Clang builds?

Hi,

Recently I've done some experiments on the LLVM/Clang code and discovered
that many of our source files often include unnecessary header files. I wrote a
simple tool that eliminates redundant includes and estimates benefits of doing
it, and the results were quite nice: for some files we were able to save 90% of
compile time! I think we want to apply some of the cleanups I found, but I'm
not sure how to better do it: the total patches are 8k lines of code for LLVM
and 3k lines of code for clang (I'll attach them for reference). My
suggestion would be that people take a look at the list of changed files and
pick the changes for the piece of code they are working on if the changes look
sane (the changes do need some checking before committing). Does it sound like a
good idea? I'd appreciate any feedback on what can we do here.

The list of files for which removing redundant headers improved compile time
(the numbers are compile time in seconds for a Debug build):

LLVM top 10
Filename						Old	New	Delta
lib/CodeGen/GlobalISel/GlobalISel.cpp			0.26	0.02	-91.9%
lib/MC/MCLabel.cpp					0.19	0.02	-88.2%
tools/llvm-readobj/ObjDumper.cpp			0.43	0.10	-76.5%
lib/MC/MCWinEH.cpp					0.51	0.13	-74.3%
lib/Transforms/Vectorize/Vectorize.cpp			0.72	0.29	-59.7%
tools/llvm-diff/DiffLog.cpp				0.58	0.26	-54.6%
lib/Target/ARM/MCTargetDesc/ARMMachORelocationInfo.cpp	0.46	0.26	-44.1%
lib/DebugInfo/DWARF/DWARFExpression.cpp			0.68	0.38	-43.3%
lib/LTO/LTOModule.cpp					2.25	1.33	-41.1%
lib/Target/TargetMachine.cpp				1.76	1.10	-37.8%

Full list:



Clang top 10
Filename						Old	New	Delta
tools/libclang/CXString.cpp				1.70	0.25	-85.2%
lib/Tooling/CommonOptionsParser.cpp			1.69	0.55	-67.3%
lib/AST/StmtViz.cpp					1.02	0.44	-57.4%
tools/driver/cc1_main.cpp				2.26	0.97	-57.1%
unittests/CodeGen/BufferSourceTest.cpp			3.08	1.83	-40.6%
lib/CodeGen/CGLoopInfo.cpp				1.91	1.34	-29.9%
unittests/Tooling/RefactoringActionRulesTest.cpp	2.46	1.79	-27.0%
unittests/CodeGen/CodeGenExternalTest.cpp		3.43	2.52	-26.5%
tools/libclang/CXStoredDiagnostic.cpp			1.67	1.26	-24.8%
tools/clang-func-mapping/ClangFnMapGen.cpp		2.48	1.89	-23.8%

Full list:


The corresponding patches (careful, they are big):


Methodology
My tool took the compile_commands.json from LLVM build and  iterated over files
trying to remove redundant headers. To find which header files could be removed
it scanned the file for "#include" lines and tried to remove them one
by one (checking if the file still compiles after the removal). When there were
no more include lines to remove, we verified the change with ninja+ninja check.
After it we compared preprocessed file size before and after the change hoping
to see that it dropped and then checked the compile time impact.
NB: As a side effect of this approach we removed all include-lines from inactive
"ifdef" sections, which means that the patches *will* break other
configurations if applied as-is.

Thanks,
Michael
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20171205/981e50d6/attachment-0005.html>
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: llvm.txt
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20171205/981e50d6/attachment-0002.txt>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20171205/981e50d6/attachment-0006.html>
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: clang.txt
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20171205/981e50d6/attachment-0003.txt>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20171205/981e50d6/attachment-0007.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: llvm_redundant_headers.patch
Type: application/octet-stream
Size: 293711 bytes
Desc: not available
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20171205/981e50d6/attachment-0002.obj>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20171205/981e50d6/attachment-0008.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: clang_redundant_headers.patch
Type: application/octet-stream
Size: 117203 bytes
Desc: not available
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20171205/981e50d6/attachment-0003.obj>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20171205/981e50d6/attachment-0009.html>

I, for one, want faster builds.

Beyond that though, this seems like obvious goodness to reduce coupling in the
codebase.  I’ve only skimmed the patch, but this seems like a clearly amazingly
great ideas.  Did you use the IWYU tool or something else?

-Chris

> On Dec 5, 2017, at 3:40 PM, Mikhail Zolotukhin via llvm-dev <llvm-dev at
lists.llvm.org> wrote:
> 
> Hi,
> 
> Recently I've done some experiments on the LLVM/Clang code and
discovered that many of our source files often include unnecessary header files.
I wrote a simple tool that eliminates redundant includes and estimates benefits
of doing it, and the results were quite nice: for some files we were able to
save 90% of compile time! I think we want to apply some of the cleanups I found,
but I'm not sure how to better do it: the total patches are 8k lines of code
for LLVM and 3k lines of code for clang (I'll attach them for reference). My
suggestion would be that people take a look at the list of changed files and
pick the changes for the piece of code they are working on if the changes look
sane (the changes do need some checking before committing). Does it sound like a
good idea? I'd appreciate any feedback on what can we do here.
> 
> The list of files for which removing redundant headers improved compile
time (the numbers are compile time in seconds for a Debug build):
> 
> LLVM top 10
> Filename						Old	New	Delta
> lib/CodeGen/GlobalISel/GlobalISel.cpp			0.26	0.02	-91.9%
> lib/MC/MCLabel.cpp					0.19	0.02	-88.2%
> tools/llvm-readobj/ObjDumper.cpp			0.43	0.10	-76.5%
> lib/MC/MCWinEH.cpp					0.51	0.13	-74.3%
> lib/Transforms/Vectorize/Vectorize.cpp			0.72	0.29	-59.7%
> tools/llvm-diff/DiffLog.cpp				0.58	0.26	-54.6%
> lib/Target/ARM/MCTargetDesc/ARMMachORelocationInfo.cpp	0.46	0.26	-44.1%
> lib/DebugInfo/DWARF/DWARFExpression.cpp			0.68	0.38	-43.3%
> lib/LTO/LTOModule.cpp					2.25	1.33	-41.1%
> lib/Target/TargetMachine.cpp				1.76	1.10	-37.8%
> 
> Full list:
> <llvm.txt>
> 
> 
> Clang top 10
> Filename						Old	New	Delta
> tools/libclang/CXString.cpp				1.70	0.25	-85.2%
> lib/Tooling/CommonOptionsParser.cpp			1.69	0.55	-67.3%
> lib/AST/StmtViz.cpp					1.02	0.44	-57.4%
> tools/driver/cc1_main.cpp				2.26	0.97	-57.1%
> unittests/CodeGen/BufferSourceTest.cpp			3.08	1.83	-40.6%
> lib/CodeGen/CGLoopInfo.cpp				1.91	1.34	-29.9%
> unittests/Tooling/RefactoringActionRulesTest.cpp	2.46	1.79	-27.0%
> unittests/CodeGen/CodeGenExternalTest.cpp		3.43	2.52	-26.5%
> tools/libclang/CXStoredDiagnostic.cpp			1.67	1.26	-24.8%
> tools/clang-func-mapping/ClangFnMapGen.cpp		2.48	1.89	-23.8%
> 
> Full list:
> <clang.txt>
> 
> The corresponding patches (careful, they are big):
> <llvm_redundant_headers.patch>
> <clang_redundant_headers.patch>
> 
> Methodology
> My tool took the compile_commands.json from LLVM build and  iterated over
files trying to remove redundant headers. To find which header files could be
removed it scanned the file for "#include" lines and tried to remove
them one by one (checking if the file still compiles after the removal). When
there were no more include lines to remove, we verified the change with
ninja+ninja check. After it we compared preprocessed file size before and after
the change hoping to see that it dropped and then checked the compile time
impact.
> NB: As a side effect of this approach we removed all include-lines from
inactive "ifdef" sections, which means that the patches *will* break
other configurations if applied as-is.
> 
> Thanks,
> Michael
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20171205/25a4a40f/attachment.html>

> To find which header files could be removed it scanned the file for
"#include" lines and tried to remove them one by one (checking if the
file
still compiles after the removal). When there were no more include lines to
remove, we verified the change with ninja+ninja check.

It looks like this makes us rely heavily on transitive header includes --
is that right?

Specifically what I mean is, suppose foo.cc uses class1 and class2.
class2.h #includes "class1.h".  If foo.cc had a #include for class1.h
and
class2.h, this tool will remove the include of class1.h.  Is that right?

If so, seems like probably more aggressive than we want to be, because it
makes our codebase fragile: If someone removes class1.h from class2.h, it
may break foo.cc.

I'm sure we can find many situations like this today, but this approach
seems it would make that situation much more common.

IWYU would avoid this pitfall, although I know IWYU has its own problems.
Alternatively maybe if the tool only removed headers whose removal
significantly decreased the preprocessed size of the file, we'd know that
we actually removed "something interesting".

-Justin

On Tue, Dec 5, 2017 at 9:38 PM Chris Lattner via llvm-dev <
llvm-dev at lists.llvm.org> wrote:
> I, for one, want faster builds.
>
> Beyond that though, this seems like obvious goodness to reduce coupling in
> the codebase.  I’ve only skimmed the patch, but this seems like a clearly
> amazingly great ideas.  Did you use the IWYU tool or something else?
>
> -Chris
>
>
> On Dec 5, 2017, at 3:40 PM, Mikhail Zolotukhin via llvm-dev <
> llvm-dev at lists.llvm.org> wrote:
>
> Hi,
>
> Recently I've done some experiments on the LLVM/Clang code and
discovered
> that many of our source files often include unnecessary header files. I
> wrote a simple tool that eliminates redundant includes and estimates
> benefits of doing it, and the results were quite nice: for some files we
> were able to save 90% of compile time! I think we want to apply some of the
> cleanups I found, but I'm not sure how to better do it: the total
patches
> are 8k lines of code for LLVM and 3k lines of code for clang (I'll
attach
> them for reference). My suggestion would be that people take a look at the
> list of changed files and pick the changes for the piece of code they are
> working on if the changes look sane (the changes do need some checking
> before committing). Does it sound like a good idea? I'd appreciate any
> feedback on what can we do here.
>
> The list of files for which removing redundant headers improved compile
> time (the numbers are compile time in seconds for a Debug build):
>
> *LLVM top 10*
> *Filename Old New Delta*
> lib/CodeGen/GlobalISel/GlobalISel.cpp 0.26 0.02 -91.9%
> lib/MC/MCLabel.cpp 0.19 0.02 -88.2%
> tools/llvm-readobj/ObjDumper.cpp 0.43 0.10 -76.5%
> lib/MC/MCWinEH.cpp 0.51 0.13 -74.3%
> lib/Transforms/Vectorize/Vectorize.cpp 0.72 0.29 -59.7%
> tools/llvm-diff/DiffLog.cpp 0.58 0.26 -54.6%
> lib/Target/ARM/MCTargetDesc/ARMMachORelocationInfo.cpp 0.46 0.26 -44.1%
> lib/DebugInfo/DWARF/DWARFExpression.cpp 0.68 0.38 -43.3%
> lib/LTO/LTOModule.cpp 2.25 1.33 -41.1%
> lib/Target/TargetMachine.cpp 1.76 1.10 -37.8%
>
> Full list:
>
> <llvm.txt>
>
>
>
> *Clang top 10*
> *Filename Old New Delta*
> tools/libclang/CXString.cpp 1.70 0.25 -85.2%
> lib/Tooling/CommonOptionsParser.cpp 1.69 0.55 -67.3%
> lib/AST/StmtViz.cpp 1.02 0.44 -57.4%
> tools/driver/cc1_main.cpp 2.26 0.97 -57.1%
> unittests/CodeGen/BufferSourceTest.cpp 3.08 1.83 -40.6%
> lib/CodeGen/CGLoopInfo.cpp 1.91 1.34 -29.9%
> unittests/Tooling/RefactoringActionRulesTest.cpp 2.46 1.79 -27.0%
> unittests/CodeGen/CodeGenExternalTest.cpp 3.43 2.52 -26.5%
> tools/libclang/CXStoredDiagnostic.cpp 1.67 1.26 -24.8%
> tools/clang-func-mapping/ClangFnMapGen.cpp 2.48 1.89 -23.8%
>
> Full list:
>
> <clang.txt>
>
>
> The corresponding patches (careful, they are big):
>
> <llvm_redundant_headers.patch>
> <clang_redundant_headers.patch>
>
>
> *Methodology*
> My tool took the compile_commands.json from LLVM build and  iterated over
> files trying to remove redundant headers. To find which header files could
> be removed it scanned the file for "#include" lines and tried to
remove
> them one by one (checking if the file still compiles after the removal).
> When there were no more include lines to remove, we verified the change
> with ninja+ninja check. After it we compared preprocessed file size before
> and after the change hoping to see that it dropped and then checked the
> compile time impact.
> NB: As a side effect of this approach we removed all include-lines from
> inactive "ifdef" sections, which means that the patches **will**
break
> other configurations if applied as-is.
>
> Thanks,
> Michael
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20171206/52155011/attachment.html>

> On Dec 5, 2017, at 9:38 PM, Chris Lattner <clattner at nondot.org>
wrote:
> 
> I, for one, want faster builds.
Good, we have at least two people on board then :)
> 
> Beyond that though, this seems like obvious goodness to reduce coupling in
the codebase.  I’ve only skimmed the patch, but this seems like a clearly
amazingly great ideas.  Did you use the IWYU tool or something else?
I tried using it, but while it gave me some interesting hints, I stopped using
it when the build broke after the proposed changes. Probably, I could’ve figured
out what went wrong and made it work, but I also noticed that the proposed by
IWYU changes are much more intrusive - i.e. it tries to forward declare symbols,
analyze include chains and leave only the last include etc (and it only would
work if one applies the changes to all affected files at once). While these all
are good ideas, I'd expect some objections against mechanical application of
such clean-ups. Plus the patch would be much less obvious.

So, instead, I implemented a light-weight version of it that just tries to
remove #include lines, making the footprint of this cleanup local. As a result,
here I get a patch consisting of many independent changes (it can be applied per
file and everything should work fine). More details of how that was done is in
“Methodology” section in the end of the original e-mail.

Thanks,
Michael
> 
> -Chris
> 
> 
>> On Dec 5, 2017, at 3:40 PM, Mikhail Zolotukhin via llvm-dev
<llvm-dev at lists.llvm.org <mailto:llvm-dev at lists.llvm.org>>
wrote:
>> 
>> Hi,
>> 
>> Recently I've done some experiments on the LLVM/Clang code and
discovered that many of our source files often include unnecessary header files.
I wrote a simple tool that eliminates redundant includes and estimates benefits
of doing it, and the results were quite nice: for some files we were able to
save 90% of compile time! I think we want to apply some of the cleanups I found,
but I'm not sure how to better do it: the total patches are 8k lines of code
for LLVM and 3k lines of code for clang (I'll attach them for reference). My
suggestion would be that people take a look at the list of changed files and
pick the changes for the piece of code they are working on if the changes look
sane (the changes do need some checking before committing). Does it sound like a
good idea? I'd appreciate any feedback on what can we do here.
>> 
>> The list of files for which removing redundant headers improved compile
time (the numbers are compile time in seconds for a Debug build):
>> 
>> LLVM top 10
>> Filename						Old	New	Delta
>> lib/CodeGen/GlobalISel/GlobalISel.cpp			0.26	0.02	-91.9%
>> lib/MC/MCLabel.cpp					0.19	0.02	-88.2%
>> tools/llvm-readobj/ObjDumper.cpp			0.43	0.10	-76.5%
>> lib/MC/MCWinEH.cpp					0.51	0.13	-74.3%
>> lib/Transforms/Vectorize/Vectorize.cpp			0.72	0.29	-59.7%
>> tools/llvm-diff/DiffLog.cpp				0.58	0.26	-54.6%
>> lib/Target/ARM/MCTargetDesc/ARMMachORelocationInfo.cpp	0.46	0.26	-44.1%
>> lib/DebugInfo/DWARF/DWARFExpression.cpp			0.68	0.38	-43.3%
>> lib/LTO/LTOModule.cpp					2.25	1.33	-41.1%
>> lib/Target/TargetMachine.cpp				1.76	1.10	-37.8%
>> 
>> Full list:
>> <llvm.txt>
>> 
>> 
>> Clang top 10
>> Filename						Old	New	Delta
>> tools/libclang/CXString.cpp				1.70	0.25	-85.2%
>> lib/Tooling/CommonOptionsParser.cpp			1.69	0.55	-67.3%
>> lib/AST/StmtViz.cpp					1.02	0.44	-57.4%
>> tools/driver/cc1_main.cpp				2.26	0.97	-57.1%
>> unittests/CodeGen/BufferSourceTest.cpp			3.08	1.83	-40.6%
>> lib/CodeGen/CGLoopInfo.cpp				1.91	1.34	-29.9%
>> unittests/Tooling/RefactoringActionRulesTest.cpp	2.46	1.79	-27.0%
>> unittests/CodeGen/CodeGenExternalTest.cpp		3.43	2.52	-26.5%
>> tools/libclang/CXStoredDiagnostic.cpp			1.67	1.26	-24.8%
>> tools/clang-func-mapping/ClangFnMapGen.cpp		2.48	1.89	-23.8%
>> 
>> Full list:
>> <clang.txt>
>> 
>> The corresponding patches (careful, they are big):
>> <llvm_redundant_headers.patch>
>> <clang_redundant_headers.patch>
>> 
>> Methodology
>> My tool took the compile_commands.json from LLVM build and  iterated
over files trying to remove redundant headers. To find which header files could
be removed it scanned the file for "#include" lines and tried to
remove them one by one (checking if the file still compiles after the removal).
When there were no more include lines to remove, we verified the change with
ninja+ninja check. After it we compared preprocessed file size before and after
the change hoping to see that it dropped and then checked the compile time
impact.
>> NB: As a side effect of this approach we removed all include-lines from
inactive "ifdef" sections, which means that the patches *will* break
other configurations if applied as-is.
>> 
>> Thanks,
>> Michael
>> _______________________________________________
>> LLVM Developers mailing list
>> llvm-dev at lists.llvm.org <mailto:llvm-dev at lists.llvm.org>
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
> 
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20171205/fa71d219/attachment.html>

On 5 Dec 2017, at 23:40, Mikhail Zolotukhin via llvm-dev <llvm-dev at
lists.llvm.org> wrote:
> 
> The list of files for which removing redundant headers improved compile
time (the numbers are compile time in seconds for a Debug build):
A few comments:

First, your old and new numbers are giving time to a hundredth of a second, yet
I generally see a variation of 0.1-0.2 seconds between runs.  How many times did
you run each one and what is the Welch’s t-test delta?

Second, most of the changes are a few tenths of a second.  From the whole of
your top 10, it looks as if you’re shaving around 3 seconds off the total build.
Presumably this is CPU time, so on a -j8 parallel build you’re saving under a
second for something that takes a few minutes.  That doesn’t mean it’s not worth
doing, but it does mean that if it has any impact on code maintainability then
it probably isn’t.

Third, it’s not clear from your measurements whether you were doing a modules
build or not.  The cost of redundant includes should be a lot lower if we’re
using modules.

Finally, redundant includes can have a counterintuitive positive benefit on
overall build times by warming the buffer cache (pulling in an include early
when the dependency graph for the build is wide lets another compile job proceed
while you’re waiting for disk, whereas pulling it in later when you’ve exhausted
the parallelism in the build process causes a synchronous stall).  Did you
measure the deltas in overall build time for a cold and warm disk cache after
making these changes?

David

It's also likely that a lot of '#include "foo.h"' can be
replaced with
'class foo;'

Especially in the transitive inclusion case, instead of removing the
#include entirely.


On Wed, Dec 6, 2017 at 8:38 AM, Chris Lattner via llvm-dev <
llvm-dev at lists.llvm.org> wrote:
> I, for one, want faster builds.
>
> Beyond that though, this seems like obvious goodness to reduce coupling in
> the codebase.  I’ve only skimmed the patch, but this seems like a clearly
> amazingly great ideas.  Did you use the IWYU tool or something else?
>
> -Chris
>
>
> On Dec 5, 2017, at 3:40 PM, Mikhail Zolotukhin via llvm-dev <
> llvm-dev at lists.llvm.org> wrote:
>
> Hi,
>
> Recently I've done some experiments on the LLVM/Clang code and
discovered
> that many of our source files often include unnecessary header files. I
> wrote a simple tool that eliminates redundant includes and estimates
> benefits of doing it, and the results were quite nice: for some files we
> were able to save 90% of compile time! I think we want to apply some of the
> cleanups I found, but I'm not sure how to better do it: the total
patches
> are 8k lines of code for LLVM and 3k lines of code for clang (I'll
attach
> them for reference). My suggestion would be that people take a look at the
> list of changed files and pick the changes for the piece of code they are
> working on if the changes look sane (the changes do need some checking
> before committing). Does it sound like a good idea? I'd appreciate any
> feedback on what can we do here.
>
> The list of files for which removing redundant headers improved compile
> time (the numbers are compile time in seconds for a Debug build):
>
> *LLVM top 10*
> *Filename Old New Delta*
> lib/CodeGen/GlobalISel/GlobalISel.cpp 0.26 0.02 -91.9%
> lib/MC/MCLabel.cpp 0.19 0.02 -88.2%
> tools/llvm-readobj/ObjDumper.cpp 0.43 0.10 -76.5%
> lib/MC/MCWinEH.cpp 0.51 0.13 -74.3%
> lib/Transforms/Vectorize/Vectorize.cpp 0.72 0.29 -59.7%
> tools/llvm-diff/DiffLog.cpp 0.58 0.26 -54.6%
> lib/Target/ARM/MCTargetDesc/ARMMachORelocationInfo.cpp 0.46 0.26 -44.1%
> lib/DebugInfo/DWARF/DWARFExpression.cpp 0.68 0.38 -43.3%
> lib/LTO/LTOModule.cpp 2.25 1.33 -41.1%
> lib/Target/TargetMachine.cpp 1.76 1.10 -37.8%
>
> Full list:
> <llvm.txt>
>
>
> *Clang top 10*
> *Filename Old New Delta*
> tools/libclang/CXString.cpp 1.70 0.25 -85.2%
> lib/Tooling/CommonOptionsParser.cpp 1.69 0.55 -67.3%
> lib/AST/StmtViz.cpp 1.02 0.44 -57.4%
> tools/driver/cc1_main.cpp 2.26 0.97 -57.1%
> unittests/CodeGen/BufferSourceTest.cpp 3.08 1.83 -40.6%
> lib/CodeGen/CGLoopInfo.cpp 1.91 1.34 -29.9%
> unittests/Tooling/RefactoringActionRulesTest.cpp 2.46 1.79 -27.0%
> unittests/CodeGen/CodeGenExternalTest.cpp 3.43 2.52 -26.5%
> tools/libclang/CXStoredDiagnostic.cpp 1.67 1.26 -24.8%
> tools/clang-func-mapping/ClangFnMapGen.cpp 2.48 1.89 -23.8%
>
> Full list:
> <clang.txt>
>
> The corresponding patches (careful, they are big):
> <llvm_redundant_headers.patch>
> <clang_redundant_headers.patch>
>
> *Methodology*
> My tool took the compile_commands.json from LLVM build and  iterated over
> files trying to remove redundant headers. To find which header files could
> be removed it scanned the file for "#include" lines and tried to
remove
> them one by one (checking if the file still compiles after the removal).
> When there were no more include lines to remove, we verified the change
> with ninja+ninja check. After it we compared preprocessed file size before
> and after the change hoping to see that it dropped and then checked the
> compile time impact.
> NB: As a side effect of this approach we removed all include-lines from
> inactive "ifdef" sections, which means that the patches **will**
break
> other configurations if applied as-is.
>
> Thanks,
> Michael
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
>
>
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20171206/499de5d9/attachment.html>

> On Dec 6, 2017, at 12:45 AM, David Chisnall via llvm-dev <llvm-dev at
lists.llvm.org> wrote:
> 
> On 5 Dec 2017, at 23:40, Mikhail Zolotukhin via llvm-dev <llvm-dev at
lists.llvm.org> wrote:
>> 
>> The list of files for which removing redundant headers improved compile
time (the numbers are compile time in seconds for a Debug build):
> 
> A few comments:
> 
> First, your old and new numbers are giving time to a hundredth of a second,
yet I generally see a variation of 0.1-0.2 seconds between runs.  How many times
did you run each one and what is the Welch’s t-test delta?
> 
> Second, most of the changes are a few tenths of a second.  From the whole
of your top 10, it looks as if you’re shaving around 3 seconds off the total
build.  Presumably this is CPU time, so on a -j8 parallel build you’re saving
under a second for something that takes a few minutes.  That doesn’t mean it’s
not worth doing, but it does mean that if it has any impact on code
maintainability then it probably isn’t.
I’m not arguing against any of your points, but also keep in mind that reducing
#includes can reduce the number of files built in an incremental build when you
change a header.

-Chris

> 
> Third, it’s not clear from your measurements whether you were doing a
modules build or not.  The cost of redundant includes should be a lot lower if
we’re using modules.
> 
> Finally, redundant includes can have a counterintuitive positive benefit on
overall build times by warming the buffer cache (pulling in an include early
when the dependency graph for the build is wide lets another compile job proceed
while you’re waiting for disk, whereas pulling it in later when you’ve exhausted
the parallelism in the build process causes a synchronous stall).  Did you
measure the deltas in overall build time for a cold and warm disk cache after
making these changes?
> 
> David
> 
> 
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

> On Dec 6, 2017, at 12:45 AM, David Chisnall <David.Chisnall at
cl.cam.ac.uk> wrote:
> 
> On 5 Dec 2017, at 23:40, Mikhail Zolotukhin via llvm-dev <llvm-dev at
lists.llvm.org> wrote:
>> 
>> The list of files for which removing redundant headers improved compile
time (the numbers are compile time in seconds for a Debug build):
> 
Thanks for the comments! I agree with all the points you brought up, and
actually I thought about most of them before, please find my comments below:
> A few comments:
> 
> First, your old and new numbers are giving time to a hundredth of a second,
yet I generally see a variation of 0.1-0.2 seconds between runs.  How many times
did you run each one and what is the Welch’s t-test delta?
I measured the time 5 times for both ‘old’ and ‘new’ versions and took the
minimum. I don’t have variance and other statistics at hand now, but I’m pretty
confident in the ballpark of the numbers. I.e. If the number goes from 1.7s to
1.1s, I’m pretty sure there is a win (even if the ‘true’ numbers would be 1.6
and 1.2 respectively).
> 
> Second, most of the changes are a few tenths of a second.  From the whole
of your top 10, it looks as if you’re shaving around 3 seconds off the total
build.  Presumably this is CPU time, so on a -j8 parallel build you’re saving
under a second for something that takes a few minutes.  That doesn’t mean it’s
not worth doing, but it does mean that if it has any impact on code
maintainability then it probably isn’t.
Correct. That’s exactly why I aimed for small local changes that would be a
strict improvement over what we have now. Also, while the effect on the total
build time isn’t big, incremental builds can see much more significant gains
from it.
> Third, it’s not clear from your measurements whether you were doing a
modules build or not.  The cost of redundant includes should be a lot lower if
we’re using modules.
No, I didn’t do modules, and yes, you are correct (and this is another argument
for probably not spending more efforts here).
> 
> Finally, redundant includes can have a counterintuitive positive benefit on
overall build times by warming the buffer cache (pulling in an include early
when the dependency graph for the build is wide lets another compile job proceed
while you’re waiting for disk, whereas pulling it in later when you’ve exhausted
the parallelism in the build process causes a synchronous stall).  Did you
measure the deltas in overall build time for a cold and warm disk cache after
making these changes?
No, I didn’t measure it with cold and warm caches. For the entire build (-j8) I
usually saw a win of ~1%, but that number is even more noisier, so I didn’t
report it. However, I’m a bit sceptic about keeping redundant includes to
prefetch them to cache. There is no guarantee that a file with the redundant
include will be compiled first, and if it’s compiled after we’re just wasting
our time. There are also other files that actually need this header and will
move it to the cache before our use, so all in all I don’t think the positive
effect of having redundant includes outweigh its cost.

Thanks,
Michael
> 
> David
> 
>

By the way, have you tried -fmodules ?

Takumi

On Wed, Dec 6, 2017 at 1:41 PM Mikhail Zolotukhin via llvm-dev <
llvm-dev at lists.llvm.org> wrote:
> Hi,
>
> Recently I've done some experiments on the LLVM/Clang code and
discovered
> that many of our source files often include unnecessary header files. I
> wrote a simple tool that eliminates redundant includes and estimates
> benefits of doing it, and the results were quite nice: for some files we
> were able to save 90% of compile time! I think we want to apply some of the
> cleanups I found, but I'm not sure how to better do it: the total
patches
> are 8k lines of code for LLVM and 3k lines of code for clang (I'll
attach
> them for reference). My suggestion would be that people take a look at the
> list of changed files and pick the changes for the piece of code they are
> working on if the changes look sane (the changes do need some checking
> before committing). Does it sound like a good idea? I'd appreciate any
> feedback on what can we do here.
>
> The list of files for which removing redundant headers improved compile
> time (the numbers are compile time in seconds for a Debug build):
>
> *LLVM top 10*
> *Filename Old New Delta*
> lib/CodeGen/GlobalISel/GlobalISel.cpp 0.26 0.02 -91.9%
> lib/MC/MCLabel.cpp 0.19 0.02 -88.2%
> tools/llvm-readobj/ObjDumper.cpp 0.43 0.10 -76.5%
> lib/MC/MCWinEH.cpp 0.51 0.13 -74.3%
> lib/Transforms/Vectorize/Vectorize.cpp 0.72 0.29 -59.7%
> tools/llvm-diff/DiffLog.cpp 0.58 0.26 -54.6%
> lib/Target/ARM/MCTargetDesc/ARMMachORelocationInfo.cpp 0.46 0.26 -44.1%
> lib/DebugInfo/DWARF/DWARFExpression.cpp 0.68 0.38 -43.3%
> lib/LTO/LTOModule.cpp 2.25 1.33 -41.1%
> lib/Target/TargetMachine.cpp 1.76 1.10 -37.8%
>
> Full list:
>
>
> *Clang top 10*
> *Filename Old New Delta*
> tools/libclang/CXString.cpp 1.70 0.25 -85.2%
> lib/Tooling/CommonOptionsParser.cpp 1.69 0.55 -67.3%
> lib/AST/StmtViz.cpp 1.02 0.44 -57.4%
> tools/driver/cc1_main.cpp 2.26 0.97 -57.1%
> unittests/CodeGen/BufferSourceTest.cpp 3.08 1.83 -40.6%
> lib/CodeGen/CGLoopInfo.cpp 1.91 1.34 -29.9%
> unittests/Tooling/RefactoringActionRulesTest.cpp 2.46 1.79 -27.0%
> unittests/CodeGen/CodeGenExternalTest.cpp 3.43 2.52 -26.5%
> tools/libclang/CXStoredDiagnostic.cpp 1.67 1.26 -24.8%
> tools/clang-func-mapping/ClangFnMapGen.cpp 2.48 1.89 -23.8%
>
> Full list:
>
> The corresponding patches (careful, they are big):
>
> *Methodology*
> My tool took the compile_commands.json from LLVM build and  iterated over
> files trying to remove redundant headers. To find which header files could
> be removed it scanned the file for "#include" lines and tried to
remove
> them one by one (checking if the file still compiles after the removal).
> When there were no more include lines to remove, we verified the change
> with ninja+ninja check. After it we compared preprocessed file size before
> and after the change hoping to see that it dropped and then checked the
> compile time impact.
> NB: As a side effect of this approach we removed all include-lines from
> inactive "ifdef" sections, which means that the patches **will**
break
> other configurations if applied as-is.
>
> Thanks,
> Michael
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20171212/75a2fdeb/attachment.html>

> On Dec 11, 2017, at 7:14 PM, NAKAMURA Takumi <geek4civic at
gmail.com> wrote:
> 
> By the way, have you tried -fmodules ?
No, I haven’t. Do you have any specific experiment in mind that you wanted to
try?

Thanks,
Michael
> 
> Takumi
> 
> On Wed, Dec 6, 2017 at 1:41 PM Mikhail Zolotukhin via llvm-dev <llvm-dev
at lists.llvm.org <mailto:llvm-dev at lists.llvm.org>> wrote:
> Hi,
> 
> Recently I've done some experiments on the LLVM/Clang code and
discovered that many of our source files often include unnecessary header files.
I wrote a simple tool that eliminates redundant includes and estimates benefits
of doing it, and the results were quite nice: for some files we were able to
save 90% of compile time! I think we want to apply some of the cleanups I found,
but I'm not sure how to better do it: the total patches are 8k lines of code
for LLVM and 3k lines of code for clang (I'll attach them for reference). My
suggestion would be that people take a look at the list of changed files and
pick the changes for the piece of code they are working on if the changes look
sane (the changes do need some checking before committing). Does it sound like a
good idea? I'd appreciate any feedback on what can we do here.
> 
> The list of files for which removing redundant headers improved compile
time (the numbers are compile time in seconds for a Debug build):
> 
> LLVM top 10
> Filename						Old	New	Delta
> lib/CodeGen/GlobalISel/GlobalISel.cpp			0.26	0.02	-91.9%
> lib/MC/MCLabel.cpp					0.19	0.02	-88.2%
> tools/llvm-readobj/ObjDumper.cpp			0.43	0.10	-76.5%
> lib/MC/MCWinEH.cpp					0.51	0.13	-74.3%
> lib/Transforms/Vectorize/Vectorize.cpp			0.72	0.29	-59.7%
> tools/llvm-diff/DiffLog.cpp				0.58	0.26	-54.6%
> lib/Target/ARM/MCTargetDesc/ARMMachORelocationInfo.cpp	0.46	0.26	-44.1%
> lib/DebugInfo/DWARF/DWARFExpression.cpp			0.68	0.38	-43.3%
> lib/LTO/LTOModule.cpp					2.25	1.33	-41.1%
> lib/Target/TargetMachine.cpp				1.76	1.10	-37.8%
> 
> Full list:
> 
> 
> Clang top 10
> Filename						Old	New	Delta
> tools/libclang/CXString.cpp				1.70	0.25	-85.2%
> lib/Tooling/CommonOptionsParser.cpp			1.69	0.55	-67.3%
> lib/AST/StmtViz.cpp					1.02	0.44	-57.4%
> tools/driver/cc1_main.cpp				2.26	0.97	-57.1%
> unittests/CodeGen/BufferSourceTest.cpp			3.08	1.83	-40.6%
> lib/CodeGen/CGLoopInfo.cpp				1.91	1.34	-29.9%
> unittests/Tooling/RefactoringActionRulesTest.cpp	2.46	1.79	-27.0%
> unittests/CodeGen/CodeGenExternalTest.cpp		3.43	2.52	-26.5%
> tools/libclang/CXStoredDiagnostic.cpp			1.67	1.26	-24.8%
> tools/clang-func-mapping/ClangFnMapGen.cpp		2.48	1.89	-23.8%
> 
> Full list:
> 
> The corresponding patches (careful, they are big):
> 
> Methodology
> My tool took the compile_commands.json from LLVM build and  iterated over
files trying to remove redundant headers. To find which header files could be
removed it scanned the file for "#include" lines and tried to remove
them one by one (checking if the file still compiles after the removal). When
there were no more include lines to remove, we verified the change with
ninja+ninja check. After it we compared preprocessed file size before and after
the change hoping to see that it dropped and then checked the compile time
impact.
> NB: As a side effect of this approach we removed all include-lines from
inactive "ifdef" sections, which means that the patches *will* break
other configurations if applied as-is.
> 
> Thanks,
> Michael
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org <mailto:llvm-dev at lists.llvm.org>
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
<http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20171212/79b27fa9/attachment.html>