<div dir="ltr">Too bad. Thx for explaining...<br></div><br><div class="gmail_quote"><div dir="ltr">On Mon, Jun 15, 2015 at 1:30 PM Benjamin Kramer <<a href="mailto:benny.kra@gmail.com">benny.kra@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On Mon, Jun 15, 2015 at 1:10 PM, Manuel Klimek <<a href="mailto:klimek@google.com" target="_blank">klimek@google.com</a>> wrote:<br>
> On Mon, Jun 15, 2015 at 12:56 PM Benjamin Kramer <<a href="mailto:benny.kra@gmail.com" target="_blank">benny.kra@gmail.com</a>><br>
> wrote:<br>
>><br>
>> On Mon, Jun 15, 2015 at 6:30 AM, Manuel Klimek <<a href="mailto:klimek@google.com" target="_blank">klimek@google.com</a>> wrote:<br>
>> > Would have been interesting in the description to mention why it<br>
>> > matters. 10<br>
>> > bytes don't sound like much :)<br>
>><br>
>> We copy millions of ParenState objects around. On formatting<br>
>> X86ISelLowering.cpp this change saves 25 MB of vector allocations,<br>
>> which is about 7-8% of the total memory allocations performed by<br>
>> clang-format on that file. Peak memory doesn't change significantly<br>
>> though.<br>
><br>
><br>
> Curious that peak memory doesn't change. Probably because we always need to<br>
> push_back and can't know how many are in there when we add them in<br>
> LineState. Would be interesting whether we can make this change have impact<br>
> on peak memory by changing the LineState.Stack to some different data<br>
> structure (perhaps SmallVector might already be enough, as depths more than<br>
> 5 seem unlikely)<br>
<br>
The problem with SmallVector is that it bloats LineState enough to<br>
kill any gain from saving ParenState allocations. Also in this file<br>
the peak memory is dominated by FormatTokens (~45%),<br>
WhiteSpaceManger::Change (~30%) and UnwrappedLineNodes (~15%) so<br>
changes here won't have a large effect on peak anyways.<br>
<br>
- Ben<br>
<br>
>><br>
>> > On Fri, Jun 12, 2015, 4:47 PM Benjamin Kramer <<a href="mailto:benny.kra@gmail.com" target="_blank">benny.kra@gmail.com</a>><br>
>> > wrote:<br>
>> >><br>
>> >> On Fri, Jun 12, 2015 at 4:28 PM, Daniel Jasper <<a href="mailto:djasper@google.com" target="_blank">djasper@google.com</a>><br>
>> >> wrote:<br>
>> >> > Thanks.. I also think we should simply use in-class initializers here<br>
>> >> > instead of the constructor initializers where we constantly have to<br>
>> >> > fix<br>
>> >> > the<br>
>> >> > order.<br>
>> >><br>
>> >> Did so in r239606 for the integer members. Bitfields don't support<br>
>> >> in-class initializers in C++11 :(<br>
>> >><br>
>> >> - Ben<br>
>> >><br>
>> >> > On Fri, Jun 12, 2015 at 3:07 PM, Benjamin Kramer<br>
>> >> > <<a href="mailto:benny.kra@googlemail.com" target="_blank">benny.kra@googlemail.com</a>><br>
>> >> > wrote:<br>
>> >> >><br>
>> >> >> Author: d0k<br>
>> >> >> Date: Fri Jun 12 08:07:03 2015<br>
>> >> >> New Revision: 239605<br>
>> >> >><br>
>> >> >> URL: <a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_viewvc_llvm-2Dproject-3Frev-3D239605-26view-3Drev&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=BSqEv9KvKMW_Ob8SyngJ70KdZISM_ASROnREeq0cCxk&m=Tt00ZYMukZMwbwFcoB0h8g6XPpVz64iFUlvDVK8MY5g&s=at0bUtLFWmGg5Rbb4MVecJA8x8yRAZQYGESxAPPg3zI&e=" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=239605&view=rev</a><br>
>> >> >> Log:<br>
>> >> >> [clang-format] Reorder and pack ParenState members to minimize<br>
>> >> >> padding<br>
>> >> >><br>
>> >> >> sizeof(ParenState) goes from 64 bytes to 52 bytes. NFC.<br>
>> >> >><br>
>> >> >> Modified:<br>
>> >> >>     cfe/trunk/lib/Format/ContinuationIndenter.h<br>
>> >> >><br>
>> >> >> Modified: cfe/trunk/lib/Format/ContinuationIndenter.h<br>
>> >> >> URL:<br>
>> >> >><br>
>> >> >><br>
>> >> >> <a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_viewvc_llvm-2Dproject_cfe_trunk_lib_Format_ContinuationIndenter.h-3Frev-3D239605-26r1-3D239604-26r2-3D239605-26view-3Ddiff&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=BSqEv9KvKMW_Ob8SyngJ70KdZISM_ASROnREeq0cCxk&m=Tt00ZYMukZMwbwFcoB0h8g6XPpVz64iFUlvDVK8MY5g&s=a-L1rAiBnQCLsZ8_dDznd55b0EHXqW3zVg_kxjoC2PU&e=" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Format/ContinuationIndenter.h?rev=239605&r1=239604&r2=239605&view=diff</a><br>
>> >> >><br>
>> >> >><br>
>> >> >><br>
>> >> >> ==============================================================================<br>
>> >> >> --- cfe/trunk/lib/Format/ContinuationIndenter.h (original)<br>
>> >> >> +++ cfe/trunk/lib/Format/ContinuationIndenter.h Fri Jun 12 08:07:03<br>
>> >> >> 2015<br>
>> >> >> @@ -148,15 +148,15 @@ struct ParenState {<br>
>> >> >>    ParenState(unsigned Indent, unsigned IndentLevel, unsigned<br>
>> >> >> LastSpace,<br>
>> >> >>               bool AvoidBinPacking, bool NoLineBreak)<br>
>> >> >>        : Indent(Indent), IndentLevel(IndentLevel),<br>
>> >> >> LastSpace(LastSpace),<br>
>> >> >> -        NestedBlockIndent(Indent), FirstLessLess(0),<br>
>> >> >> -        BreakBeforeClosingBrace(false), QuestionColumn(0),<br>
>> >> >> -        AvoidBinPacking(AvoidBinPacking),<br>
>> >> >> BreakBeforeParameter(false),<br>
>> >> >> -        NoLineBreak(NoLineBreak), LastOperatorWrapped(true),<br>
>> >> >> ColonPos(0),<br>
>> >> >> -        StartOfFunctionCall(0), StartOfArraySubscripts(0),<br>
>> >> >> +        NestedBlockIndent(Indent), FirstLessLess(0),<br>
>> >> >> QuestionColumn(0),<br>
>> >> >> +        ColonPos(0), StartOfFunctionCall(0),<br>
>> >> >> StartOfArraySubscripts(0),<br>
>> >> >>          NestedNameSpecifierContinuation(0), CallContinuation(0),<br>
>> >> >> VariablePos(0),<br>
>> >> >> -        ContainsLineBreak(false), ContainsUnwrappedBuilder(0),<br>
>> >> >> -        AlignColons(true), ObjCSelectorNameFound(false),<br>
>> >> >> -        HasMultipleNestedBlocks(false), NestedBlockInlined(false)<br>
>> >> >> {}<br>
>> >> >> +        BreakBeforeClosingBrace(false),<br>
>> >> >> AvoidBinPacking(AvoidBinPacking),<br>
>> >> >> +        BreakBeforeParameter(false), NoLineBreak(NoLineBreak),<br>
>> >> >> +        LastOperatorWrapped(true), ContainsLineBreak(false),<br>
>> >> >> +        ContainsUnwrappedBuilder(false), AlignColons(true),<br>
>> >> >> +        ObjCSelectorNameFound(false),<br>
>> >> >> HasMultipleNestedBlocks(false),<br>
>> >> >> +        NestedBlockInlined(false) {}<br>
>> >> >><br>
>> >> >>    /// \brief The position to which a specific parenthesis level<br>
>> >> >> needs<br>
>> >> >> to<br>
>> >> >> be<br>
>> >> >>    /// indented.<br>
>> >> >> @@ -182,31 +182,9 @@ struct ParenState {<br>
>> >> >>    /// on a level.<br>
>> >> >>    unsigned FirstLessLess;<br>
>> >> >><br>
>> >> >> -  /// \brief Whether a newline needs to be inserted before the<br>
>> >> >> block's<br>
>> >> >> closing<br>
>> >> >> -  /// brace.<br>
>> >> >> -  ///<br>
>> >> >> -  /// We only want to insert a newline before the closing brace if<br>
>> >> >> there<br>
>> >> >> also<br>
>> >> >> -  /// was a newline after the beginning left brace.<br>
>> >> >> -  bool BreakBeforeClosingBrace;<br>
>> >> >> -<br>
>> >> >>    /// \brief The column of a \c ? in a conditional expression;<br>
>> >> >>    unsigned QuestionColumn;<br>
>> >> >><br>
>> >> >> -  /// \brief Avoid bin packing, i.e. multiple parameters/elements<br>
>> >> >> on<br>
>> >> >> multiple<br>
>> >> >> -  /// lines, in this context.<br>
>> >> >> -  bool AvoidBinPacking;<br>
>> >> >> -<br>
>> >> >> -  /// \brief Break after the next comma (or all the commas in this<br>
>> >> >> context if<br>
>> >> >> -  /// \c AvoidBinPacking is \c true).<br>
>> >> >> -  bool BreakBeforeParameter;<br>
>> >> >> -<br>
>> >> >> -  /// \brief Line breaking in this context would break a formatting<br>
>> >> >> rule.<br>
>> >> >> -  bool NoLineBreak;<br>
>> >> >> -<br>
>> >> >> -  /// \brief True if the last binary operator on this level was<br>
>> >> >> wrapped<br>
>> >> >> to the<br>
>> >> >> -  /// next line.<br>
>> >> >> -  bool LastOperatorWrapped;<br>
>> >> >> -<br>
>> >> >>    /// \brief The position of the colon in an ObjC method<br>
>> >> >> declaration/call.<br>
>> >> >>    unsigned ColonPos;<br>
>> >> >><br>
>> >> >> @@ -230,40 +208,62 @@ struct ParenState {<br>
>> >> >>    /// Used to align further variables if necessary.<br>
>> >> >>    unsigned VariablePos;<br>
>> >> >><br>
>> >> >> +  /// \brief Whether a newline needs to be inserted before the<br>
>> >> >> block's<br>
>> >> >> closing<br>
>> >> >> +  /// brace.<br>
>> >> >> +  ///<br>
>> >> >> +  /// We only want to insert a newline before the closing brace if<br>
>> >> >> there<br>
>> >> >> also<br>
>> >> >> +  /// was a newline after the beginning left brace.<br>
>> >> >> +  bool BreakBeforeClosingBrace : 1;<br>
>> >> >> +<br>
>> >> >> +  /// \brief Avoid bin packing, i.e. multiple parameters/elements<br>
>> >> >> on<br>
>> >> >> multiple<br>
>> >> >> +  /// lines, in this context.<br>
>> >> >> +  bool AvoidBinPacking : 1;<br>
>> >> >> +<br>
>> >> >> +  /// \brief Break after the next comma (or all the commas in this<br>
>> >> >> context if<br>
>> >> >> +  /// \c AvoidBinPacking is \c true).<br>
>> >> >> +  bool BreakBeforeParameter : 1;<br>
>> >> >> +<br>
>> >> >> +  /// \brief Line breaking in this context would break a formatting<br>
>> >> >> rule.<br>
>> >> >> +  bool NoLineBreak : 1;<br>
>> >> >> +<br>
>> >> >> +  /// \brief True if the last binary operator on this level was<br>
>> >> >> wrapped<br>
>> >> >> to the<br>
>> >> >> +  /// next line.<br>
>> >> >> +  bool LastOperatorWrapped : 1;<br>
>> >> >> +<br>
>> >> >>    /// \brief \c true if this \c ParenState already contains a<br>
>> >> >> line-break.<br>
>> >> >>    ///<br>
>> >> >>    /// The first line break in a certain \c ParenState causes extra<br>
>> >> >> penalty so<br>
>> >> >>    /// that clang-format prefers similar breaks, i.e. breaks in the<br>
>> >> >> same<br>
>> >> >>    /// parenthesis.<br>
>> >> >> -  bool ContainsLineBreak;<br>
>> >> >> +  bool ContainsLineBreak : 1;<br>
>> >> >><br>
>> >> >>    /// \brief \c true if this \c ParenState contains multiple<br>
>> >> >> segments<br>
>> >> >> of<br>
>> >> >> a<br>
>> >> >>    /// builder-type call on one line.<br>
>> >> >> -  bool ContainsUnwrappedBuilder;<br>
>> >> >> +  bool ContainsUnwrappedBuilder : 1;<br>
>> >> >><br>
>> >> >>    /// \brief \c true if the colons of the curren ObjC method<br>
>> >> >> expression<br>
>> >> >> should<br>
>> >> >>    /// be aligned.<br>
>> >> >>    ///<br>
>> >> >>    /// Not considered for memoization as it will always have the<br>
>> >> >> same<br>
>> >> >> value at<br>
>> >> >>    /// the same token.<br>
>> >> >> -  bool AlignColons;<br>
>> >> >> +  bool AlignColons : 1;<br>
>> >> >><br>
>> >> >>    /// \brief \c true if at least one selector name was found in the<br>
>> >> >> current<br>
>> >> >>    /// ObjC method expression.<br>
>> >> >>    ///<br>
>> >> >>    /// Not considered for memoization as it will always have the<br>
>> >> >> same<br>
>> >> >> value at<br>
>> >> >>    /// the same token.<br>
>> >> >> -  bool ObjCSelectorNameFound;<br>
>> >> >> +  bool ObjCSelectorNameFound : 1;<br>
>> >> >><br>
>> >> >>    /// \brief \c true if there are multiple nested blocks inside<br>
>> >> >> these<br>
>> >> >> parens.<br>
>> >> >>    ///<br>
>> >> >>    /// Not considered for memoization as it will always have the<br>
>> >> >> same<br>
>> >> >> value at<br>
>> >> >>    /// the same token.<br>
>> >> >> -  bool HasMultipleNestedBlocks;<br>
>> >> >> +  bool HasMultipleNestedBlocks : 1;<br>
>> >> >><br>
>> >> >>    // \brief The start of a nested block (e.g. lambda introducer in<br>
>> >> >> C++<br>
>> >> >> or<br>
>> >> >>    // "function" in JavaScript) is not wrapped to a new line.<br>
>> >> >> -  bool NestedBlockInlined;<br>
>> >> >> +  bool NestedBlockInlined : 1;<br>
>> >> >><br>
>> >> >>    bool operator<(const ParenState &Other) const {<br>
>> >> >>      if (Indent != Other.Indent)<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" rel="noreferrer" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><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" rel="noreferrer" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
</blockquote></div>