<br><br><div class="gmail_quote">On Wed, Jun 27, 2012 at 4:15 PM, Axel Naumann <span dir="ltr"><<a href="mailto:Axel.Naumann@cern.ch" target="_blank">Axel.Naumann@cern.ch</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Hi Chandler,<br>
<div class="im"><br>
On 06/27/2012 11:41 AM, Chandler Carruth wrote:<br>
> I don't care about backing this out -- it's not breaking anything.<br>
<br>
</div>True after r159262.<br>
<div class="im"><br>
> I just have reservations about the interface and strategy as a whole.<br>
> Let's just talk about it.<br>
</div>> ==============================__==============================__==================<br>
> --- cfe/trunk/include/clang/Basic/__FileManager.h (original)<br>
> +++ cfe/trunk/include/clang/Basic/__FileManager.h Wed Jun 27<br>
<div class="im">> 04:17:42 2012<br>
> @@ -222,6 +222,9 @@<br>
> /// FileManager's FileSystemOptions.<br>
</div>> bool getNoncachedStatValue(__StringRef Path, struct stat<br>
<div class="im">> &StatBuf);<br>
><br>
> + /// \brief Remove the real file Entry from the cache.<br>
> + void InvalidateCache(const FileEntry* Entry);<br>
> +<br>
><br>
><br>
> Please follow the LLVM coding conventions, especially if you're doing<br>
> post-commit review. ;] Methods are 'camelCase' and the '*' attaches to<br>
> the variable or parameter name.<br>
<br>
</div>Will do for the '*' (after discussing the more fundamental requests :-).<br>
Regarding the camelCase: the patch follows the CamelCasing of<br>
FileManager::GetUniqueIDMapping() / PrintStats() ... i.e. let's say<br>
casing is undefined.<br>
<br>
> ==============================__==============================__==================<br>
> --- cfe/trunk/lib/Basic/__FileManager.cpp (original)<br>
> +++ cfe/trunk/lib/Basic/__FileManager.cpp Wed Jun 27<br>
<div class="im">> 04:17:42 2012<br>
> @@ -111,6 +111,8 @@<br>
> }<br>
><br>
> size_t size() const { return UniqueFiles.size(); }<br>
> +<br>
> + friend class FileManager;<br>
><br>
><br>
> Adding these friends seems extremely unfortunate. Why are they required?<br>
<br>
</div>Because of the UniqueFiles.erase() below. Instead we could add erase().<br>
The two types containing the friend declarations are visible only inside<br>
the TU so we thought friending is fine. We can decide later once we know<br>
what the patch should do.<br>
<br>
> +void FileManager::InvalidateCache(__const FileEntry* Entry) {<br>
<div class="im">> + if (!Entry)<br>
> + return;<br>
><br>
><br>
> Why do you support a null entry being passed in at all? What's the use<br>
> case for that?<br>
<br>
</div>No use case. Will change to assert().<br>
<br>
> +<br>
> + SeenFileEntries.erase(Entry->__getName());<br>
> + UniqueRealFiles.UniqueFiles.__erase(*Entry);<br>
<div class="im">><br>
><br>
> Mutating these data structures in place seems like a really unfortunate<br>
> strategy. It precludes lots of design choices down the road such as<br>
> making data structures immutable, or otherwise optimizing for the expect<br>
> case of the cache remaining valid. It also punches a hole through any<br>
> hope of sharing this cache across compilations.<br>
<br>
</div>Good points. See below.<br>
<div class="im"><br>
> I don't understand the motivation to drop *only* a single file really.<br>
<br>
</div>$ #include "FileWithErrorFixedLaterTryAgain.h"<br>
$ #include "FileWithErrorFixedLaterTryAgain.h"<br>
<br>
So having the ability to remove the last N files is good enough. See<br>
Vassil's "[cfe-dev] Avoid clang::SourceManger's cache" for details.<br>
<div class="im"><br>
> For the purposes of Cling, why not just add a non-caching made to the<br>
> entire setup?<br>
<br>
</div>It's a huge performance hit; think STL.<br>
<div class="im"><br>
> In general, I would prefer to drop the *entire* cache than<br>
> bother with fine-grained cache eviction as it doesn't seem clear that<br>
> the complexity and implementation constraints fine-grained invalidation<br>
> impose are justified by the gains.<br>
<br>
</div>Okay. I see three maybe more reasonable options, in decreasing order of<br>
preference for us:<br>
* truncate the cache (we'd anyway only remove the last N files)<br>
* add interface to decide which file should be cached and which<br>
shouldn't, based on the file's full name, in our case we'd likely only<br>
cache read-only files (e.g. STL).<br>
* clear the cache altogether. That's the least performant option for us.<br>
<br>
Should we implement the first option? Or keep the current version?<br>
<br>
Cheers, Axel.<br>
<div class="HOEnZb"><div class="h5">_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
</div></div></blockquote></div><br>There has been a discussion about avoiding warnings in 3rd party headers on top of STL headers. In this case we could expect that Cling did the smart things and cached 3rd party headers but not local ones. Or perhaps had the ability to only clear ones "own" files.<br>
<br>From what I understood, in Cling you don't edit many files at once. However if we are talking about being able to react to a modification to a header file, without really knowing its dependencies, then the truncation cannot be used easily here it seems.<br>
<br>-- Matthieu<br><br>