This is an archive of the discontinued LLVM Phabricator instance.

[clang][lex] Avoid `DirectoryLookup` copies
ClosedPublic

Authored by jansvoboda11 on Oct 15 2022, 8:43 AM.

Details

Summary

This patch fixes a performance regression introduced in D121685 that was caused by copying DirectoryLookup.

rdar://101206790

Diff Detail

Event Timeline

jansvoboda11 created this revision.Oct 15 2022, 8:43 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 15 2022, 8:43 AM
jansvoboda11 requested review of this revision.Oct 15 2022, 8:43 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 15 2022, 8:43 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

The change looks good to me. And based on the scope of D121685 it should be sufficient. Do you have any ideas for testing? If it would require significant testing infrastructure work, we can do that separately. But if you have some simple testing ideas in mind, that would be worth adding.

We could invoke clang with the -stats option and compare the result against the expected number of stat calls.

We could invoke clang with the -stats option and compare the result against the expected number of stat calls.

The -stats-file= option only tells you how many times Clang requested DirectoryEntry object from FileManager (i.e. calling FileManager::getDirectory(StringRef)). This function only touches the real file system once and caches information internally. Checking this doesn't really help us catch regressions like this one. It's a cheap operation and it's pretty common and reasonable for this number to change between commits.

What's actually expensive about this regression is calling HeaderSearch::loadSubdirectoryModuleMaps(DirectoryLookup &) multiple times, since Dir.haveSearchedAllModuleMaps() never changes (we only mutate the local DirectoryLookup copy). That function accesses the real filesystem via LLVM's VFS and that is actually the expensive operation:

llvm::vfs::FileSystem &FS = FileMgr.getVirtualFileSystem();
for (llvm::vfs::directory_iterator Dir = FS.dir_begin(DirNative, EC), DirEnd;
     Dir != DirEnd && !EC; Dir.increment(EC)) {
    //
}

It would be nice to have statistics for these as well, but we don't have any infrastructure for that ATM.

Add regression test

ributzka added inline comments.Oct 19 2022, 5:37 PM
clang/test/Modules/file-manager-lookup-count.m
73

The content of the stats file may change over time, so the test shouldn't limit it to just the listed entries. I would drop the first and last CHECK and change CHECK-NEXT to just CHECK.

Remove regression tests.

This revision is now accepted and ready to land.Oct 20 2022, 3:59 PM
jansvoboda11 edited the summary of this revision. (Show Details)Oct 20 2022, 4:09 PM
This revision was landed with ongoing or failed builds.Oct 20 2022, 4:12 PM
This revision was automatically updated to reflect the committed changes.