I see. You're right the name sounds slow indeed. Thank you for the explanation!
> On Feb 19, 2019, at 6:43 AM, Nico Weber <tha...@chromium.org> wrote: > > I didn't realize it just copied a string. I just saw a call to > "fillRealPathName" and the "RealPath" bit sounded slow. From clicking around > in http://llvm-cs.pcc.me.uk/tools/clang/lib/Basic/FileManager.cpp#361 > <http://llvm-cs.pcc.me.uk/tools/clang/lib/Basic/FileManager.cpp#361> it looks > like it's not really computing a realpath (for symlinks). It still does quite > a bit of string processing if InterndFileName isn't absolute, but that's > probably fine. It's still the kind of thing I'd carefully measure since > getFile(openFile=false) was performance-sensitive in a certain case iirc (I > think when using pch files?) > > On Mon, Feb 18, 2019 at 5:26 PM Jan Korous <jkor...@apple.com > <mailto:jkor...@apple.com>> wrote: > Hi Nico, > > I didn't think it necessary as the change doesn't introduce any interaction > with filesystem - it's just copying a string. > > Do you mean it causes a performance regression? > > Thanks. > > Jan > >> On Feb 15, 2019, at 6:15 AM, Nico Weber <tha...@chromium.org >> <mailto:tha...@chromium.org>> wrote: >> >> Did you do any performance testing to check if this slows down clang? >> >> On Thu, Feb 14, 2019 at 6:02 PM Jan Korous via cfe-commits >> <cfe-commits@lists.llvm.org <mailto:cfe-commits@lists.llvm.org>> wrote: >> Author: jkorous >> Date: Thu Feb 14 15:02:35 2019 >> New Revision: 354075 >> >> URL: http://llvm.org/viewvc/llvm-project?rev=354075&view=rev >> <http://llvm.org/viewvc/llvm-project?rev=354075&view=rev> >> Log: >> [clang][FileManager] fillRealPathName even if we aren't opening the file >> >> The pathname wasn't previously filled when the getFile() method was called >> with openFile = false. >> We are caching FileEntry-s in ParsedAST::Includes in clangd and this caused >> the problem. >> >> This fixes an internal test failure in clangd - ClangdTests.GoToInclude.All >> >> rdar://47536127 <> >> >> Differential Revision: https://reviews.llvm.org/D58213 >> <https://reviews.llvm.org/D58213> >> >> Modified: >> cfe/trunk/lib/Basic/FileManager.cpp >> cfe/trunk/unittests/Basic/FileManagerTest.cpp >> >> Modified: cfe/trunk/lib/Basic/FileManager.cpp >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Basic/FileManager.cpp?rev=354075&r1=354074&r2=354075&view=diff >> >> <http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Basic/FileManager.cpp?rev=354075&r1=354074&r2=354075&view=diff> >> ============================================================================== >> --- cfe/trunk/lib/Basic/FileManager.cpp (original) >> +++ cfe/trunk/lib/Basic/FileManager.cpp Thu Feb 14 15:02:35 2019 >> @@ -267,6 +267,9 @@ const FileEntry *FileManager::getFile(St >> if (UFE.File) { >> if (auto PathName = UFE.File->getName()) >> fillRealPathName(&UFE, *PathName); >> + } else if (!openFile) { >> + // We should still fill the path even if we aren't opening the file. >> + fillRealPathName(&UFE, InterndFileName); >> } >> return &UFE; >> } >> >> Modified: cfe/trunk/unittests/Basic/FileManagerTest.cpp >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Basic/FileManagerTest.cpp?rev=354075&r1=354074&r2=354075&view=diff >> >> <http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Basic/FileManagerTest.cpp?rev=354075&r1=354074&r2=354075&view=diff> >> ============================================================================== >> --- cfe/trunk/unittests/Basic/FileManagerTest.cpp (original) >> +++ cfe/trunk/unittests/Basic/FileManagerTest.cpp Thu Feb 14 15:02:35 2019 >> @@ -346,4 +346,18 @@ TEST_F(FileManagerTest, getVirtualFileFi >> EXPECT_EQ(file->tryGetRealPathName(), ExpectedResult); >> } >> >> +TEST_F(FileManagerTest, getFileDontOpenRealPath) { >> + auto statCache = llvm::make_unique<FakeStatCache>(); >> + statCache->InjectDirectory("/tmp/abc", 42); >> + SmallString<64> Path("/tmp/abc/foo.cpp"); >> + statCache->InjectFile(Path.str().str().c_str(), 43); >> + manager.setStatCache(std::move(statCache)); >> + >> + const FileEntry *file = manager.getFile(Path, /*openFile=*/false); >> + >> + ASSERT_TRUE(file != nullptr); >> + >> + ASSERT_EQ(file->tryGetRealPathName(), Path); >> +} >> + >> } // anonymous namespace >> >> >> _______________________________________________ >> cfe-commits mailing list >> cfe-commits@lists.llvm.org <mailto:cfe-commits@lists.llvm.org> >> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits >> <https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits> >
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
