r354075 - [clang][FileManager] fillRealPathName even if we aren't opening the file

[Nico Weber via cfe-commits]

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
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 <jkorous at 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 <thakis at 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 at 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
>> 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
>>
>> 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
>>
>> ==============================================================================
>> --- 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
>>
>> ==============================================================================
>> --- 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 at lists.llvm.org
>> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20190219/70e998d4/attachment.html>