<html><head><meta http-equiv="Content-Type" content="text/html; charset=us-ascii"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; line-break: after-white-space;" class="">I see. You're right the name sounds slow indeed. Thank you for the explanation!<br class=""><div><br class=""><blockquote type="cite" class=""><div class="">On Feb 19, 2019, at 6:43 AM, Nico Weber <<a href="mailto:thakis@chromium.org" class="">thakis@chromium.org</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><div dir="ltr" class=""><div dir="ltr" class=""><div dir="ltr" class=""><div dir="ltr" class="">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 <a href="http://llvm-cs.pcc.me.uk/tools/clang/lib/Basic/FileManager.cpp#361" class="">http://llvm-cs.pcc.me.uk/tools/clang/lib/Basic/FileManager.cpp#361</a> 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?)</div></div></div></div><br class=""><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, Feb 18, 2019 at 5:26 PM Jan Korous <<a href="mailto:jkorous@apple.com" class="">jkorous@apple.com</a>> wrote:<br class=""></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div style="overflow-wrap: break-word;" class="">Hi Nico,<div class=""><br class=""></div><div class="">I didn't think it necessary as the change doesn't introduce any interaction with filesystem - it's just copying a string.</div><div class=""><br class=""></div><div class="">Do you mean it causes a performance regression?</div><div class=""><br class=""></div><div class="">Thanks.</div><div class=""><br class=""></div><div class="">Jan<br class=""><div class=""><br class=""><blockquote type="cite" class=""><div class="">On Feb 15, 2019, at 6:15 AM, Nico Weber <<a href="mailto:thakis@chromium.org" target="_blank" class="">thakis@chromium.org</a>> wrote:</div><br class="gmail-m_-3085581552019888486Apple-interchange-newline"><div class=""><div dir="ltr" class="">Did you do any performance testing to check if this slows down clang?</div><br class=""><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, Feb 14, 2019 at 6:02 PM Jan Korous via cfe-commits <<a href="mailto:cfe-commits@lists.llvm.org" target="_blank" class="">cfe-commits@lists.llvm.org</a>> wrote:<br class=""></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Author: jkorous<br class="">
Date: Thu Feb 14 15:02:35 2019<br class="">
New Revision: 354075<br class="">
<br class="">
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=354075&view=rev" rel="noreferrer" target="_blank" class="">http://llvm.org/viewvc/llvm-project?rev=354075&view=rev</a><br class="">
Log:<br class="">
[clang][FileManager] fillRealPathName even if we aren't opening the file<br class="">
<br class="">
The pathname wasn't previously filled when the getFile() method was called with openFile = false.<br class="">
We are caching FileEntry-s in ParsedAST::Includes in clangd and this caused the problem.<br class="">
<br class="">
This fixes an internal test failure in clangd - ClangdTests.GoToInclude.All<br class="">
<br class="">
<a class="">rdar://47536127</a><br class="">
<br class="">
Differential Revision: <a href="https://reviews.llvm.org/D58213" rel="noreferrer" target="_blank" class="">https://reviews.llvm.org/D58213</a><br class="">
<br class="">
Modified:<br class="">
cfe/trunk/lib/Basic/FileManager.cpp<br class="">
cfe/trunk/unittests/Basic/FileManagerTest.cpp<br class="">
<br class="">
Modified: cfe/trunk/lib/Basic/FileManager.cpp<br class="">
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Basic/FileManager.cpp?rev=354075&r1=354074&r2=354075&view=diff" rel="noreferrer" target="_blank" class="">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Basic/FileManager.cpp?rev=354075&r1=354074&r2=354075&view=diff</a><br class="">
==============================================================================<br class="">
--- cfe/trunk/lib/Basic/FileManager.cpp (original)<br class="">
+++ cfe/trunk/lib/Basic/FileManager.cpp Thu Feb 14 15:02:35 2019<br class="">
@@ -267,6 +267,9 @@ const FileEntry *FileManager::getFile(St<br class="">
if (UFE.File) {<br class="">
if (auto PathName = UFE.File->getName())<br class="">
fillRealPathName(&UFE, *PathName);<br class="">
+ } else if (!openFile) {<br class="">
+ // We should still fill the path even if we aren't opening the file.<br class="">
+ fillRealPathName(&UFE, InterndFileName);<br class="">
}<br class="">
return &UFE;<br class="">
}<br class="">
<br class="">
Modified: cfe/trunk/unittests/Basic/FileManagerTest.cpp<br class="">
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Basic/FileManagerTest.cpp?rev=354075&r1=354074&r2=354075&view=diff" rel="noreferrer" target="_blank" class="">http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Basic/FileManagerTest.cpp?rev=354075&r1=354074&r2=354075&view=diff</a><br class="">
==============================================================================<br class="">
--- cfe/trunk/unittests/Basic/FileManagerTest.cpp (original)<br class="">
+++ cfe/trunk/unittests/Basic/FileManagerTest.cpp Thu Feb 14 15:02:35 2019<br class="">
@@ -346,4 +346,18 @@ TEST_F(FileManagerTest, getVirtualFileFi<br class="">
EXPECT_EQ(file->tryGetRealPathName(), ExpectedResult);<br class="">
}<br class="">
<br class="">
+TEST_F(FileManagerTest, getFileDontOpenRealPath) {<br class="">
+ auto statCache = llvm::make_unique<FakeStatCache>();<br class="">
+ statCache->InjectDirectory("/tmp/abc", 42);<br class="">
+ SmallString<64> Path("/tmp/abc/foo.cpp");<br class="">
+ statCache->InjectFile(Path.str().str().c_str(), 43);<br class="">
+ manager.setStatCache(std::move(statCache));<br class="">
+<br class="">
+ const FileEntry *file = manager.getFile(Path, /*openFile=*/false);<br class="">
+<br class="">
+ ASSERT_TRUE(file != nullptr);<br class="">
+<br class="">
+ ASSERT_EQ(file->tryGetRealPathName(), Path);<br class="">
+}<br class="">
+<br class="">
} // anonymous namespace<br class="">
<br class="">
<br class="">
_______________________________________________<br class="">
cfe-commits mailing list<br class="">
<a href="mailto:cfe-commits@lists.llvm.org" target="_blank" class="">cfe-commits@lists.llvm.org</a><br class="">
<a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits" rel="noreferrer" target="_blank" class="">https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits</a><br class="">
</blockquote></div>
</div></blockquote></div><br class=""></div></div></blockquote></div>
</div></blockquote></div><br class=""></body></html>