Remove unnecessary includes #6060

tautschnig · 2021-04-27T21:59:04Z

Manual removal of includes based on include-what-you-use's output,
filtered for includes that should be removed.

A fully automated use of include-what-you-use doesn't seem feasible at this time for there are a number of false positives of includes that iwyu claims can be safely removed.

Each commit message has a non-empty body, explaining why the change was made.
n/a Methods or procedures I have added are documented, following the guidelines provided in CODING_STANDARD.md.
n/a The feature or user visible behaviour I have added or modified has been documented in the User Guide in doc/cprover-manual/
Regression or unit tests are included, or existing tests cover the modified code (in this case I have detailed which ones those are in the commit message).
My commit message includes data points confirming performance improvements (if claimed).
My PR is restricted to a single feature or bugfix.
White-space or formatting changes outside the feature-related changed lines are in commits of their own.

codecov · 2021-04-27T23:45:29Z

Codecov Report

Merging #6060 (450845d) into develop (1f0d7b3) will increase coverage by 0.21%.
The diff coverage is 94.73%.

@@             Coverage Diff             @@
##           develop    #6060      +/-   ##
===========================================
+ Coverage    74.17%   74.39%   +0.21%     
===========================================
  Files         1446     1447       +1     
  Lines       157784   157787       +3     
===========================================
+ Hits        117036   117383     +347     
+ Misses       40748    40404     -344

Impacted Files	Coverage Δ
jbmc/src/janalyzer/janalyzer_main.cpp	`100.00% <ø> (ø)`
jbmc/src/java_bytecode/assignments_from_json.cpp	`97.26% <ø> (ø)`
jbmc/src/java_bytecode/ci_lazy_methods.cpp	`99.47% <ø> (ø)`
jbmc/src/java_bytecode/ci_lazy_methods.h	`100.00% <ø> (ø)`
jbmc/src/java_bytecode/ci_lazy_methods_needed.cpp	`98.24% <ø> (ø)`
jbmc/src/java_bytecode/ci_lazy_methods_needed.h	`100.00% <ø> (ø)`
jbmc/src/java_bytecode/convert_java_nondet.cpp	`81.15% <ø> (ø)`
...java_bytecode/create_array_with_type_intrinsic.cpp	`97.67% <ø> (ø)`
jbmc/src/java_bytecode/expr2java.cpp	`86.61% <ø> (ø)`
jbmc/src/java_bytecode/expr2java.h	`100.00% <ø> (ø)`
... and 705 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 56adbb2...450845d. Read the comment docs.

martin-cs

I am really glad this PR exists and even more glad that wasn't the one to have to write it! I think it is definitely a good thing and should reduce build times. Do you have a rough number for how much time it saves? A few concerns:

Not sure why the coverage metrics drop. Is it counting forward declarations of classes as executable code?
This has a very "breaks all existing PRs" feel to it. What, if anything, are we going to do about that?
Having gone to all of this trouble, is there anything we could add to the CI / review process to avoid accumulating unnecessary includes and forward declarations? Could we have some kind of 'try to remove #include lines from the PR and see if it still builds' task. If it build develop first and then just tried a loop of removing lines adding headers from the diff, applying and seeing if it still builds?

martin-cs · 2021-05-02T09:55:11Z

src/goto-cc/gcc_message_handler.cpp

@@ -8,7 +8,9 @@ Author: Daniel Kroening, [email protected]

 #include "gcc_message_handler.h"

-#include <util/unicode.h>
+#ifdef _MSC_VER


This seems to be an idiom. Would it be better to have the ifdef guard in util/unicode.h?

Well, it's really just the use of widen (in most of those files) that's required with Visual Studio only. The header provides more functionality than that, so I don't think that's really an option.

tautschnig · 2021-05-02T11:17:46Z

I am really glad this PR exists and even more glad that wasn't the one to have to write it! I think it is definitely a good thing and should reduce build times. Do you have a rough number for how much time it saves? A few concerns:

I've now added data to the commit message. Builds seem approximately 7% faster.

1. Not sure why the coverage metrics drop.  Is it counting forward declarations of `class`es as executable code?

I have no idea, and the Codecov page doesn't seem to be working at the moment. It seems that quite often with code removal the coverage metrics appear to drop by a tiny fraction, which is weird.

2. This has a very "breaks all existing PRs" feel to it.  What, if anything, are we going to do about that?

Yes, there may be an impact on several PRs, but I don't expect that many actually: most of the includes simply weren't necessary, and when PRs change implementations only and not header files then most likely they're not impacted at all as an #include may have been shifted from the header file to the cpp file.

3. Having gone to all of this trouble, is there anything we could add to the CI / review process to avoid accumulating unnecessary includes and forward declarations?  Could we have some kind of 'try to remove #include lines from the PR and see if it still builds' task.  If it build develop first and then just tried a loop of removing lines adding headers from the diff, applying and seeing if it still builds?

I do think that we could use more recent versions of include-what-you-use in CI, perhaps 8.0 (which is in Ubuntu 20.04) is quite reasonable already. I've worked with 5.0 (from Ubuntu 18.04), which wasn't too great. The issue mainly is around detecting use of types in templates. I'll create a separate PR to play with this (also need to figure out how long it takes to run).

chrisr-diffblue

Wow - thats a lot of work... 👍

Manual removal of includes based on include-what-you-use's output, filtered for includes that should be removed. The goal is to avoid unnecessary build dependencies, reducing the amount of code that needs to be rebuilt during incremental builds. The build time for fresh rebuilds decreases by about 7% as the following experiment shows: ``` rm -rf build && ccache -C cmake -H. -Bbuild -G Ninja \ -DCMAKE_BUILD_TYPE=Release \ -DCMAKE_EXE_LINKER_FLAGS="-fuse-ld=lld -Wl,--threads" /usr/bin/time -v ninja -C build -j32 ``` Repeated execution for both develop and this branch producing the following data, which less than 1s of variation across multiple runs: develop: ``` Command being timed: "ninja -C build -j32" User time (seconds): 2714.02 System time (seconds): 203.75 Percent of CPU this job got: 2833% Elapsed (wall clock) time (h:mm:ss or m:ss): 1:42.96 ``` this PR: ``` Command being timed: "ninja -C build -j32" User time (seconds): 2507.14 System time (seconds): 189.95 Percent of CPU this job got: 2822% Elapsed (wall clock) time (h:mm:ss or m:ss): 1:35.56 ```

tautschnig added the cleanup label Apr 27, 2021

tautschnig self-assigned this Apr 27, 2021

tautschnig force-pushed the includes-cleanup branch 16 times, most recently from 54d6fc8 to d524f42 Compare May 1, 2021 19:40

tautschnig removed their assignment May 1, 2021

tautschnig marked this pull request as ready for review May 1, 2021 19:42

tautschnig requested review from allredj, chrisr-diffblue, kroening, martin-cs, peterschrammel, pkesseli, romainbrenguier and smowton as code owners May 1, 2021 19:42

peterschrammel approved these changes May 1, 2021

View reviewed changes

tautschnig assigned kroening May 1, 2021

kroening approved these changes May 1, 2021

View reviewed changes

tautschnig assigned martin-cs and unassigned kroening May 1, 2021

tautschnig force-pushed the includes-cleanup branch 2 times, most recently from 4b186f5 to 4e8601f Compare May 1, 2021 23:00

martin-cs approved these changes May 2, 2021

View reviewed changes

tautschnig assigned chrisr-diffblue and unassigned martin-cs May 2, 2021

tautschnig force-pushed the includes-cleanup branch from 4e8601f to 70b704b Compare May 2, 2021 11:17

tautschnig force-pushed the includes-cleanup branch from 70b704b to c7a5e26 Compare May 6, 2021 14:48

chrisr-diffblue approved these changes May 6, 2021

View reviewed changes

tautschnig added 2 commits May 6, 2021 20:39

fixup! Implement __builtin_ctz{,l,ll} via count_trailing_zeros_exprt

e2af83f

tautschnig force-pushed the includes-cleanup branch from c7a5e26 to 450845d Compare May 6, 2021 20:39

tautschnig assigned tautschnig and unassigned chrisr-diffblue May 6, 2021

tautschnig merged commit 1ab5de1 into diffblue:develop May 7, 2021

tautschnig deleted the includes-cleanup branch May 7, 2021 06:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Remove unnecessary includes #6060

Remove unnecessary includes #6060

Uh oh!

tautschnig commented Apr 27, 2021 •

edited

Loading

Uh oh!

codecov bot commented Apr 27, 2021 •

edited

Loading

Uh oh!

martin-cs left a comment

Uh oh!

martin-cs May 2, 2021

Uh oh!

tautschnig May 2, 2021

Uh oh!

tautschnig commented May 2, 2021

Uh oh!

chrisr-diffblue left a comment

Uh oh!

Uh oh!

Remove unnecessary includes #6060

Remove unnecessary includes #6060

Uh oh!

Conversation

tautschnig commented Apr 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Apr 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

martin-cs left a comment

Choose a reason for hiding this comment

Uh oh!

martin-cs May 2, 2021

Choose a reason for hiding this comment

Uh oh!

tautschnig May 2, 2021

Choose a reason for hiding this comment

Uh oh!

tautschnig commented May 2, 2021

Uh oh!

chrisr-diffblue left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tautschnig commented Apr 27, 2021 •

edited

Loading

codecov bot commented Apr 27, 2021 •

edited

Loading