[PATCH] D83621: [clang][Tooling] Try to avoid file system access if there is no record for the file in compile_commads.json

This revision was automatically updated to reflect the committed changes. Closed by commit rGd19f0666bcd8: [clang][Tooling] Try to avoid file system access if there is no record for the… (authored by ArcsinX, committed by sammccall). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

[PATCH] D83621: [clang][Tooling] Try to avoid file system access if there is no record for the file in compile_commads.json

sammccall added a comment. In D83621#2153711 , @ArcsinX wrote: > As far as I do not have commit access, could you please commit for me? > Aleksandr Platonov Oops, missed this! Committed as d19f0666bcd8f7d26aaf4019244c3ed91e47b1b7

[PATCH] D83621: [clang][Tooling] Try to avoid file system access if there is no record for the file in compile_commads.json

ArcsinX marked 3 inline comments as done. ArcsinX added a comment. As far as I do not have commit access, could you please commit for me? Aleksandr Platonov Comment at: clang/lib/Tooling/FileMatchTrie.cpp:111 + // As far as we do not support file symlinks we compare +

[PATCH] D83621: [clang][Tooling] Try to avoid file system access if there is no record for the file in compile_commads.json

ArcsinX updated this revision to Diff 278230. ArcsinX added a comment. Update comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83621/new/ https://reviews.llvm.org/D83621 Files: clang/lib/Tooling/FileMatchTrie.cpp

[PATCH] D83621: [clang][Tooling] Try to avoid file system access if there is no record for the file in compile_commads.json

sammccall accepted this revision. sammccall added a comment. This revision is now accepted and ready to land. Hmm, there is actually a case the old behavior may have been papering over: case-insensitive filesystems. If the real file is foo.cc and we query Foo.cc, then the trie would

[PATCH] D83621: [clang][Tooling] Try to avoid file system access if there is no record for the file in compile_commads.json

ArcsinX updated this revision to Diff 278161. ArcsinX added a comment. Support only directory simlinks. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83621/new/ https://reviews.llvm.org/D83621 Files: clang/lib/Tooling/FileMatchTrie.cpp

[PATCH] D83621: [clang][Tooling] Try to avoid file system access if there is no record for the file in compile_commads.json

sammccall added a subscriber: chandlerc. sammccall added a comment. In D83621#2152618 , @ArcsinX wrote: > In D83621#2151716 , @sammccall wrote: > > > In D83621#2146750 ,

[PATCH] D83621: [clang][Tooling] Try to avoid file system access if there is no record for the file in compile_commads.json

ArcsinX added a comment. In D83621#2151716 , @sammccall wrote: > In D83621#2146750 , @ArcsinX wrote: > > > > - don't scan for equivalences if the set of candidates exceeds some size > > > threshold (10 or so) > >

[PATCH] D83621: [clang][Tooling] Try to avoid file system access if there is no record for the file in compile_commads.json

sammccall added a comment. In D83621#2146750 , @ArcsinX wrote: > > - don't scan for equivalences if the set of candidates exceeds some size > > threshold (10 or so) > > - don't scan for equivalences if the node we'd scan under is the root > > After such

[PATCH] D83621: [clang][Tooling] Try to avoid file system access if there is no record for the file in compile_commads.json

ArcsinX added a comment. In D83621#2146746 , @klimek wrote: > IIRC the symlink checking was there because some part of the system on linux > tends to give us realpaths (with CMake builds), and that leads to not finding > anything if there are symlinks

[PATCH] D83621: [clang][Tooling] Try to avoid file system access if there is no record for the file in compile_commads.json

ArcsinX added a comment. In D83621#2146706 , @sammccall wrote: > Wow, yeah, this seems pretty bad! I'm not very familiar with this code. I'm > curious, for "it tooks more than 1 second" - what OS is this on? Windows. I faced this problem on a huge

[PATCH] D83621: [clang][Tooling] Try to avoid file system access if there is no record for the file in compile_commads.json

klimek added a subscriber: djasper. klimek added a comment. @djasper wrote this iirc, but I doubt he'll have much time to look into this :) IIRC the symlink checking was there because some part of the system on linux tends to give us realpaths (with CMake builds), and that leads to not finding

[PATCH] D83621: [clang][Tooling] Try to avoid file system access if there is no record for the file in compile_commads.json

sammccall added a comment. > But I am not sure, is it safe to completely remove FileMatchTrie? Missed this... honestly I don't know. It does seem a little overly defensive to me, but any change in symlink behaviour tends to break *someone*. Repository: rG LLVM Github Monorepo CHANGES SINCE

[PATCH] D83621: [clang][Tooling] Try to avoid file system access if there is no record for the file in compile_commads.json

sammccall added a subscriber: klimek. sammccall added a comment. Wow, yeah, this seems pretty bad! I'm not very familiar with this code. I'm curious, for "it tooks more than 1 second" - what OS is this on? My guess is that way back when, the performance of looking up a CDB entry that didn't

[PATCH] D83621: [clang][Tooling] Try to avoid file system access if there is no record for the file in compile_commads.json

ArcsinX added a comment. Also I think that `FileMatchTrie` introduced in https://reviews.llvm.org/D30 is not efficient solution to fight with symlinks and for most cases `InterpolatingCompilationDatabase` will be accurate enough. But I am not sure, is it safe to completely remove

[PATCH] D83621: [clang][Tooling] Try to avoid file system access if there is no record for the file in compile_commads.json

ArcsinX created this revision. Herald added subscribers: cfe-commits, usaxena95, kadircet, ilya-biryukov. Herald added a project: clang. If there is no record in compile_commands.json, we try to find suitable record with `MatchTrie.findEquivalent()` call. This is very expensive operation with a