[cfe-commits] [PATCH] More Abbreviations for PCH Serialization

[Jonathan Turner]

On Jun 1, 2011, at 4:03 PM, Douglas Gregor wrote:
> Generally looks good, with a few comments:
> 
> Index: include/clang/Serialization/ASTWriter.h
> ===================================================================
> --- include/clang/Serialization/ASTWriter.h	(revision 132422)
> +++ include/clang/Serialization/ASTWriter.h	(working copy)
> @@ -279,6 +279,9 @@
>   /// file.
>   unsigned NumVisibleDeclContexts;
> 
> +  /// \brief The number of allowed abbreviations in bits
> +  unsigned NumAllowedAbbrevsSize;
> +
> 
> This should be a namespace-level constant named something like "DeclTypeBlockCodeSize", rather than a member of ASTWriter. It isn't going to change.

Good point.  I've moved it to a constant.

> Index: lib/Serialization/ASTWriterStmt.cpp
> ===================================================================
> --- lib/Serialization/ASTWriterStmt.cpp	(revision 132422)
> +++ lib/Serialization/ASTWriterStmt.cpp	(working copy)
> @@ -30,6 +30,7 @@
> 
>   public:
>     serialization::StmtCode Code;
> +    unsigned AbbrevToUse;
> 
>     ASTStmtWriter(ASTWriter &Writer, ASTWriter::RecordData &Record)
>       : Writer(Writer), Record(Record) { }
> @@ -392,6 +393,13 @@
>     Record.push_back(NumTemplateArgs);
>   }
> 
> +  DeclarationName::NameKind nk = (E->getDecl()->getDeclName().getNameKind());
> +
> +  if ((!E->hasExplicitTemplateArgs()) && (!E->hasQualifier()) && (!(E->getDecl() != E->getFoundDecl()))
> 
> Why not just use == for this last condition?

Fixed.

> +      && (nk == DeclarationName::Identifier || nk == DeclarationName::ObjCZeroArgSelector)) {
> +    AbbrevToUse = Writer.getDeclRefExprAbbrev();
> +  }
> +
> 
> Why is the DeclarationName::ObjCZeroArgSelector check here?

There are a group of DeclarationNames that don't use source location information (which in turn complicates the abbreviation) in ASTWriter::AddDeclarationNameLoc: Identifier, ObjCZeroArgSelector, ObjCOneArgSelector, ObjCMultiArgSelector, and CXXUsingDirective.  

Having said that, it might make sense to deal with these other cases in another patch.  Removing the support for ObjCZeroArgSelector in this patch doesn't affect any of the abbreviation performance numbers, so I've gone ahead and removed it.

> @@ -411,6 +419,11 @@
>   VisitExpr(E);
>   Writer.AddSourceLocation(E->getLocation(), Record);
>   Writer.AddAPInt(E->getValue(), Record);
> +
> +  if (E->getValue().getBitWidth() == 32) {
> +    AbbrevToUse = Writer.getIntegerLiteralAbbrev();
> +  }
> +
>   Code = serialization::EXPR_INTEGER_LITERAL;
> }
> 
> Are we getting a decent abbreviation percentage for just 32-bit integer literals? It  seems to me like we've have a lot of 'long' literals as well in headers, particularly when we're dealing with the null pointer constant 0L when compiling for a 64-bit platform.

A good point.  It seems that, for now, 32-bit is the preferred one (see below).  Once I start on variable-sized abbreviations working, it might make sense to revisit this and make it more agnostic.

32-bit integer literal percentages: C++ string: 95.18%, C++ iostream: 94.07%, stdio.h: 100%, ObjC Cocoa.h: 96.86%, and ObjC WebKit.h: 96.89%. 

Jonathan

-------------- next part --------------
A non-text attachment was scrubbed...
Name: clang_pch_abbrev.patch
Type: application/octet-stream
Size: 15642 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20110601/a4af44bd/attachment.obj>