This is an archive of the discontinued LLVM Phabricator instance.

Don't instantiate a full host toolchain in ASTMatchersTest.
ClosedPublic

Authored by jlebar on Jun 28 2016, 12:00 PM.

Details

Summary

This test was stat()'ing large swaths of /usr/lib hundreds of times, as
every invocation of matchesConditionally*() created a new Linux
toolchain.

In addition to being slow, perf indicated this was causing substantial
contention in the kernel.

Something is...interesting in the kernel, as without this patch I
sometimes see ~11m spent in the kernel, and sometimes ~5m. This
corresponds to bimodal ninja check-clang times of ~30s and ~20s.

It's not clear to me exactly what causes the bimodality. In any case,
this change makes this test run in 2.5s, down from 17s, and it seems to
cause us to get the 20s ninja check-clang time unconditionally.

Diff Detail

Repository
rL LLVM

Event Timeline

jlebar updated this revision to Diff 62123.Jun 28 2016, 12:00 PM
jlebar retitled this revision from to Don't instantiate a full host toolchain in ASTMatchersTest..
jlebar updated this object.
jlebar added a reviewer: chandlerc.
jlebar added a subscriber: cfe-commits.
klimek added inline comments.Jun 28 2016, 12:21 PM
unittests/ASTMatchers/ASTMatchersTest.h
77 ↗(On Diff #62123)

typo: instantiting

81–83 ↗(On Diff #62123)

Couldn't we use -ccc-install-dir to prevent toolchain search?

jlebar added inline comments.Jun 28 2016, 1:00 PM
unittests/ASTMatchers/ASTMatchersTest.h
81–83 ↗(On Diff #62123)

Couldn't we use -ccc-install-dir to prevent toolchain search?

I tried -ccc-install-dir /does/not/exist and it had no speedup relative to the original.

chandlerc added inline comments.Jun 28 2016, 1:12 PM
unittests/ASTMatchers/ASTMatchersTest.h
81–83 ↗(On Diff #62123)

Yea, that just won't help sadly.

As I indicated to Justin, I think the correct fix long-term is to expose a routine that uses CC1 flags directly for use in unittests. That will completely eliminate the need for this. Fundamentally, the unittests shouldn't be reaching through the entire driver, they should be directly running the frontend. There should be specific and dedicated unittests for the driver bits that point the driver at a fake tree instead of the real tree as well.

But I think this is a reasonable workaround until such an API can be provided.

jlebar updated this revision to Diff 62132.Jun 28 2016, 1:21 PM
jlebar marked 3 inline comments as done.

Fix typo in comment.

But I think this is a reasonable workaround until such an API can be provided.

Should I take that as an LG, or are we waiting for someone else to approve this?

chandlerc accepted this revision.Jun 30 2016, 11:17 AM
chandlerc edited edge metadata.

But I think this is a reasonable workaround until such an API can be provided.

Should I take that as an LG, or are we waiting for someone else to approve this?

Lacking a more detailed response from Manuel, yea, I think you should check this in... I wrote most of the toolchain search logic you're turning off so I'm happy to provide an LGTM here. I think this is fine for now, fixes an immediate issue, and can be revisited and improved later easily. So yes, LG.

This revision is now accepted and ready to land.Jun 30 2016, 11:17 AM
This revision was automatically updated to reflect the committed changes.