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

On Tue, 2013-01-08 at 14:15 +0100, Michael Stahl wrote:
> there may be more places where a similar approach could be applicable,
> both in API implementations in sw and in writerfilter/xmloff import
> code, e.g. see cc99bb9f383a65912d004e227a5b6a88b401bbba which was purely
> result of me debugging some crash and wondering why a certain insert
> method in sw core is called so often (in that case sw API impl. actually
> did the right thing).

        :-) as I say, it seems to me that being able to manipulate SfxItemSet's
from UNO (or better not using UNO at all to talk to the core from the
DomainMapper ;-), might allow the same item-set to be re-used 

> > 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.
...
> i suppose this exception would be thrown by
> SwUnoCursorHelper::SetPropertyValues now anyway... which is actually a
> problem because in that case
>   m_pImpl->m_pDoc->GetIDocumentUndoRedo().EndUndo(UNDO_INSERT, NULL);
> won't be executed and we get a confused UNDO stack in writer...

        Ah right - we need the try/catch goodness there still. I've added that
back.

> i guess it's best to move the Start/EndUndo into
> SwUnoCursorHelper::SetPropertyValues to fix that (can't be omitted
> entirely since it's possible that multiple different Undo objects are
> created).

        I've re-worked it as attached; I'd love to use the same code for
setting paragraph properties (which I suspect would give us another real
win there). Having said that switching the #if 1 to turn that on breaks
'make slowcheck' - though quite why is not immediately apparent to me
from code-reading ;-)

        Any help getting that finished off / merged v. much appreciated.

        Thanks !

                Michael.

-- 
michael.meeks@suse.com  <><, Pseudo Engineer, itinerant idiot

From e16eee537677587537b3884276a528e63a5a4c07 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.

Change-Id: I2582adee1bf35b07b90af810cb0d19dadc1d348f
---
 sw/inc/unocrsrhelper.hxx           |   13 +++++
 sw/source/core/unocore/unoobj.cxx  |   87 +++++++++++++++++++++++++++--------
 sw/source/core/unocore/unotext.cxx |   74 ++++++++++++++++---------------
 3 files changed, 118 insertions(+), 56 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 01318bb..f5736b8 100644
--- a/sw/source/core/unocore/unoobj.cxx
+++ b/sw/source/core/unocore/unoobj.cxx
@@ -1861,34 +1861,81 @@ 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)
+{
+    if (!rPropertyValues.getLength())
+        return;
+
     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 + "' ";
+            break;
+        }
+        else if (pEntry->nFlags & beans::PropertyAttribute::READONLY)
+        {
+            aPropertyVetoExMsg += "Property is read-only: '" + rPropertyName + "' ";
+            break;
+        } 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 < (sal_Int32)aEntries.size() ); ++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 d2e3add..8060a1b 100644
--- a/sw/source/core/unocore/unotext.cxx
+++ b/sw/source/core/unocore/unotext.cxx
@@ -17,6 +17,7 @@
  *   the License at http://www.apache.org/licenses/LICENSE-2.0 .
  */
 
+#include <stdio.h>
 
 #include <stdlib.h>
 
@@ -1309,6 +1310,7 @@ throw (lang::IllegalArgumentException, uno::RuntimeException)
     {
         aPam.Move( fnMoveBackward, fnGoNode );
     }
+#if 1
     if (rProperties.getLength())
     {
         // now set the properties
@@ -1345,6 +1347,26 @@ throw (lang::IllegalArgumentException, uno::RuntimeException)
             }
         }
     }
+#else
+    try
+    {
+        SfxItemPropertySet const*const pParaPropSet =
+            aSwMapProvider.GetPropertySet(PROPERTY_MAP_PARAGRAPH);
+        if (!bIllegalException)
+            SwUnoCursorHelper::SetPropertyValues(aPam, *pParaPropSet, rProperties);
+    }
+    catch (const lang::IllegalArgumentException& rIllegal)
+    {
+        sMessage = rIllegal.Message;
+        bIllegalException = true;
+    }
+    catch (const uno::RuntimeException& rRuntime)
+    {
+        sMessage = rRuntime.Message;
+        bRuntimeException = true;
+    }
+#endif
+
     m_pDoc->GetIDocumentUndoRedo().EndUndo(UNDO_END, NULL);
     if (bIllegalException || bRuntimeException)
     {
@@ -1415,43 +1437,23 @@ throw (lang::IllegalArgumentException, uno::RuntimeException)
         pCursor->GetPoint()->nContent = nContentPos;
     }
 
-    if (rCharacterAndParagraphProperties.getLength())
+    try
     {
-        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)
-            {
-                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);
+    }
+    catch (const lang::IllegalArgumentException& rIllegal)
+    {
+        sMessage = rIllegal.Message;
+        bIllegalException = true;
+    }
+    catch (const uno::RuntimeException& rRuntime)
+    {
+        sMessage = rRuntime.Message;
+        bRuntimeException = true;
     }
     m_pImpl->m_pDoc->GetIDocumentUndoRedo().EndUndo(UNDO_INSERT, NULL);
     if (bIllegalException || bRuntimeException)
-- 
1.7.7