pahole changes in various

I'm not seeing as much as I would expect here, mostly because pahole
seems to be having trouble parsing quite a few of our structures, and
consequently producing useless data than I then ignore.

XDash 24bytes -> 20bytes
vcl::font::FeatureDefinition 64bytes -> 56bytes
SvXMLTokenMapEntry 16bytes -> 12bytes
SvXMLItemMapEntry 16bytes -> 12bytes
SwContentAtPos 40bytes -> 32bytes

Change-Id: I74c8b93f74b8352f48ef552d7d4239aa7f4237d4
Reviewed-on: https://gerrit.libreoffice.org/69304
Tested-by: Jenkins
Reviewed-by: Noel Grandin <noel.grandin@collabora.co.uk>

compilerplugins/clang/pahole-all-classes.py

@@ -3,11 +3,15 @@
 # Find holes in structures, so that we can pack them and improve our memory density.
 #
 # In order to make this work, you need to
-# (1) be operating in a workspace where you have a debug build of LibreOffice
-# (2) first run the unusedfields loplugin to generate a log file
-# (3) install the pahole stuff into your gdb, I used this one: https://github.com/PhilArmstrong/pahole-gdb
-# (4) ./compilerplugins/clang/pahole-all-classes.py > ./compilerplugins/clang/pahole.results
-#     Warning: running this script will make GDB soak up about 8G of RAM
+# (1) Be operating in a workspace where you have a __NON-DEBUG__ build of LibreOffice, but __WITH SYMBOLS__.
+#     (A debug build has different sizes for some things in the standard library.)
+# (2) First run the unusedfields loplugin to generate a log file
+# (3) Install the pahole stuff into your gdb, I used this one:
+#     https://github.com/PhilArmstrong/pahole-gdb
+# (4) Edit the loop near the top of the script to only produce results for one of our modules.
+#     Note that this will make GDB soak up about 8G of RAM, which is why I don't do more than one module at a time
+# (5) Run the script
+#     ./compilerplugins/clang/pahole-all-classes.py > ./compilerplugins/clang/pahole.results
 #
 
 import _thread
@@ -19,7 +23,7 @@ import re
 
 # search for all the class names in the file produced by the unusedfields loplugin
 #a = subprocess.Popen("grep 'definition:' workdir/loplugin.unusedfields.log | sort -u", stdout=subprocess.PIPE, shell=True)
-a = subprocess.Popen("cat n1", stdout=subprocess.PIPE, shell=True)
+a = subprocess.Popen("cat ../libo/n1", stdout=subprocess.PIPE, shell=True)
 
 classSourceLocDict = dict()
 classSet = set()
@@ -28,13 +32,14 @@ with a.stdout as txt:
         tokens = line.decode('utf8').strip().split("\t")
         className = tokens[2].strip()
         srcLoc = tokens[5].strip()
+        # ignore things like unions
         if "anonymous" in className: continue
-        # for now, just check the stuff in /sc/inc
-        if not srcLoc.startswith("sc/inc/"):
-                continue
+        # ignore duplicates
         if className in classSet: continue
-        classSourceLocDict[srcLoc] = className
-        classSet.add(className)
+        # for now, just check the stuff in /sc/inc
+        if srcLoc.startswith("a"):
+            classSourceLocDict[srcLoc] = className
+            classSet.add(className)
 a.terminate()
 
 gdbProc = subprocess.Popen("gdb", stdin=subprocess.PIPE, stdout=subprocess.PIPE, stderr=subprocess.STDOUT, shell=True)
@@ -65,8 +70,6 @@ def write_pahole_commands():
 
 _thread.start_new_thread( write_pahole_commands, () )
 
-time.sleep(2)
-
 # Use generator because lines often end up merged together in gdb's output, and we need
 # to split them up, and that creates a mess in the parsing logic.
 def read_generator():
@@ -79,13 +82,17 @@ def read_generator():
             yield split
 
 firstLineRegex = re.compile("/\*\s+(\d+)\s+\*/ struct")
-fieldLineRegex = re.compile("/\*\s+\d+\s+(\d+)\s+\*/ ")
+fieldLineRegex = re.compile("/\*\s+(\d+)\s+(\d+)\s+\*/ ")
 holeLineRegex = re.compile("/\* XXX (\d+) bit hole, try to pack \*/")
+# sometimes pahole can't determine the size of a sub-struct, and then it returns bad data
+bogusLineRegex = re.compile("/\*\s+\d+\s+0\s+\*/")
 structLines = list()
 foundHole = False
 cumulativeHoleBits = 0
 structSize = 0
-found8ByteField = False
+foundBogusLine = False
+# pahole doesn't report space at the end of the structure, so work it out myself
+sizeOfFields = 0
 for line in read_generator():
     structLines.append(line)
     firstLineMatch = firstLineRegex.match(line)
@@ -97,27 +104,30 @@ for line in read_generator():
         cumulativeHoleBits += int(holeLineMatch.group(1))
     fieldLineMatch = fieldLineRegex.match(line)
     if fieldLineMatch:
-        fieldSize = int(fieldLineMatch.group(1))
-        if fieldSize == 8:
-            found8ByteField = True
+        fieldSize = int(fieldLineMatch.group(2))
+        sizeOfFields = int(fieldLineMatch.group(1)) + fieldSize
+    if bogusLineRegex.match(line):
+        foundBogusLine = True
     if line == "}":
         # Ignore very large structs, packing those is not going to help much, and
         # re-organising them can make them much less readable.
-        if foundHole and len(structLines) < 12 and structSize < 100:
-            # If we have an 8-byte field, then the whole structure must be 8-byte aligned, otherwise
-            # it must be 4-byte aligned. (that's my approximation of the rules, the real ones are probably
-            # more complicated). So check if removing the holes will remove enough space to actually shrink
-            # this structure.
-            alignBytes = 4
-            if found8ByteField: alignBytes = 8
-            if (cumulativeHoleBits / 8) >= alignBytes:
-                # print("Found one " + str(structSize) + " " + str(cumulativeHoleBits/8) + " " + str(newStructSize%4))
+        if foundHole and len(structLines) < 12 and structSize < 100 and not foundBogusLine:
+            # Verify that we have enough hole-space that removing it will result in a structure
+            # that still satifies alignment requirements, otherwise the compiler will just put empty
+            # space at the end of the struct.
+            # TODO improve detection of the required alignment for a structure
+            potentialSpace = (cumulativeHoleBits / 8) + (sizeOfFields - structSize)
+            if potentialSpace >= 8:
                 for line in structLines:
                     print(line)
+                if (sizeOfFields - structSize) > 0:
+                    print("hole at end of struct: " + str(sizeOfFields - structSize))
+        #  reset state
         structLines.clear()
         foundHole = False
         cumulativeHoleBits = 0
         structSize = 0
-        found8ByteField = False
+        foundBogusLine = False
+        actualStructSize = 0
 
 gdbProc.terminate()
\ No newline at end of file

compilerplugins/clang/pahole.results

-ScColorScaleEntry sc/inc/colorscale.hxx:46
-/*   48     */ struct ScColorScaleEntry {
-/*   0    8 */    double mnVal
-/*   8    4 */    class Color maColor
-/* XXX 32 bit hole, try to pack */
-/*  16    8 */    class std::unique_ptr<ScFormulaCell, std::default_delete<ScFormulaCell> > mpCell
-/*  24    8 */    class std::unique_ptr<ScFormulaListener, std::default_delete<ScFormulaListener> > mpListener
-/*  32    4 */    enum ScColorScaleEntryType meType
-/* XXX 32 bit hole, try to pack */
-/*  40    8 */    class ScConditionalFormat * mpFormat
-}
-ScDetOpList sc/inc/detdata.hxx:66
-/*   64     */ struct ScDetOpList {
-/*   0    1 */    bool bHasAddError
-/* XXX 56 bit hole, try to pack */
-/*   8   56 */    class std::__debug::vector<std::unique_ptr<ScDetOpData, std::default_delete<ScDetOpData> >, std::allocator<std::unique_ptr<ScDetOpData, std::default_delete<ScDetOpData> > > > aDetOpDataVector
-}
-ScDocRowHeightUpdater::TabRanges sc/inc/dociter.hxx:569
-/*   24     */ struct ScDocRowHeightUpdater::TabRanges {
-/*   0    2 */    short mnTab
-/* XXX 48 bit hole, try to pack */
-/*   8   16 */    class std::shared_ptr<ScFlatBoolRowSegments> mpRanges
-}
-ScPivotField sc/inc/pivot.hxx:122
-/*   56     */ struct ScPivotField {
-/*   0    2 */    short nCol
-/* XXX 48 bit hole, try to pack */
-/*   8    8 */    long mnOriginalDim
-/*  16    4 */    enum PivotFunc nFuncMask
-/*  20    1 */    unsigned char mnDupCount
-/* XXX 24 bit hole, try to pack */
-/*  24   32 */    struct com::sun::star::sheet::DataPilotFieldReference maFieldRef
-}
-ScPivotFuncData sc/inc/pivot.hxx:164
-/*   56     */ struct ScPivotFuncData {
-/*   0    2 */    short mnCol
-/* XXX 48 bit hole, try to pack */
-/*   8    8 */    long mnOriginalDim
-/*  16    4 */    enum PivotFunc mnFuncMask
-/*  20    1 */    unsigned char mnDupCount
-/* XXX 24 bit hole, try to pack */
-/*  24   32 */    struct com::sun::star::sheet::DataPilotFieldReference maFieldRef
-}
-ScExternalSingleRefToken sc/inc/token.hxx:131
-/*   56     */ struct ScExternalSingleRefToken {
-/*   0   16 */    class formula::FormulaToken formula::FormulaToken
-/*  16    2 */    const unsigned short mnFileId
-/* XXX 48 bit hole, try to pack */
-/*  24   16 */    const class svl::SharedString maTabName
-/*  40   12 */    struct ScSingleRefData maSingleRef
-}
-ScExternalDoubleRefToken sc/inc/token.hxx:155
-/*   64     */ struct ScExternalDoubleRefToken {
-/*   0   16 */    class formula::FormulaToken formula::FormulaToken
-/*  16    2 */    const unsigned short mnFileId
-/* XXX 48 bit hole, try to pack */
-/*  24   16 */    const class svl::SharedString maTabName
-/*  40   24 */    struct ScComplexRefData maDoubleRef
-}
-ScExternalNameToken sc/inc/token.hxx:182
-/*   40     */ struct ScExternalNameToken {
-/*   0   16 */    class formula::FormulaToken formula::FormulaToken
-/*  16    2 */    const unsigned short mnFileId
-/* XXX 48 bit hole, try to pack */
-/*  24   16 */    const class svl::SharedString maName
-}

include/svx/xdash.hxx

@@ -33,8 +33,8 @@
 class SVX_DLLPUBLIC XDash final
 {
     css::drawing::DashStyle  eDash;
-    sal_uInt16               nDots;
     sal_uInt32               nDotLen;
+    sal_uInt16               nDots;
     sal_uInt16               nDashes;
     sal_uInt32               nDashLen;
     sal_uInt32               nDistance;

include/vcl/font/Feature.hxx

@@ -56,10 +56,10 @@ public:
 class VCL_DLLPUBLIC FeatureDefinition
 {
 private:
-    uint32_t m_nCode;
     OUString m_sDescription;
     const char* m_pDescriptionID;
     OUString m_sNumericPart;
+    uint32_t m_nCode;
     FeatureParameterType m_eType;
     // the index of the parameter defines the enum value, string is the description
     std::vector<FeatureParameter> m_aEnumParameters;

include/xmloff/xmltkmap.hxx

@@ -34,17 +34,17 @@ class SvXMLTokenMap_Impl;
 
 struct SvXMLTokenMapEntry
 {
-    sal_uInt16 const  nPrefixKey;
     enum xmloff::token::XMLTokenEnum const eLocalName;
-    sal_uInt16 const  nToken;
     sal_Int32 nFastToken;
+    sal_uInt16 const  nPrefixKey;
+    sal_uInt16 const  nToken;
 
     SvXMLTokenMapEntry( sal_uInt16 nPrefix, xmloff::token::XMLTokenEnum eName,
                         sal_uInt16 nTok, sal_Int32 nFastTok = 0 ) :
-        nPrefixKey( nPrefix ),
         eLocalName( eName ),
-        nToken( nTok ),
-        nFastToken( sal_uInt32( nPrefixKey + 1 ) << 16 | eLocalName )
+        nFastToken( sal_uInt32( nPrefix + 1 ) << 16 | eLocalName ),
+        nPrefixKey( nPrefix ),
+        nToken( nTok )
     {
         if ( nFastTok )     // alternative value for duplicate/dummy tokens
             nFastToken = nFastTok;

sc/inc/simplerangelist.hxx

@@ -37,10 +37,10 @@ class ScSimpleRangeList
 public:
     struct Range
     {
-        SCCOL mnCol1;
         SCROW mnRow1;
-        SCCOL mnCol2;
         SCROW mnRow2;
+        SCCOL mnCol1;
+        SCCOL mnCol2;
         explicit Range(SCCOL nCol1, SCROW nRow1, SCCOL nCol2, SCROW nRow2);
     };
     typedef std::shared_ptr< ::std::list<Range> > RangeListRef;

sc/source/core/tool/simplerangelist.cxx

@@ -29,7 +29,7 @@ using ::std::pair;
 using ::std::max;
 
 ScSimpleRangeList::Range::Range(SCCOL nCol1, SCROW nRow1, SCCOL nCol2, SCROW nRow2) :
-    mnCol1(nCol1), mnRow1(nRow1), mnCol2(nCol2), mnRow2(nRow2) {}
+    mnRow1(nRow1), mnRow2(nRow2), mnCol1(nCol1), mnCol2(nCol2) {}
 
 ScSimpleRangeList::ScSimpleRangeList()
 {

svx/source/xoutdev/xattr.cxx

@@ -384,8 +384,8 @@ sal_uInt16 XLineStyleItem::GetValueCount() const
 XDash::XDash(css::drawing::DashStyle eTheDash, sal_uInt16 nTheDots, sal_uInt32 nTheDotLen,
              sal_uInt16 nTheDashes, sal_uInt32 nTheDashLen, sal_uInt32 nTheDistance) :
     eDash(eTheDash),
-    nDots(nTheDots),
     nDotLen(nTheDotLen),
+    nDots(nTheDots),
     nDashes(nTheDashes),
     nDashLen(nTheDashLen),
     nDistance(nTheDistance)

sw/inc/crsrsh.hxx

@@ -95,8 +95,6 @@ namespace o3tl {
 
 struct SwContentAtPos
 {
-    IsAttrAtPos eContentAtPos;
-
     union {
         const SwField* pField;
         const SfxPoolItem* pAttr;
@@ -104,9 +102,8 @@ struct SwContentAtPos
         SwContentNode * pNode;
         const sw::mark::IFieldmark* pFieldmark;
     } aFnd;
-
+    IsAttrAtPos eContentAtPos;
     int nDist;
-
     OUString sStr;
     const SwTextAttr* pFndTextAttr;
 

sw/source/filter/xml/xmlitmap.hxx

@@ -40,15 +40,21 @@ struct SvXMLItemMapEntry
 {
     sal_uInt16 const nNameSpace;      // declares the Namespace in which this item
                                 // exists
+    sal_uInt16 nWhichId;        // the WhichId to identify the item
+                                // in the pool
     enum ::xmloff::token::XMLTokenEnum const eLocalName;
                                 // the local name for the item inside
                                 // the Namespace (as an XMLTokenEnum)
-    sal_uInt16 nWhichId;        // the WhichId to identify the item
-                                // in the pool
     sal_uInt32 const nMemberId;       // the memberid specifies which part
                                 // of the item should be imported or
                                 // exported with this Namespace
                                 // and localName
+    SvXMLItemMapEntry(
+        sal_uInt16 nameSpace,
+        enum ::xmloff::token::XMLTokenEnum localName,
+        sal_uInt16 whichId,
+        sal_uInt32 memberId)
+    : nNameSpace(nameSpace), nWhichId(whichId), eLocalName(localName), nMemberId(memberId) {}
 };
 
 class SvXMLItemMapEntries_impl;

vcl/source/font/Feature.cxx

@@ -92,8 +92,8 @@ uint32_t FeatureParameter::getCode() const { return m_nCode; }
 // FeatureDefinition
 
 FeatureDefinition::FeatureDefinition()
-    : m_nCode(0)
-    , m_pDescriptionID(nullptr)
+    : m_pDescriptionID(nullptr)
+    , m_nCode(0)
     , m_eType(FeatureParameterType::BOOL)
 {
 }
@@ -101,9 +101,9 @@ FeatureDefinition::FeatureDefinition()
 FeatureDefinition::FeatureDefinition(uint32_t nCode, OUString const& rDescription,
                                      FeatureParameterType eType,
                                      std::vector<FeatureParameter> const& rEnumParameters)
-    : m_nCode(nCode)
-    , m_sDescription(rDescription)
+    : m_sDescription(rDescription)
     , m_pDescriptionID(nullptr)
+    , m_nCode(nCode)
     , m_eType(eType)
     , m_aEnumParameters(rEnumParameters)
 {
@@ -111,17 +111,17 @@ FeatureDefinition::FeatureDefinition(uint32_t nCode, OUString const& rDescriptio
 
 FeatureDefinition::FeatureDefinition(uint32_t nCode, const char* pDescriptionID,
                                      OUString const& rNumericPart)
-    : m_nCode(nCode)
-    , m_pDescriptionID(pDescriptionID)
+    : m_pDescriptionID(pDescriptionID)
     , m_sNumericPart(rNumericPart)
+    , m_nCode(nCode)
     , m_eType(FeatureParameterType::BOOL)
 {
 }
 
 FeatureDefinition::FeatureDefinition(uint32_t nCode, const char* pDescriptionID,
                                      std::vector<FeatureParameter> aEnumParameters)
-    : m_nCode(nCode)
-    , m_pDescriptionID(pDescriptionID)
+    : m_pDescriptionID(pDescriptionID)
+    , m_nCode(nCode)
     , m_eType(FeatureParameterType::ENUM)
     , m_aEnumParameters(std::move(aEnumParameters))
 {