<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Mar 25, 2015 at 7:45 PM, Richard Smith <span dir="ltr"><<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><span class="">On Wed, Mar 25, 2015 at 6:47 PM, Sean Silva <span dir="ltr"><<a href="mailto:chisophugis@gmail.com" target="_blank">chisophugis@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><span>On Wed, Mar 25, 2015 at 6:10 PM, Richard Smith <span dir="ltr"><<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><span>On Wed, Mar 25, 2015 at 6:01 PM, Sean Silva <span dir="ltr"><<a href="mailto:chisophugis@gmail.com" target="_blank">chisophugis@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><span>On Wed, Mar 25, 2015 at 5:56 PM, Sean Silva <span dir="ltr"><<a href="mailto:chisophugis@gmail.com" target="_blank">chisophugis@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div>Awesome performance numbers!</div><div><br></div>When we discussed this previously, one alternative (should we ever need it; not sure we do) if the memory overhead is excessive is to use a waymarking scheme to find the canonical decl in constant time (unlike Use <a href="http://www.llvm.org/docs/ProgrammersManual.html#the-waymarking-algorithm" target="_blank">http://www.llvm.org/docs/ProgrammersManual.html#the-waymarking-algorithm</a> , we don't have random access, but we can instead take advantage of the stable address of the canonical decl and directly store bits of the stable address).</div></blockquote><div><br></div></span><div>D'oh, just saw the review thread and <a href="https://llvm.org/bugs/show_bug.cgi?id=21264" target="_blank">https://llvm.org/bugs/show_bug.cgi?id=21264</a> where Richard suggests a more elaborate (but not guaranteed constant time?) scheme.</div></div></div></div></blockquote><div><br></div></span><div>It's guaranteed constant time to get the canonical decl (because every decl either is it or has a pointer to it) and to find the previous decl (although the constant on that is a bit higher if there are multiple redeclarations, as is the memory usage).</div></div></div></div></blockquote><div><br></div></span><div>Doesn't your suggested scheme require a hash table lookup for getPreviousDecl? That isn't guaranteed constant time.</div></div></div></div></blockquote><div><br></div></span><div>Well sure, but not in an interesting way; that is not the pathological case we're trying to avoid.</div></div></div></div></blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><span class=""><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>I was thinking of something cruder like having the low two bits of the pointers in the chain be {0,bit from address} or {1,bit from address}, with {1,bit from address} meaning "if you have already collected 32/64 bits of the address, go ahead and reinterpret them as a pointer to the canonical decl". That has constant time finding of the canonical decl, constant time previous decl, and doesn't require any auxiliary data structures on the side. Although I'm not sure how practical of a "constant time" 10's of linked-list follows are (and not to mention maintaining that structure, which takes a similar "constant time").</div></div></div></div></blockquote><div><br></div></span><div>The expected common case is a short redeclaration chain; I would expect most of the gain for non-modules builds comes from reducing the cost from walking < 5 PreviousDecl links down to walking zero of them, and your proposed scheme doesn't help for that case.</div></div></div></div></blockquote><div><br></div><div><br></div><div>To elaborate on the sentiment of my original message, it was more along the lines of "D'oh, looks like Richard has already done some thinking about this that surely more accurately reflects the real holistic performance of this datastructure, compared to the hopelessly naive scheme I had in mind that does overcome the algorithmic problem but likely fails to take into account the full context of the situation."</div><div><br></div><div>The "(but not guaranteed constant time?)" was just a side note along the lines of "I find it a bit curious that a hash table on the side is necessary considering that a guaranteed-constant-time (also no-memory-overhead) way exists for the entire chain."</div><div><br></div><div>-- Sean Silva</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><div class="h5"><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><div><div>-- Sean Silva</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><span><font color="#888888"><div>-- Sean Silva</div></font></span><div><div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><span><font color="#888888"><div><br></div><div>-- Sean Silva</div></font></span></div><div><div><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Mar 25, 2015 at 4:18 PM, Manuel Klimek <span dir="ltr"><<a href="mailto:klimek@google.com" target="_blank">klimek@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">Author: klimek<br>
Date: Wed Mar 25 18:18:30 2015<br>
New Revision: 233228<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=233228&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=233228&view=rev</a><br>
Log:<br>
Keep track of canonical decls in Redeclarable.<br>
<br>
More than 2x speedup on modules builds with large redecl chains.<br>
Roughly 15-20% speedup on non-modules builds for very large TUs.<br>
Between 2-3% cost in memory on large TUs.<br>
<br>
Modified:<br>
cfe/trunk/include/clang/AST/Decl.h<br>
cfe/trunk/include/clang/AST/Redeclarable.h<br>
cfe/trunk/lib/Serialization/ASTReaderDecl.cpp<br>
<br>
Modified: cfe/trunk/include/clang/AST/Decl.h<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/Decl.h?rev=233228&r1=233227&r2=233228&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/Decl.h?rev=233228&r1=233227&r2=233228&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/include/clang/AST/Decl.h (original)<br>
+++ cfe/trunk/include/clang/AST/Decl.h Wed Mar 25 18:18:30 2015<br>
@@ -3738,8 +3738,6 @@ void Redeclarable<decl_type>::setPreviou<br>
assert(RedeclLink.NextIsLatest() &&<br>
"setPreviousDecl on a decl already in a redeclaration chain");<br>
<br>
- decl_type *First;<br>
-<br>
if (PrevDecl) {<br>
// Point to previous. Make sure that this is actually the most recent<br>
// redeclaration, or we can build invalid chains. If the most recent<br>
<br>
Modified: cfe/trunk/include/clang/AST/Redeclarable.h<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/Redeclarable.h?rev=233228&r1=233227&r2=233228&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/Redeclarable.h?rev=233228&r1=233227&r2=233228&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/include/clang/AST/Redeclarable.h (original)<br>
+++ cfe/trunk/include/clang/AST/Redeclarable.h Wed Mar 25 18:18:30 2015<br>
@@ -121,14 +121,15 @@ protected:<br>
///<br>
/// If there is only one declaration, it is <pointer to self, true><br>
DeclLink RedeclLink;<br>
+ decl_type *First;<br>
<br>
decl_type *getNextRedeclaration() const {<br>
return RedeclLink.getNext(static_cast<const decl_type *>(this));<br>
}<br>
<br>
public:<br>
- Redeclarable(const ASTContext &Ctx)<br>
- : RedeclLink(LatestDeclLink(Ctx)) {}<br>
+ Redeclarable(const ASTContext &Ctx)<br>
+ : RedeclLink(LatestDeclLink(Ctx)), First(static_cast<decl_type *>(this)) {}<br>
<br>
/// \brief Return the previous declaration of this declaration or NULL if this<br>
/// is the first declaration.<br>
@@ -144,21 +145,11 @@ public:<br>
<br>
/// \brief Return the first declaration of this declaration or itself if this<br>
/// is the only declaration.<br>
- decl_type *getFirstDecl() {<br>
- decl_type *D = static_cast<decl_type*>(this);<br>
- while (D->getPreviousDecl())<br>
- D = D->getPreviousDecl();<br>
- return D;<br>
- }<br>
+ decl_type *getFirstDecl() { return First; }<br>
<br>
/// \brief Return the first declaration of this declaration or itself if this<br>
/// is the only declaration.<br>
- const decl_type *getFirstDecl() const {<br>
- const decl_type *D = static_cast<const decl_type*>(this);<br>
- while (D->getPreviousDecl())<br>
- D = D->getPreviousDecl();<br>
- return D;<br>
- }<br>
+ const decl_type *getFirstDecl() const { return First; }<br>
<br>
/// \brief True if this is the first declaration in its redeclaration chain.<br>
bool isFirstDecl() const { return RedeclLink.NextIsLatest(); }<br>
<br>
Modified: cfe/trunk/lib/Serialization/ASTReaderDecl.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Serialization/ASTReaderDecl.cpp?rev=233228&r1=233227&r2=233228&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Serialization/ASTReaderDecl.cpp?rev=233228&r1=233227&r2=233228&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/lib/Serialization/ASTReaderDecl.cpp (original)<br>
+++ cfe/trunk/lib/Serialization/ASTReaderDecl.cpp Wed Mar 25 18:18:30 2015<br>
@@ -2106,6 +2106,7 @@ ASTDeclReader::VisitRedeclarable(Redecla<br>
// which is the one that matters and mark the real previous DeclID to be<br>
// loaded & attached later on.<br>
D->RedeclLink = Redeclarable<T>::PreviousDeclLink(FirstDecl);<br>
+ D->First = FirstDecl->getCanonicalDecl();<br>
}<br>
<br>
// Note that this declaration has been deserialized.<br>
@@ -2209,6 +2210,7 @@ void ASTDeclReader::mergeRedeclarable(Re<br>
// of the existing declaration, so that this declaration has the<br>
// appropriate canonical declaration.<br>
D->RedeclLink = Redeclarable<T>::PreviousDeclLink(ExistingCanon);<br>
+ D->First = ExistingCanon;<br>
<br>
// When we merge a namespace, update its pointer to the first namespace.<br>
// We cannot have loaded any redeclarations of this declaration yet, so<br>
@@ -2828,6 +2830,7 @@ void ASTDeclReader::attachPreviousDeclIm<br>
Redeclarable<DeclT> *D,<br>
Decl *Previous, Decl *Canon) {<br>
D->RedeclLink.setPrevious(cast<DeclT>(Previous));<br>
+ D->First = cast<DeclT>(Previous)->First;<br>
}<br>
namespace clang {<br>
template<><br>
@@ -2838,6 +2841,7 @@ void ASTDeclReader::attachPreviousDeclIm<br>
FunctionDecl *PrevFD = cast<FunctionDecl>(Previous);<br>
<br>
FD->RedeclLink.setPrevious(PrevFD);<br>
+ FD->First = PrevFD->First;<br>
<br>
// If the previous declaration is an inline function declaration, then this<br>
// declaration is too.<br>
<br>
<br>
_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">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>
</blockquote></div><br></div>
</div></div></blockquote></div></div></div><br></div></div>
<br>_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">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>
<br></blockquote></div></div></div><br></div></div>
</blockquote></div></div></div><br></div></div>
</blockquote></div></div></div><br></div></div>
</blockquote></div><br></div></div>