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

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

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.
rsmith added inline comments.Wed, Mar 4, 9:40 AM
clang/lib/Serialization/ModuleManager.cpp
183

This change is proving really bad for us. This prevents using mmap for loading module files, and instead forces the entire file to be loaded every time. Please revert.

dexonsmith added inline comments.Wed, Mar 4, 10:17 AM
clang/lib/Serialization/ModuleManager.cpp
183

Can we limit the revert to explicit vs. implicit module builds? The scenario Volodymyr was defending against is implicit-only.

vsapsai marked an inline comment as done.Wed, Mar 4, 4:19 PM
vsapsai added inline comments.
clang/lib/Serialization/ModuleManager.cpp
183

Richard, can you please tell what is your modules configuration and how you are invoking clang? For implicit builds loading a module instead of mmap is still better than building a module. But for explicit modules I see there is no need to use isVolatile as modules aren't changing all the time.

dblaikie added inline comments.Thu, Mar 5, 1:35 PM
clang/lib/Serialization/ModuleManager.cpp
183

Richard/Google use explicit modules, if that's the particular parameter you're asking about.

So, yes, for Google's needs a solution that allows mmap to continue to be used for explicit modules would suffice, I believe.

(not to in any way derail that from being addressed - I thought implicit modules weren't race-y - they're hashed, etc, so they shouldn't collide/be overwritten? Seems like a loss in performance to have to copy the whole thing into memory rather than using mmap, if that could be avoided by more precise hashing/collision avoidance?)

vsapsai marked an inline comment as done.Thu, Mar 5, 7:42 PM
vsapsai added inline comments.
clang/lib/Serialization/ModuleManager.cpp
183

I need a specific command-line invocation to test with. If tests in clang/test/Modules/explicit* are relevant to Google's setup, I can use those as an example. It's just they were touched last time in 2016.

To discuss racy-ness, that is kinda inherent problem for incremental builds. If a module becomes invalid, all of its users will try to get an up-to-date version of it. And they can try to build that module themselves or check and load already rebuilt module. Unfortunately, I don't see how different hashing can help with it, after all multiple compiler invocations do want the same .pcm file.

rsmith added inline comments.Thu, Mar 5, 9:18 PM
clang/lib/Serialization/ModuleManager.cpp
183

Preventing the use of mmap seems unacceptable in general -- for either implicit or explicit modules. Anything that forces compile time to be linear in the total size of transitive modules used is not reasonable. To really see the problems here you'll need a real world testcase with thousands to tens of thousands of modules, which I can't reasonably give you.

There should be ways we can avoid problems with racyness. Perhaps we could switch to a content-addressed module file store?
In any case, this patch has introduced a regression, so we should revert this change while we work out what to do.