[REVIEW] domain-mapper import speedup patch ...

Hi guys,

        Still annoyed by the (increasingly small) RTF performance
regressions[1], and given that the profiling now shows the
domain-mapper's use/abuse of the core as being the main factor (at least
for file 3 in [1]).

        I attach a prototype patch. It passes make check and make slowcheck in
sw/ (not tried subsequentcheck).

        There are probably better ways to do this :-) clearly it would be nice
to manage layered SfxItemSets via UNO in an even more performant and
elegant way that doesn't rely on great big uno PropertyValue sequences
getting sent repeatedly and so on. Anyhow the attached gives a
reasonable win on RTF import: 25% faster or so. It's hard to see how it
could make things much perform worse. It has a FIXME - I'm unclear
exactly what's going on there. Clearly I'm wandering at the edge of my
competence wrt. the writer bits here, so help/encouragement appreciated.

        Thoughts / review etc. much appreciated; if no-one screams I'm inclined
to remove the CursorMap property checks I don't like and push it to
master (which is a threat to focus review incidentally ;-).

        Thanks !

                Michael.

[1] - https://bugs.freedesktop.org/show_bug.cgi?id=44736
-- 
michael.meeks@suse.com  <><, Pseudo Engineer, itinerant idiot

From 4cd1269bf5d2328930a821f36adcf522010becf4 Mon Sep 17 00:00:00 2001
From: Michael Meeks <michael.meeks@suse.com>
Date: Fri, 21 Dec 2012 15:27:27 +0000
Subject: [PATCH] fdo#44736 - set and fetch multiple properties concurrently

The domain-mapper calls SwXText::insertTextPortion very extensively
accounting for about half of import time for large, lightly formatted
text documents. The vast majority of the work is consumed managing
char + para properties - so try to batch that, making it 70% faster
for my lightly formatted test. Saves around 25% of load time for me.
---
 sw/inc/unocrsrhelper.hxx           |   13 ++++++
 sw/source/core/unocore/unoobj.cxx  |   80 +++++++++++++++++++++++++++---------
 sw/source/core/unocore/unotext.cxx |   33 ++++-----------
 3 files changed, 82 insertions(+), 44 deletions(-)

diff --git a/sw/inc/unocrsrhelper.hxx b/sw/inc/unocrsrhelper.hxx
index a5892f6..aa7fd4d 100644
--- a/sw/inc/unocrsrhelper.hxx
+++ b/sw/inc/unocrsrhelper.hxx
@@ -151,6 +151,19 @@ namespace SwUnoCursorHelper
                 ::com::sun::star::lang::IllegalArgumentException,
                 ::com::sun::star::lang::WrappedTargetException,
                 ::com::sun::star::uno::RuntimeException);
+    /// @param bTableMode: attributes should be applied to a table selection
+    void SetPropertyValues(
+            SwPaM& rPaM,
+            const SfxItemPropertySet & rPropSet,
+            const ::com::sun::star::uno::Sequence< ::com::sun::star::beans::PropertyValue > &
+            rPropertyValues,
+            const SetAttrMode nAttrMode = nsSetAttrMode::SETATTR_DEFAULT,
+            const bool bTableMode = false)
+        throw (::com::sun::star::beans::UnknownPropertyException,
+                ::com::sun::star::beans::PropertyVetoException,
+                ::com::sun::star::lang::IllegalArgumentException,
+                ::com::sun::star::lang::WrappedTargetException,
+                ::com::sun::star::uno::RuntimeException);
     ::com::sun::star::uno::Any  GetPropertyValue(
             SwPaM& rPaM,
             const SfxItemPropertySet & rPropSet,
diff --git a/sw/source/core/unocore/unoobj.cxx b/sw/source/core/unocore/unoobj.cxx
index 371e8fb..213a365 100644
--- a/sw/source/core/unocore/unoobj.cxx
+++ b/sw/source/core/unocore/unoobj.cxx
@@ -1861,34 +1861,74 @@ throw (beans::UnknownPropertyException, beans::PropertyVetoException,
         lang::IllegalArgumentException, lang::WrappedTargetException,
         uno::RuntimeException)
 {
+    uno::Sequence< beans::PropertyValue > aValues(1);
+    aValues[0].Name = rPropertyName;
+    aValues[0].Value = rValue;
+    SetPropertyValues(rPaM, rPropSet, aValues, nAttrMode, bTableMode);
+}
+
+void SwUnoCursorHelper::SetPropertyValues(
+    SwPaM& rPaM, const SfxItemPropertySet& rPropSet,
+    const uno::Sequence< beans::PropertyValue > &rPropertyValues,
+    const SetAttrMode nAttrMode, const bool bTableMode)
+throw (beans::UnknownPropertyException, beans::PropertyVetoException,
+        lang::IllegalArgumentException, lang::WrappedTargetException,
+        uno::RuntimeException)
+{
     SwDoc *const pDoc = rPaM.GetDoc();
-    SfxItemPropertySimpleEntry const*const pEntry =
-        rPropSet.getPropertyMap().getByName(rPropertyName);
-    if (!pEntry)
+    rtl::OUString aUnknownExMsg, aPropertyVetoExMsg;
+
+    // Build set of attributes we want to fetch
+    std::vector<sal_uInt16> aWhichPairs;
+    std::vector<SfxItemPropertySimpleEntry const*> aEntries;
+    aEntries.reserve(rPropertyValues.getLength());
+    for (sal_Int32 i = 0; i < rPropertyValues.getLength(); ++i)
     {
-        throw beans::UnknownPropertyException(
-            OUString(RTL_CONSTASCII_USTRINGPARAM("Unknown property: "))
-                + rPropertyName,
-            static_cast<cppu::OWeakObject *>(0));
+        const rtl::OUString &rPropertyName = rPropertyValues[i].Name;
+
+        SfxItemPropertySimpleEntry const* pEntry =
+            rPropSet.getPropertyMap().getByName(rPropertyName);
+
+        // Queue up any exceptions until the end ...
+        if (!pEntry)
+            aUnknownExMsg += "Unknown property: '" + rPropertyName + "' ";
+        else if (pEntry->nFlags & beans::PropertyAttribute::READONLY)
+        {
+            aPropertyVetoExMsg += "Property is read-only: '" + rPropertyName + "' ";
+            pEntry = NULL;
+        } else {
+// FIXME: we should have some nice way of merging ranges surely ?
+            aWhichPairs.push_back(pEntry->nWID);
+            aWhichPairs.push_back(pEntry->nWID);
+        }
+        aEntries.push_back(pEntry);
     }
 
-    if (pEntry->nFlags & beans::PropertyAttribute::READONLY)
+    if (!aWhichPairs.empty())
     {
-        throw beans::PropertyVetoException(
-            OUString(RTL_CONSTASCII_USTRINGPARAM("Property is read-only: "))
-                + rPropertyName,
-            static_cast<cppu::OWeakObject *>(0));
-    }
+        aWhichPairs.push_back(0); // terminate
+        SfxItemSet aItemSet(pDoc->GetAttrPool(), &aWhichPairs[0]);
 
-    SfxItemSet aItemSet( pDoc->GetAttrPool(), pEntry->nWID, pEntry->nWID );
-    SwUnoCursorHelper::GetCrsrAttr( rPaM, aItemSet );
+        // Fetch, overwrite, and re-set the attributes from the core
+        SwUnoCursorHelper::GetCrsrAttr( rPaM, aItemSet );
 
-    if (!SwUnoCursorHelper::SetCursorPropertyValue(
-                *pEntry, rValue, rPaM, aItemSet))
-    {
-        rPropSet.setPropertyValue(*pEntry, rValue, aItemSet );
+        for (sal_Int32 i = 0; i < rPropertyValues.getLength(); ++i)
+        {
+            const uno::Any &rValue = rPropertyValues[i].Value;
+            SfxItemPropertySimpleEntry const*const pEntry = aEntries[i];
+            if (!pEntry)
+                continue;
+            if (!SwUnoCursorHelper::SetCursorPropertyValue(*pEntry, rValue, rPaM, aItemSet))
+                rPropSet.setPropertyValue(*pEntry, rValue, aItemSet);
+        }
+
+        SwUnoCursorHelper::SetCrsrAttr(rPaM, aItemSet, nAttrMode, bTableMode);
     }
-    SwUnoCursorHelper::SetCrsrAttr(rPaM, aItemSet, nAttrMode, bTableMode);
+
+    if (!aUnknownExMsg.isEmpty())
+        throw beans::UnknownPropertyException(aUnknownExMsg, static_cast<cppu::OWeakObject *>(0));
+    if (!aPropertyVetoExMsg.isEmpty())
+        throw beans::PropertyVetoException(aPropertyVetoExMsg, static_cast<cppu::OWeakObject 
*>(0));
 }
 
 uno::Sequence< beans::PropertyState >
diff --git a/sw/source/core/unocore/unotext.cxx b/sw/source/core/unocore/unotext.cxx
index 0515b3c..a5a1fc7 100644
--- a/sw/source/core/unocore/unotext.cxx
+++ b/sw/source/core/unocore/unotext.cxx
@@ -1,3 +1,4 @@
+#include <stdio.h>
 /* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*- */
 /*
  * This file is part of the LibreOffice project.
@@ -1421,38 +1422,22 @@ throw (lang::IllegalArgumentException, uno::RuntimeException)
         const SfxItemPropertyMap &rCursorMap =
             aSwMapProvider.GetPropertySet(PROPERTY_MAP_TEXT_CURSOR)
                 ->getPropertyMap();
-        beans::PropertyValue const*const pValues =
-            rCharacterAndParagraphProperties.getConstArray();
-        SfxItemPropertySet const*const pCursorPropSet =
-            aSwMapProvider.GetPropertySet(PROPERTY_MAP_TEXT_CURSOR);
         const sal_Int32 nLen(rCharacterAndParagraphProperties.getLength());
         for (sal_Int32 nProp = 0; nProp < nLen; ++nProp)
         {
-            if (!rCursorMap.getByName( pValues[nProp].Name ))
-            {
-                bIllegalException = true;
-                break;
-            }
-            try
-            {
-                SwUnoCursorHelper::SetPropertyValue(
-                    *pCursor, *pCursorPropSet,
-                    pValues[nProp].Name, pValues[nProp].Value,
-                    nsSetAttrMode::SETATTR_NOFORMATATTR);
-            }
-            catch (const lang::IllegalArgumentException& rIllegal)
+            // FIXME: unclear that we need to do this really ...
+            if (!rCursorMap.getByName( rCharacterAndParagraphProperties[nProp].Name ))
             {
-                sMessage = rIllegal.Message;
                 bIllegalException = true;
                 break;
             }
-            catch (const uno::RuntimeException& rRuntime)
-            {
-                sMessage = rRuntime.Message;
-                bRuntimeException = true;
-                break;
-            }
         }
+
+        SfxItemPropertySet const*const pCursorPropSet =
+            aSwMapProvider.GetPropertySet(PROPERTY_MAP_TEXT_CURSOR);
+        SwUnoCursorHelper::SetPropertyValues(*pCursor, *pCursorPropSet,
+                                             rCharacterAndParagraphProperties,
+                                             nsSetAttrMode::SETATTR_NOFORMATATTR);
     }
     m_pImpl->m_pDoc->GetIDocumentUndoRedo().EndUndo(UNDO_INSERT, NULL);
     if (bIllegalException || bRuntimeException)
-- 
1.7.10.4