Page MenuHomePhabricator

[modules] Do not cache invalid state for modules that we attempted to load.
ClosedPublic

Authored by vsapsai on Jan 16 2020, 11:58 AM.

Details

Summary

Partially reverts 0a2be46cfdb698fefcc860a56b47dde0884d5335 as it turned
out to cause redundant module rebuilds in multi-process incremental builds.
When a module was getting out of date, all compilation processes started at the
same time were marking it as ToBuild. So each process was building the same
module instead of checking if it was built by someone else and using that
result. In addition to the work duplication, contention on the same .pcm file
wasn't making builds faster.

Note that for a single-process build this change would cause redundant module
reads and validations. But reading a module is faster than building it and
multi-process builds are more common than single-process. So I'm willing to
make such a trade-off.

rdar://problem/54395127

Diff Detail

Event Timeline

vsapsai created this revision.Jan 16 2020, 11:58 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 16 2020, 11:58 AM
vsapsai marked an inline comment as done.Jan 16 2020, 12:13 PM

Anecdotal build time measurements before and after the change. First row is a clean build, subsequent rows are incremental builds.

RevisionBefore changeAfter changeChange (after - before)Relative change
5da385fb56c27:4028:20+40s+2.41%
d4e006e844630:5028:29-141s-7.62%
77d049d0c650:280:280s0%
1b9ef3bbb590:100:11+1s+10%
ab411801b8211:3411:15-19s-2.74%

Cannot claim huge build time improvements but seems like there are no real regressions.

clang/lib/Serialization/ModuleManager.cpp
183–185

Made this change because if we don't have a valid module but opened a corresponding .pcm file earlier, there is a high chance that .pcm file was rebuilt.

Unit tests: pass. 61863 tests passed, 0 failed and 781 were skipped.

clang-tidy: unknown.

clang-format: pass.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

dexonsmith accepted this revision.Jan 16 2020, 12:25 PM

LGTM.

clang/lib/Serialization/ModuleManager.cpp
183–185

Please add a comment in the code explaining that.

This revision is now accepted and ready to land.Jan 16 2020, 12:25 PM
This revision was automatically updated to reflect the committed changes.