r239605 - [clang-format] Reorder and pack ParenState members to minimize padding
Manuel Klimek
klimek at google.com
Mon Jun 15 04:32:39 PDT 2015
Too bad. Thx for explaining...
On Mon, Jun 15, 2015 at 1:30 PM Benjamin Kramer <benny.kra at gmail.com> wrote:
> On Mon, Jun 15, 2015 at 1:10 PM, Manuel Klimek <klimek at google.com> wrote:
> > 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)
>
> The problem with SmallVector is that it bloats LineState enough to
> kill any gain from saving ParenState allocations. Also in this file
> the peak memory is dominated by FormatTokens (~45%),
> WhiteSpaceManger::Change (~30%) and UnwrappedLineNodes (~15%) so
> changes here won't have a large effect on peak anyways.
>
> - 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/54bad47e/attachment.html>
More information about the cfe-commits
mailing list