[FileManager] Do not call 'real_path' in getFile().

[ioeric]

Summary

This partially rolls back the change in D48903:
https://github.com/llvm-mirror/clang/commit/89aa7f45a1f728144935289d4ce69d8522999de0#diff-0025af005307891b5429b6a834823d5eR318

real_path can be very expensive on real file systems, and calling it on each
opened file can slow down the compilation. This also slows down deserialized
ASTs for which real paths need to be recalculated for each input files again.

For clangd code completion latency (using preamble):
Before

After

[ioeric]

• Add comment

[simark]

lib/Basic/FileManager.cpp

318–319

What's the rationale for only computing the field if UFE.File is non-null?

Previously, if you looked up the file with openFile == false and then later openFile == true, the RealPathName field would not be set because of this. That doesn't seem right.

325

If the path contains symlinks, doesn't this put a non-real path in the RealPathName field? Won't users (e.g. clangd) use this value thinking it is a real path, when it is actually not?

[ioeric]

lib/Basic/FileManager.cpp

318–319

There has been no guarantee that RealFilePath is always set. I think that's the reason why the acceasor is called tryGetRealPathName.

325

This was the original behavior. In general, File Manager should never call real_path for users because it can be very expensive. Users should call real_path if they want to resolve symlinks. That said, it's fair to say that "RealPathName" is just a wrong name, and we should clean it up at some point.

[simark]

lib/Basic/FileManager.cpp

318–319

The way I understood it was that it could be empty because computing the real path can fail. Not just because we didn't skipped computing it.

325

Ok, then if the goal is not to actually have a real path (in the realpath(3) sense), that's fine. But I think we should rename the field sooner than later, it's really confusing.

That also means that it's kind of useless for us in clangd, so we should always call real_path there and not rely on that field.

[ioeric]

lib/Basic/FileManager.cpp

318–319

I agree that the API is confusing and lack of documentation (and we should fix), but the previous implementation did skip the computation if File is not available though.

325

Ok, then if the goal is not to actually have a real path (in the realpath(3) sense), that's fine. But I think we should rename the field sooner than later, it's really confusing.

+1

That also means that it's kind of useless for us in clangd, so we should always call real_path there and not rely on that field.

I guess it depends on whether you want symlink resolution or not. I know that clangd's index symbol collector resolves symlink explicitly, but I don't think clangd enforces symlink resolution in general?

[simark]

lib/Basic/FileManager.cpp

318–319

I agree that the API is confusing and lack of documentation (and we should fix), but the previous implementation did skip the computation if File is not available though.

Did it have a reason not to? What is the RealPathName field useful for, if it's unreliable?

325

I guess it depends on whether you want symlink resolution or not. I know that clangd's index symbol collector resolves symlink explicitly, but I don't think clangd enforces symlink resolution in general?

If we don't, we probably risk having duplicate results similar to what

https://reviews.llvm.org/D48687

fixed, by with paths differing because of symlinks instead of dot-dots.

[ioeric]

lib/Basic/FileManager.cpp

318–319

I think the biggest concern is the performance.

For example, clangd code completion latency increased dramatically with real_path:
With real_path:

Wihtou real_path:

325

Was the bug addressed in D48687 actually caused by symlinks though? If want we want is absolute paths with dotdot cleaned, it should be much cheaper to call VFS::makeAbsolutePath with remove_dot_dot.

In general, it's unclear whether clangd should resolve symlink (it might not always be what users want), and it should probably be a decision made by the build system integration. I think we would need a much more careful design if we decide to handle symlinks in clangd.

[simark]

lib/Basic/FileManager.cpp

318–319

But with this patch, we are not using real_path anymore, so it can't be the reason for not computing RealPathName when not opening the file. Since the non-real_path method is much more lightweight, would it make a significant difference if we always computed the field?

In the end I don't really mind, I am just looking for a rational explanation to why it's done this way.

325

Was the bug addressed in D48687 actually caused by symlinks though?

No, it was caused by non-relative path that included dotdots, I was extrapolating.

If want we want is absolute paths with dotdot cleaned, it should be much cheaper to call VFS::makeAbsolutePath with remove_dot_dot.

Agreed.

In general, it's unclear whether clangd should resolve symlink (it might not always be what users want), and it should probably be a decision made by the build system integration. I think we would need a much more careful design if we decide to handle symlinks in clangd.

Indeed, if those paths end up displayed in the UI, it could be preferrable to not resolve the symlinks. Using the real_path was kind of the "atomic bomb" solution to get rid of duplicates.

[ioeric]

lib/Basic/FileManager.cpp

318–319

Ohh, sorry that I was confused with the other thread...

Honestly, I also don't know where all these came from (it was implemented many years ago...). For now, I'm just trying to fix the problem with the safest change, as the old behavior, although confusing, should be pretty stable. Changing behavior further would likely cause more problems. I would prefer making further change when we are actually cleaning up or redesigning RealPathName/tryGetRealPath.

[ioeric]

• Compute absolute paths even when file is not opened.

lib/Basic/FileManager.cpp

318–319

Ok, I gave this a try, and it seems that computing absolute paths for unopened files doesn't cause any problem. Thanks!

[ilya-biryukov]

LGTM from my side. We need this to unbreak performance in code completion

lib/Basic/FileManager.cpp

const FileEntry *FileManager::getFile(StringRef Filename, bool openFile,
   UFE.ModTime = Data.ModTime;
   UFE.Dir     = DirInfo;
   UFE.UID     = NextFileUID++;
   UFE.UniqueID = Data.UniqueID;
   UFE.IsNamedPipe = Data.IsNamedPipe;
   UFE.InPCH = Data.InPCH;
   UFE.File = std::move(F);
   UFE.IsValid = true;
 
-  SmallString<128> RealPathName;
-  if (!FS->getRealPath(InterndFileName, RealPathName))
-    UFE.RealPathName = RealPathName.str();
-
+  llvm::SmallString<128> AbsPath(InterndFileName);
+  // This is not the same as `VFS::getRealPath()`, which resolves symlinks but
+  // can be very expensive on real file systems.
+  // FIXME: the semantic of RealPathName is unclear, and the name might be
+  // misleading. We need to clean up the interface here.
+  makeAbsolutePath(AbsPath);
+  llvm::sys::path::remove_dots(AbsPath, /*remove_dot_dot=*/true);
+  UFE.RealPathName = AbsPath.str();
   return &UFE;
 }
 
 const FileEntry *
 FileManager::getVirtualFile(StringRef Filename, off_t Size,
                             time_t ModificationTime) {
   ++NumFileLookups;