[cfe-commits] r159256 - in /cfe/trunk: include/clang/Basic/FileManager.h lib/Basic/FileManager.cpp

[Axel Naumann]

Hi Chandler,

On 06/27/2012 11:41 AM, Chandler Carruth wrote:
> I don't care about backing this out -- it's not breaking anything.

True after r159262.

> I just have reservations about the interface and strategy as a whole.
> Let's just talk about it.
>           ==============================__==============================__==================
>             --- cfe/trunk/include/clang/Basic/__FileManager.h (original)
>             +++ cfe/trunk/include/clang/Basic/__FileManager.h Wed Jun 27
>         04:17:42 2012
>             @@ -222,6 +222,9 @@
>                /// FileManager's FileSystemOptions.
>                bool getNoncachedStatValue(__StringRef Path, struct stat
>         &StatBuf);
>
>             +  /// \brief Remove the real file Entry from the cache.
>             +  void InvalidateCache(const FileEntry* Entry);
>             +
>
>
> Please follow the LLVM coding conventions, especially if you're doing
> post-commit review. ;] Methods are 'camelCase' and the '*' attaches to
> the variable or parameter name.

Will do for the '*' (after discussing the more fundamental requests :-). 
Regarding the camelCase: the patch follows the CamelCasing of 
FileManager::GetUniqueIDMapping() / PrintStats() ... i.e. let's say 
casing is undefined.

>           ==============================__==============================__==================
>             --- cfe/trunk/lib/Basic/__FileManager.cpp (original)
>             +++ cfe/trunk/lib/Basic/__FileManager.cpp Wed Jun 27
>         04:17:42 2012
>             @@ -111,6 +111,8 @@
>                }
>
>                size_t size() const { return UniqueFiles.size(); }
>             +
>             +  friend class FileManager;
>
>
> Adding these friends seems extremely unfortunate. Why are they required?

Because of the UniqueFiles.erase() below. Instead we could add erase(). 
The two types containing the friend declarations are visible only inside 
the TU so we thought friending is fine. We can decide later once we know 
what the patch should do.

>             +void FileManager::InvalidateCache(__const FileEntry* Entry) {
>             +  if (!Entry)
>             +    return;
>
>
> Why do you support a null entry being passed in at all? What's the use
> case for that?

No use case. Will change to assert().

>             +
>             +  SeenFileEntries.erase(Entry->__getName());
>             +  UniqueRealFiles.UniqueFiles.__erase(*Entry);
>
>
> Mutating these data structures in place seems like a really unfortunate
> strategy. It precludes lots of design choices down the road such as
> making data structures immutable, or otherwise optimizing for the expect
> case of the cache remaining valid. It also punches a hole through any
> hope of sharing this cache across compilations.

Good points. See below.

> I don't understand the motivation to drop *only* a single file really.

$ #include "FileWithErrorFixedLaterTryAgain.h"
$ #include "FileWithErrorFixedLaterTryAgain.h"

So having the ability to remove the last N files is good enough. See 
Vassil's "[cfe-dev] Avoid clang::SourceManger's cache" for details.

> For the purposes of Cling, why not just add a non-caching made to the
> entire setup?

It's a huge performance hit; think STL.

> In general, I would prefer to drop the *entire* cache than
> bother with fine-grained cache eviction as it doesn't seem clear that
> the complexity and implementation constraints fine-grained invalidation
> impose are justified by the gains.

Okay. I see three maybe more reasonable options, in decreasing order of 
preference for us:
* truncate the cache (we'd anyway only remove the last N files)
* add interface to decide which file should be cached and which 
shouldn't, based on the file's full name, in our case we'd likely only 
cache read-only files (e.g. STL).
* clear the cache altogether. That's the least performant option for us.

Should we implement the first option? Or keep the current version?

Cheers, Axel.