Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reduce resource consumption of test compilation by simplifying mock library inclusions #10917

Closed
sunjayBhatia opened this issue Apr 23, 2020 · 17 comments
Assignees

Comments

@sunjayBhatia
Copy link
Member

sunjayBhatia commented Apr 23, 2020

Description:
Many tests/test libraries over-include especially mock libraries which causes increased memory usage, disk usage, and compilation time. This is especially a problem on Windows with cl.exe but we have also observed significant resource usage that can be reduced by as much as half on Linux with clang.

Attached to this issue is a file showing the peak working set memory usage of cl.exe compiling various tests/test libraries (units in KB). We can see many of the tests cause the compiler to use multiple GB of memory, often for simple tests that do not actually require many of the includes they are being built with. We have been able to reduce this usage (see linked PR) by cleaning up these includes. Our primary focus has been enabling faster/less resource intensive compilation of tests in Windows CI but this endeavor seems like it would be relevant to compilation performance on other platforms. We can potentially reduce compilation time, disk usage of artifacts/parameter files, and memory usage of the compiler (by orders of magnitude in some cases, multiple GB reduction in others).

While we have had some success empirically assessing the memory usage of compiling tests and finding the most expensive, it is not super feasible to go one-by-one and manually assess/fix the includes etc. to pare-down each test. We are hoping to potentially get some help reducing the overhead of compiling these tests from other contributors working toward similar changes as in the PR linked below as well as finding a programmatic solution that could potentially be automated (in CI or otherwise) that could identify over-inclusion of mocks and other large libraries that are not used. Other strategies to help solve this could be further breaking down larger libraries with many classes into their component parts so tests only include exactly what interfaces etc. they need.

Relevant Links:

@sunjayBhatia
Copy link
Member Author

cc @wrowe

@sunjayBhatia
Copy link
Member Author

cc @mattklein123 @lizan as you have been working on improving build performance in CI etc.

@mattklein123
Copy link
Member

See #8770. cc also @htuch as we have noted understanding and improving compile performance as a possible intern project for this summer. I would love to make progress on this.

@htuch
Copy link
Member

htuch commented Apr 23, 2020

I think this is also related to getting https://include-what-you-use.org/ working for Envoy. Last time I looked it needed a custom toolchain but I think this has changed.

@ahedberg
Copy link
Contributor

/assign ahedberg

@ahedberg
Copy link
Contributor

/assign foreseeable

@repokitteh-read-only
Copy link

foreseeable cannot be assigned to this issue.

🐱

Caused by: a #10917 (comment) was created by @ahedberg.

see: more, trace.

@ahedberg
Copy link
Contributor

/assign foreseeable

@asraa
Copy link
Contributor

asraa commented Jun 19, 2020

Nice!! I just saw this. I have been thinking about this in the context of our frequent timeouts and OOMs in OSS-Fuzz. I ran a few bazel performance analysis on some fuzz targets and found bottlenecks in server/mocks.cc and in gperftools:

   39.021 s   42.47%   action 'CcConfigureMakeRule external/envoy/bazel/foreign_cc/gperftools_build/include'
   40.090 s   43.63%   action 'Compiling test/mocks/server/mocks.cc'

I had some draft patches splitting up server/mocks.cc as well, and I'm happy that this affects others, as I will happily help review/breakdown mock libraries.

lizan pushed a commit that referenced this issue Jun 25, 2020
…11649)

Commit Message: Split huge monolith mock header to speed up test compilation
Additional Description: `cluster_manager_test` only used a simple mock class `MockAdmin` from `test/mocks/server/mocks.h`, which is a huge mock library. After splitting, the overall build time for `cluster_manager_test` reduced from 143.481s to 82.443s in my build cluster.

Risk Level: low
Testing: existing tests
Docs Changes: N/A
Release Notes: no
Related Issues: #10917 

Signed-off-by: Muge Chen <mugechen@google.com>
chaoqin-li1123 pushed a commit to chaoqin-li1123/envoy that referenced this issue Jun 25, 2020
…nvoyproxy#11649)

Commit Message: Split huge monolith mock header to speed up test compilation
Additional Description: `cluster_manager_test` only used a simple mock class `MockAdmin` from `test/mocks/server/mocks.h`, which is a huge mock library. After splitting, the overall build time for `cluster_manager_test` reduced from 143.481s to 82.443s in my build cluster.

Risk Level: low
Testing: existing tests
Docs Changes: N/A
Release Notes: no
Related Issues: envoyproxy#10917

Signed-off-by: Muge Chen <mugechen@google.com>
Signed-off-by: chaoqinli <chaoqinli@google.com>
songhu pushed a commit to songhu/envoy that referenced this issue Jun 25, 2020
…nvoyproxy#11649)

Commit Message: Split huge monolith mock header to speed up test compilation
Additional Description: `cluster_manager_test` only used a simple mock class `MockAdmin` from `test/mocks/server/mocks.h`, which is a huge mock library. After splitting, the overall build time for `cluster_manager_test` reduced from 143.481s to 82.443s in my build cluster.

Risk Level: low
Testing: existing tests
Docs Changes: N/A
Release Notes: no
Related Issues: envoyproxy#10917 

Signed-off-by: Muge Chen <mugechen@google.com>
lizan pushed a commit that referenced this issue Jul 6, 2020
…n performance (#11797)

Commit Message: breakdown `test/mocks/server/mocks.h` into different mock classes
Additional Description:  `test/mocks/server/mocks.h` is a wide-used mock header included by various test files. However it's very huge and most test files only used a small portion of it. Splitting it up into different mock classes will be helpful to reduce compilation time.

Risk Level: low
Testing: existing tests
Docs Changes: N/A
Release Notes: no
Related Issues: #10917

Signed-off-by: Muge Chen <mugechen@google.com>
alyssawilk pushed a commit that referenced this issue Jul 8, 2020
Commit Message: remove superfluous includes
Additional Description: The monolith mock library mocks/server/mocks.h is included by several tests but never used. Remove them to speed up building phase
Risk Level: low
Testing: existing tests
Docs Changes: N/A
Release Notes: no
Related Issues: #10917

Signed-off-by: Muge Chen <mugechen@google.com>
htuch pushed a commit that referenced this issue Jul 23, 2020
… up test compilation (#12048)

breakdown test/mocks/upstream/mocks.h into different mock classes

test/mocks/upstream/mocks.h is a wide-used mock header included by various test files. However it's very huge and most test files only used a small portion of it. Splitting it up into different mock classes will be helpful to reduce compilation time. (similar to #11797 )

Risk Level: low
Testing: existing tests
Docs Changes: N/A
Release Notes: no
Related Issues: #10917

Signed-off-by: Muge Chen <mugechen@google.com>
yashwant121 pushed a commit to yashwant121/envoy that referenced this issue Jul 24, 2020
…nvoyproxy#11649)

Commit Message: Split huge monolith mock header to speed up test compilation
Additional Description: `cluster_manager_test` only used a simple mock class `MockAdmin` from `test/mocks/server/mocks.h`, which is a huge mock library. After splitting, the overall build time for `cluster_manager_test` reduced from 143.481s to 82.443s in my build cluster.

Risk Level: low
Testing: existing tests
Docs Changes: N/A
Release Notes: no
Related Issues: envoyproxy#10917 

Signed-off-by: Muge Chen <mugechen@google.com>
Signed-off-by: yashwant121 <yadavyashwant36@gmail.com>
jwendell pushed a commit to jwendell/envoy that referenced this issue Jul 24, 2020
Commit Message: remove superfluous includes
Additional Description: The monolith mock library mocks/server/mocks.h is included by several tests but never used. Remove them to speed up building phase
Risk Level: low
Testing: existing tests
Docs Changes: N/A
Release Notes: no
Related Issues: envoyproxy#10917

Signed-off-by: Muge Chen <mugechen@google.com>
@foreseeable
Copy link
Contributor

I used Bazel to generated a few performance profiles for some test after simplifying some mock library inclusions:
https://drive.google.com/file/d/1O8B0mLy-VjNXLRSqquNHYaa7izp4Sk7b/view?usp=sharing

(one can open them from chrome://tracing with chrome)

The performance bottlenecks for those tests before was building the monolithic mock headers. But now I can not figure out what to optimize for next step. Can someone have a look of them? Appreciated.

@lizan
Copy link
Member

lizan commented Jul 29, 2020

@foreseeable Great job. I think we can call this issue done.

For the whole build performance, it would be nice if you can take a look why compiling test.cc take that long time. If you pass -ftime-trace to the compiler (via bazel --copt or --per_file_copt), you should be able to get details of that.

KBaichoo pushed a commit to KBaichoo/envoy that referenced this issue Jul 30, 2020
…/...) (envoyproxy#12046)

Commit Message: refactor header inclusion to speed up building (for test/common/...)
Risk Level: low
Testing: existing tests
Docs Changes: N/A
Release Notes: no
Related Issues: envoyproxy#10917

Signed-off-by: Muge Chen <mugechen@google.com>
Signed-off-by: Kevin Baichoo <kbaichoo@google.com>
KBaichoo pushed a commit to KBaichoo/envoy that referenced this issue Jul 30, 2020
… up test compilation (envoyproxy#12048)

breakdown test/mocks/upstream/mocks.h into different mock classes

test/mocks/upstream/mocks.h is a wide-used mock header included by various test files. However it's very huge and most test files only used a small portion of it. Splitting it up into different mock classes will be helpful to reduce compilation time. (similar to envoyproxy#11797 )

Risk Level: low
Testing: existing tests
Docs Changes: N/A
Release Notes: no
Related Issues: envoyproxy#10917

Signed-off-by: Muge Chen <mugechen@google.com>
Signed-off-by: Kevin Baichoo <kbaichoo@google.com>
scheler pushed a commit to scheler/envoy that referenced this issue Aug 4, 2020
…n performance (envoyproxy#11797)

Commit Message: breakdown `test/mocks/server/mocks.h` into different mock classes
Additional Description:  `test/mocks/server/mocks.h` is a wide-used mock header included by various test files. However it's very huge and most test files only used a small portion of it. Splitting it up into different mock classes will be helpful to reduce compilation time.

Risk Level: low
Testing: existing tests
Docs Changes: N/A
Release Notes: no
Related Issues: envoyproxy#10917

Signed-off-by: Muge Chen <mugechen@google.com>
Signed-off-by: scheler <santosh.cheler@appdynamics.com>
scheler pushed a commit to scheler/envoy that referenced this issue Aug 4, 2020
Commit Message: remove superfluous includes
Additional Description: The monolith mock library mocks/server/mocks.h is included by several tests but never used. Remove them to speed up building phase
Risk Level: low
Testing: existing tests
Docs Changes: N/A
Release Notes: no
Related Issues: envoyproxy#10917

Signed-off-by: Muge Chen <mugechen@google.com>
Signed-off-by: scheler <santosh.cheler@appdynamics.com>
scheler pushed a commit to scheler/envoy that referenced this issue Aug 4, 2020
Commit Message: refactor header inclusion to speed up building
Additional Description:
Risk Level: low
Testing: exsiting tests
Docs Changes: N/A
Release Notes: no
Related Issues: envoyproxy#10917

Signed-off-by: Muge Chen <mugechen@google.com>
Signed-off-by: scheler <santosh.cheler@appdynamics.com>
scheler pushed a commit to scheler/envoy that referenced this issue Aug 4, 2020
…ation/...) (envoyproxy#12053)

Commit Message: refactor header inclusion to speed up building
Additional Description:
Risk Level: low
Testing: exsiting tests
Docs Changes: N/A
Release Notes: no
Related Issues: envoyproxy#10917

Signed-off-by: Muge Chen <mugechen@google.com>
Signed-off-by: scheler <santosh.cheler@appdynamics.com>
scheler pushed a commit to scheler/envoy that referenced this issue Aug 4, 2020
…ions/filters/network/...) (envoyproxy#12051)

Commit Message: refactor header inclusion to speed up building
Additional Description:
Risk Level: low
Testing: exsiting tests
Docs Changes: N/A
Release Notes: no
Related Issues: envoyproxy#10917

Signed-off-by: Muge Chen <mugechen@google.com>
Signed-off-by: scheler <santosh.cheler@appdynamics.com>
scheler pushed a commit to scheler/envoy that referenced this issue Aug 4, 2020
…ions/transport_sockets/...) (envoyproxy#12050)

Commit Message: refactor header inclusion to speed up building
Additional Description:
Risk Level: low
Testing: exsiting tests
Docs Changes: N/A
Release Notes: no
Related Issues: envoyproxy#10917

Signed-off-by: Muge Chen <mugechen@google.com>
Signed-off-by: scheler <santosh.cheler@appdynamics.com>
scheler pushed a commit to scheler/envoy that referenced this issue Aug 4, 2020
…ions/filters/http) (envoyproxy#12047)

Commit Message: refactor header inclusion to speed up building
Additional Description:
Risk Level: low
Testing: exsiting tests
Docs Changes: N/A
Release Notes: no
Related Issues: envoyproxy#10917

Signed-off-by: Muge Chen <mugechen@google.com>
Signed-off-by: scheler <santosh.cheler@appdynamics.com>
scheler pushed a commit to scheler/envoy that referenced this issue Aug 4, 2020
…ions/[upstream|common|tracer]/...) (envoyproxy#12049)

Commit Message: refactor header inclusion to speed up building
Additional Description:
Risk Level: low
Testing: exsiting tests
Docs Changes: N/A
Release Notes: no
Related Issues: envoyproxy#10917

Signed-off-by: Muge Chen <mugechen@google.com>
Signed-off-by: scheler <santosh.cheler@appdynamics.com>
scheler pushed a commit to scheler/envoy that referenced this issue Aug 4, 2020
…/...) (envoyproxy#12045)

Commit Message: refactor header inclusion to speed up building (for test/server/...)
Additional Description:
Risk Level: low
Testing: existing tests
Docs Changes: N/A
Release Notes: no
Related Issues: envoyproxy#10917

Signed-off-by: Muge Chen <mugechen@google.com>
Signed-off-by: scheler <santosh.cheler@appdynamics.com>
scheler pushed a commit to scheler/envoy that referenced this issue Aug 4, 2020
…/...) (envoyproxy#12046)

Commit Message: refactor header inclusion to speed up building (for test/common/...)
Risk Level: low
Testing: existing tests
Docs Changes: N/A
Release Notes: no
Related Issues: envoyproxy#10917

Signed-off-by: Muge Chen <mugechen@google.com>
Signed-off-by: scheler <santosh.cheler@appdynamics.com>
chaoqin-li1123 pushed a commit to chaoqin-li1123/envoy that referenced this issue Aug 7, 2020
… up test compilation (envoyproxy#12048)

breakdown test/mocks/upstream/mocks.h into different mock classes

test/mocks/upstream/mocks.h is a wide-used mock header included by various test files. However it's very huge and most test files only used a small portion of it. Splitting it up into different mock classes will be helpful to reduce compilation time. (similar to envoyproxy#11797 )

Risk Level: low
Testing: existing tests
Docs Changes: N/A
Release Notes: no
Related Issues: envoyproxy#10917

Signed-off-by: Muge Chen <mugechen@google.com>
Signed-off-by: chaoqinli <chaoqinli@google.com>
@sunjayBhatia
Copy link
Member Author

@foreseeable Great job. I think we can call this issue done.

Closing this as per this comment, if this needs to be reopened, I can do so

htuch pushed a commit that referenced this issue Aug 28, 2020
We need to reduce resource consumption of test compilation by simplifying mock library inclusions. #10917
One way to do that is to break the monolithic mock headers into different mock classes and then refactor the code base (like #12053 #12051 #11797)
It's hard to refactor them manually. So I wrote headersplit, a tool based on libclang and python to help me divide the monolithic mock header and replace includes after dividing automatically. This tool can also generate building time comparison between before/after refactoring.

Risk level: low
Testing: Build succeeds.

Signed-off-by: Muge Chen <mugechen@google.com>
clarakosi pushed a commit to clarakosi/envoy that referenced this issue Sep 3, 2020
)

We need to reduce resource consumption of test compilation by simplifying mock library inclusions. envoyproxy#10917
One way to do that is to break the monolithic mock headers into different mock classes and then refactor the code base (like envoyproxy#12053 envoyproxy#12051 envoyproxy#11797)
It's hard to refactor them manually. So I wrote headersplit, a tool based on libclang and python to help me divide the monolithic mock header and replace includes after dividing automatically. This tool can also generate building time comparison between before/after refactoring.

Risk level: low
Testing: Build succeeds.

Signed-off-by: Muge Chen <mugechen@google.com>
Signed-off-by: Clara Andrew-Wani <candrewwani@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants