r239605 - [clang-format] Reorder and pack ParenState members to minimize padding

Manuel Klimek klimek at google.com
Mon Jun 15 04:10:09 PDT 2015 

On Mon, Jun 15, 2015 at 12:56 PM Benjamin Kramer <benny.kra at gmail.com>
wrote:

> On Mon, Jun 15, 2015 at 6:30 AM, Manuel Klimek <klimek at google.com> wrote:
> > Would have been interesting in the description to mention why it
> matters. 10
> > bytes don't sound like much :)
>
> We copy millions of ParenState objects around. On formatting
> X86ISelLowering.cpp this change saves 25 MB of vector allocations,
> which is about 7-8% of the total memory allocations performed by
> clang-format on that file. Peak memory doesn't change significantly
> though.
>

Curious that peak memory doesn't change. Probably because we always need to
push_back and can't know how many are in there when we add them in
LineState. Would be interesting whether we can make this change have impact
on peak memory by changing the LineState.Stack to some different data
structure (perhaps SmallVector might already be enough, as depths more than
5 seem unlikely)


>
> - Ben
>
> > On Fri, Jun 12, 2015, 4:47 PM Benjamin Kramer <benny.kra at gmail.com>
> wrote:
> >>
> >> On Fri, Jun 12, 2015 at 4:28 PM, Daniel Jasper <djasper at google.com>
> wrote:
> >> > Thanks.. I also think we should simply use in-class initializers here
> >> > instead of the constructor initializers where we constantly have to
> fix
> >> > the
> >> > order.
> >>
> >> Did so in r239606 for the integer members. Bitfields don't support
> >> in-class initializers in C++11 :(
> >>
> >> - Ben
> >>
> >> > On Fri, Jun 12, 2015 at 3:07 PM, Benjamin Kramer
> >> > <benny.kra at googlemail.com>
> >> > wrote:
> >> >>
> >> >> Author: d0k
> >> >> Date: Fri Jun 12 08:07:03 2015
> >> >> New Revision: 239605
> >> >>
> >> >> URL: http://llvm.org/viewvc/llvm-project?rev=239605&view=rev
> >> >> Log:
> >> >> [clang-format] Reorder and pack ParenState members to minimize
> padding
> >> >>
> >> >> sizeof(ParenState) goes from 64 bytes to 52 bytes. NFC.
> >> >>
> >> >> Modified:
> >> >>     cfe/trunk/lib/Format/ContinuationIndenter.h
> >> >>
> >> >> Modified: cfe/trunk/lib/Format/ContinuationIndenter.h
> >> >> URL:
> >> >>
> >> >>
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Format/ContinuationIndenter.h?rev=239605&r1=239604&r2=239605&view=diff
> >> >>
> >> >>
> >> >>
> ==============================================================================
> >> >> --- cfe/trunk/lib/Format/ContinuationIndenter.h (original)
> >> >> +++ cfe/trunk/lib/Format/ContinuationIndenter.h Fri Jun 12 08:07:03
> >> >> 2015
> >> >> @@ -148,15 +148,15 @@ struct ParenState {
> >> >>    ParenState(unsigned Indent, unsigned IndentLevel, unsigned
> >> >> LastSpace,
> >> >>               bool AvoidBinPacking, bool NoLineBreak)
> >> >>        : Indent(Indent), IndentLevel(IndentLevel),
> >> >> LastSpace(LastSpace),
> >> >> -        NestedBlockIndent(Indent), FirstLessLess(0),
> >> >> -        BreakBeforeClosingBrace(false), QuestionColumn(0),
> >> >> -        AvoidBinPacking(AvoidBinPacking),
> BreakBeforeParameter(false),
> >> >> -        NoLineBreak(NoLineBreak), LastOperatorWrapped(true),
> >> >> ColonPos(0),
> >> >> -        StartOfFunctionCall(0), StartOfArraySubscripts(0),
> >> >> +        NestedBlockIndent(Indent), FirstLessLess(0),
> >> >> QuestionColumn(0),
> >> >> +        ColonPos(0), StartOfFunctionCall(0),
> >> >> StartOfArraySubscripts(0),
> >> >>          NestedNameSpecifierContinuation(0), CallContinuation(0),
> >> >> VariablePos(0),
> >> >> -        ContainsLineBreak(false), ContainsUnwrappedBuilder(0),
> >> >> -        AlignColons(true), ObjCSelectorNameFound(false),
> >> >> -        HasMultipleNestedBlocks(false), NestedBlockInlined(false) {}
> >> >> +        BreakBeforeClosingBrace(false),
> >> >> AvoidBinPacking(AvoidBinPacking),
> >> >> +        BreakBeforeParameter(false), NoLineBreak(NoLineBreak),
> >> >> +        LastOperatorWrapped(true), ContainsLineBreak(false),
> >> >> +        ContainsUnwrappedBuilder(false), AlignColons(true),
> >> >> +        ObjCSelectorNameFound(false),
> HasMultipleNestedBlocks(false),
> >> >> +        NestedBlockInlined(false) {}
> >> >>
> >> >>    /// \brief The position to which a specific parenthesis level
> needs
> >> >> to
> >> >> be
> >> >>    /// indented.
> >> >> @@ -182,31 +182,9 @@ struct ParenState {
> >> >>    /// on a level.
> >> >>    unsigned FirstLessLess;
> >> >>
> >> >> -  /// \brief Whether a newline needs to be inserted before the
> block's
> >> >> closing
> >> >> -  /// brace.
> >> >> -  ///
> >> >> -  /// We only want to insert a newline before the closing brace if
> >> >> there
> >> >> also
> >> >> -  /// was a newline after the beginning left brace.
> >> >> -  bool BreakBeforeClosingBrace;
> >> >> -
> >> >>    /// \brief The column of a \c ? in a conditional expression;
> >> >>    unsigned QuestionColumn;
> >> >>
> >> >> -  /// \brief Avoid bin packing, i.e. multiple parameters/elements on
> >> >> multiple
> >> >> -  /// lines, in this context.
> >> >> -  bool AvoidBinPacking;
> >> >> -
> >> >> -  /// \brief Break after the next comma (or all the commas in this
> >> >> context if
> >> >> -  /// \c AvoidBinPacking is \c true).
> >> >> -  bool BreakBeforeParameter;
> >> >> -
> >> >> -  /// \brief Line breaking in this context would break a formatting
> >> >> rule.
> >> >> -  bool NoLineBreak;
> >> >> -
> >> >> -  /// \brief True if the last binary operator on this level was
> >> >> wrapped
> >> >> to the
> >> >> -  /// next line.
> >> >> -  bool LastOperatorWrapped;
> >> >> -
> >> >>    /// \brief The position of the colon in an ObjC method
> >> >> declaration/call.
> >> >>    unsigned ColonPos;
> >> >>
> >> >> @@ -230,40 +208,62 @@ struct ParenState {
> >> >>    /// Used to align further variables if necessary.
> >> >>    unsigned VariablePos;
> >> >>
> >> >> +  /// \brief Whether a newline needs to be inserted before the
> block's
> >> >> closing
> >> >> +  /// brace.
> >> >> +  ///
> >> >> +  /// We only want to insert a newline before the closing brace if
> >> >> there
> >> >> also
> >> >> +  /// was a newline after the beginning left brace.
> >> >> +  bool BreakBeforeClosingBrace : 1;
> >> >> +
> >> >> +  /// \brief Avoid bin packing, i.e. multiple parameters/elements on
> >> >> multiple
> >> >> +  /// lines, in this context.
> >> >> +  bool AvoidBinPacking : 1;
> >> >> +
> >> >> +  /// \brief Break after the next comma (or all the commas in this
> >> >> context if
> >> >> +  /// \c AvoidBinPacking is \c true).
> >> >> +  bool BreakBeforeParameter : 1;
> >> >> +
> >> >> +  /// \brief Line breaking in this context would break a formatting
> >> >> rule.
> >> >> +  bool NoLineBreak : 1;
> >> >> +
> >> >> +  /// \brief True if the last binary operator on this level was
> >> >> wrapped
> >> >> to the
> >> >> +  /// next line.
> >> >> +  bool LastOperatorWrapped : 1;
> >> >> +
> >> >>    /// \brief \c true if this \c ParenState already contains a
> >> >> line-break.
> >> >>    ///
> >> >>    /// The first line break in a certain \c ParenState causes extra
> >> >> penalty so
> >> >>    /// that clang-format prefers similar breaks, i.e. breaks in the
> >> >> same
> >> >>    /// parenthesis.
> >> >> -  bool ContainsLineBreak;
> >> >> +  bool ContainsLineBreak : 1;
> >> >>
> >> >>    /// \brief \c true if this \c ParenState contains multiple
> segments
> >> >> of
> >> >> a
> >> >>    /// builder-type call on one line.
> >> >> -  bool ContainsUnwrappedBuilder;
> >> >> +  bool ContainsUnwrappedBuilder : 1;
> >> >>
> >> >>    /// \brief \c true if the colons of the curren ObjC method
> >> >> expression
> >> >> should
> >> >>    /// be aligned.
> >> >>    ///
> >> >>    /// Not considered for memoization as it will always have the same
> >> >> value at
> >> >>    /// the same token.
> >> >> -  bool AlignColons;
> >> >> +  bool AlignColons : 1;
> >> >>
> >> >>    /// \brief \c true if at least one selector name was found in the
> >> >> current
> >> >>    /// ObjC method expression.
> >> >>    ///
> >> >>    /// Not considered for memoization as it will always have the same
> >> >> value at
> >> >>    /// the same token.
> >> >> -  bool ObjCSelectorNameFound;
> >> >> +  bool ObjCSelectorNameFound : 1;
> >> >>
> >> >>    /// \brief \c true if there are multiple nested blocks inside
> these
> >> >> parens.
> >> >>    ///
> >> >>    /// Not considered for memoization as it will always have the same
> >> >> value at
> >> >>    /// the same token.
> >> >> -  bool HasMultipleNestedBlocks;
> >> >> +  bool HasMultipleNestedBlocks : 1;
> >> >>
> >> >>    // \brief The start of a nested block (e.g. lambda introducer in
> C++
> >> >> or
> >> >>    // "function" in JavaScript) is not wrapped to a new line.
> >> >> -  bool NestedBlockInlined;
> >> >> +  bool NestedBlockInlined : 1;
> >> >>
> >> >>    bool operator<(const ParenState &Other) const {
> >> >>      if (Indent != Other.Indent)
> >> >>
> >> >>
> >> >> _______________________________________________
> >> >> cfe-commits mailing list
> >> >> cfe-commits at cs.uiuc.edu
> >> >> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
> >> >
> >> >
> >> _______________________________________________
> >> cfe-commits mailing list
> >> cfe-commits at cs.uiuc.edu
> >> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150615/31a938fb/attachment.html>

More information about the cfe-commits mailing list