[Libreoffice-commits] core.git: Branch 'feature/chart-opengl2' - 5 commits - include/svl sc/inc sc/source svl/source

Markus Mohrhard markus.mohrhard at googlemail.com
Sat Dec 28 08:21:45 PST 2013


 include/svl/broadcast.hxx       |   19 ++++++
 sc/inc/column.hxx               |    1 
 sc/source/core/data/column2.cxx |   15 ++++
 sc/source/core/data/table1.cxx  |    3 
 svl/source/notify/broadcast.cxx |  126 +++++++++++++++++++++++-----------------
 5 files changed, 113 insertions(+), 51 deletions(-)

New commits:
commit a0c3fe0c8c37fa5df3860afe1f8ef6f369ce4580
Author: Markus Mohrhard <markus.mohrhard at googlemail.com>
Date:   Mon Dec 23 23:46:02 2013 +0100

    fix crash when erasing entry while iterating through vector
    
    Broadcast might result in calling Remove on the same object which erases
    the entry from the listeners vector. If we create a copy we can still
    iterate through the vector as all iterators are still valid.

diff --git a/svl/source/notify/broadcast.cxx b/svl/source/notify/broadcast.cxx
index 425a1e2..19abe38 100644
--- a/svl/source/notify/broadcast.cxx
+++ b/svl/source/notify/broadcast.cxx
@@ -124,7 +124,8 @@ void SvtBroadcaster::Broadcast( const SfxHint &rHint )
     Normalize();
 
     ListenersType::iterator dest(maDestructedListeners.begin());
-    for (ListenersType::iterator it(maListeners.begin()); it != maListeners.end(); ++it)
+    ListenersType aListeners(maListeners); // this copy is important to avoid erasing entries while iterating
+    for (ListenersType::iterator it(aListeners.begin()); it != aListeners.end(); ++it)
     {
         // skip the destructed ones
         while (dest != maDestructedListeners.end() && (*dest < *it))
commit 0c80a98738abe0dfb6e8599c6ac23bb1310d03a3
Author: Eike Rathke <erack at redhat.com>
Date:   Mon Dec 23 13:04:15 2013 +0100

    iterators are not pointers
    
    Change-Id: Ic9809beead66cf0d0e6a6a28bb97b220fb667578

diff --git a/svl/source/notify/broadcast.cxx b/svl/source/notify/broadcast.cxx
index 8d3905a..425a1e2 100644
--- a/svl/source/notify/broadcast.cxx
+++ b/svl/source/notify/broadcast.cxx
@@ -79,7 +79,7 @@ SvtBroadcaster::SvtBroadcaster( const SvtBroadcaster &rBC ) :
         Normalize();
 
     ListenersType::iterator dest(maDestructedListeners.begin());
-    for (ListenersType::iterator it(maListeners.begin()); it < maListeners.end(); ++it)
+    for (ListenersType::iterator it(maListeners.begin()); it != maListeners.end(); ++it)
     {
         bool bStart = true;
 
@@ -108,7 +108,7 @@ SvtBroadcaster::~SvtBroadcaster()
     // listeners, with the exception of those that already asked to be removed
     // during their own destruction
     ListenersType::iterator dest(maDestructedListeners.begin());
-    for (ListenersType::iterator it(maListeners.begin()); it < maListeners.end(); ++it)
+    for (ListenersType::iterator it(maListeners.begin()); it != maListeners.end(); ++it)
     {
         // skip the destructed ones
         while (dest != maDestructedListeners.end() && (*dest < *it))
@@ -124,7 +124,7 @@ void SvtBroadcaster::Broadcast( const SfxHint &rHint )
     Normalize();
 
     ListenersType::iterator dest(maDestructedListeners.begin());
-    for (ListenersType::iterator it(maListeners.begin()); it < maListeners.end(); ++it)
+    for (ListenersType::iterator it(maListeners.begin()); it != maListeners.end(); ++it)
     {
         // skip the destructed ones
         while (dest != maDestructedListeners.end() && (*dest < *it))
commit c2530c5381805b6c3da6a2d58a77af2f9a797568
Author: Jan Holesovsky <kendy at collabora.com>
Date:   Sat Dec 21 20:51:29 2013 +0100

    Make the broadcasting work even when about to destruct.
    
    Change-Id: Idba9302e1ec5234d3d472cda047c09ba52afd328

diff --git a/include/svl/broadcast.hxx b/include/svl/broadcast.hxx
index 72ac303..a77ebdb 100644
--- a/include/svl/broadcast.hxx
+++ b/include/svl/broadcast.hxx
@@ -82,6 +82,7 @@ private:
     bool mbAboutToDie:1;
     bool mbDisposing:1;
     bool mbNormalized:1;
+    bool mbDestNormalized:1;
 };
 
 
diff --git a/svl/source/notify/broadcast.cxx b/svl/source/notify/broadcast.cxx
index 50d68a9..8d3905a 100644
--- a/svl/source/notify/broadcast.cxx
+++ b/svl/source/notify/broadcast.cxx
@@ -21,44 +21,23 @@
 #include <svl/listener.hxx>
 #include <svl/smplhint.hxx>
 
-namespace {
-
-class StartListeningHandler : std::unary_function<SvtListener*, void>
+void SvtBroadcaster::Normalize()
 {
-    SvtBroadcaster& mrBC;
-public:
-    StartListeningHandler( SvtBroadcaster& rBC ) : mrBC(rBC) {}
-
-    void operator() ( SvtListener* p )
+    if (!mbNormalized)
     {
-        p->StartListening(mrBC);
+        std::sort(maListeners.begin(), maListeners.end());
+        ListenersType::iterator itUniqueEnd = std::unique(maListeners.begin(), maListeners.end());
+        maListeners.erase(itUniqueEnd, maListeners.end());
+        mbNormalized = true;
     }
-};
-
-class NotifyHandler : std::unary_function<SvtListener*, void>
-{
-    SvtBroadcaster& mrBC;
-    const SfxHint& mrHint;
-public:
-    NotifyHandler( SvtBroadcaster& rBC, const SfxHint& rHint ) : mrBC(rBC), mrHint(rHint) {}
 
-    void operator() ( SvtListener* p )
+    if (!mbDestNormalized)
     {
-        p->Notify(mrBC, mrHint);
+        std::sort(maDestructedListeners.begin(), maDestructedListeners.end());
+        ListenersType::iterator itUniqueEnd = std::unique(maDestructedListeners.begin(), maDestructedListeners.end());
+        maDestructedListeners.erase(itUniqueEnd, maDestructedListeners.end());
+        mbDestNormalized = true;
     }
-};
-
-}
-
-void SvtBroadcaster::Normalize()
-{
-    if (mbNormalized)
-        return;
-
-    std::sort(maListeners.begin(), maListeners.end());
-    ListenersType::iterator itUniqueEnd = std::unique(maListeners.begin(), maListeners.end());
-    maListeners.erase(itUniqueEnd, maListeners.end());
-    mbNormalized = true;
 }
 
 void SvtBroadcaster::Add( SvtListener* p )
@@ -75,6 +54,7 @@ void SvtBroadcaster::Remove( SvtListener* p )
     if (mbAboutToDie)
     {
         maDestructedListeners.push_back(p);
+        mbDestNormalized = false;
         return;
     }
 
@@ -88,12 +68,33 @@ void SvtBroadcaster::Remove( SvtListener* p )
         ListenersGone();
 }
 
-SvtBroadcaster::SvtBroadcaster() : mbAboutToDie(false), mbDisposing(false), mbNormalized(false) {}
+SvtBroadcaster::SvtBroadcaster() : mbAboutToDie(false), mbDisposing(false), mbNormalized(false), mbDestNormalized(false) {}
 
 SvtBroadcaster::SvtBroadcaster( const SvtBroadcaster &rBC ) :
-    maListeners(rBC.maListeners), mbAboutToDie(false), mbDisposing(false), mbNormalized(rBC.mbNormalized)
+    maListeners(rBC.maListeners), maDestructedListeners(rBC.maDestructedListeners),
+    mbAboutToDie(rBC.mbAboutToDie), mbDisposing(false),
+    mbNormalized(rBC.mbNormalized), mbDestNormalized(rBC.mbDestNormalized)
 {
-    std::for_each(maListeners.begin(), maListeners.end(), StartListeningHandler(*this));
+    if (mbAboutToDie)
+        Normalize();
+
+    ListenersType::iterator dest(maDestructedListeners.begin());
+    for (ListenersType::iterator it(maListeners.begin()); it < maListeners.end(); ++it)
+    {
+        bool bStart = true;
+
+        if (mbAboutToDie)
+        {
+            // skip the destructed ones
+            while (dest != maDestructedListeners.end() && (*dest < *it))
+                ++dest;
+
+            bStart = (dest == maDestructedListeners.end() || *dest != *it);
+        }
+
+        if (bStart)
+            (*it)->StartListening(*this);
+    }
 }
 
 SvtBroadcaster::~SvtBroadcaster()
@@ -101,12 +102,6 @@ SvtBroadcaster::~SvtBroadcaster()
     mbDisposing = true;
     Broadcast( SfxSimpleHint(SFX_HINT_DYING) );
 
-    // normalize the list of listeners than already asked for destruction
-    std::sort(maDestructedListeners.begin(), maDestructedListeners.end());
-    ListenersType::iterator itUniqueEnd = std::unique(maDestructedListeners.begin(), maDestructedListeners.end());
-    maDestructedListeners.erase(itUniqueEnd, maDestructedListeners.end());
-
-    // and the list of registered listeners too
     Normalize();
 
     // now when both lists are sorted, we can linearly unregister all
@@ -126,12 +121,18 @@ SvtBroadcaster::~SvtBroadcaster()
 
 void SvtBroadcaster::Broadcast( const SfxHint &rHint )
 {
-    if (mbAboutToDie)
-        return;
-
     Normalize();
-    ListenersType listeners(maListeners);
-    std::for_each(listeners.begin(), listeners.end(), NotifyHandler(*this, rHint));
+
+    ListenersType::iterator dest(maDestructedListeners.begin());
+    for (ListenersType::iterator it(maListeners.begin()); it < maListeners.end(); ++it)
+    {
+        // skip the destructed ones
+        while (dest != maDestructedListeners.end() && (*dest < *it))
+            ++dest;
+
+        if (dest == maDestructedListeners.end() || *dest != *it)
+            (*it)->Notify(*this, rHint);
+    }
 }
 
 void SvtBroadcaster::ListenersGone() {}
commit dd853ba910ee38ba3c68d2d58967f18ed809ace8
Author: Jan Holesovsky <kendy at collabora.com>
Date:   Sat Dec 21 17:26:01 2013 +0100

    Don't call EndListening() on already destructed listeners.
    
    Change-Id: I9bda35f2246de9d37077dda33c710b89ee008e5a

diff --git a/include/svl/broadcast.hxx b/include/svl/broadcast.hxx
index d122e02..72ac303 100644
--- a/include/svl/broadcast.hxx
+++ b/include/svl/broadcast.hxx
@@ -70,10 +70,15 @@ public:
      * themselves from the broadcaster - the broadcaster will not broadcast
      * anything after the PrepareForDesctruction() call anyway.
      */
-    void PrepareForDestruction() { mbAboutToDie = true; }
+    void PrepareForDestruction();
 
 private:
     ListenersType maListeners;
+
+    /// When the broadcaster is about to die, collect listeners that asked for removal.
+    ListenersType maDestructedListeners;
+
+    /// Indicate that this broadcaster will be destructed (we indicate this on all ScColumn's broadcasters during the ScTable destruction, eg.)
     bool mbAboutToDie:1;
     bool mbDisposing:1;
     bool mbNormalized:1;
diff --git a/svl/source/notify/broadcast.cxx b/svl/source/notify/broadcast.cxx
index 3575c7f..50d68a9 100644
--- a/svl/source/notify/broadcast.cxx
+++ b/svl/source/notify/broadcast.cxx
@@ -35,18 +35,6 @@ public:
     }
 };
 
-class EndListeningHandler : std::unary_function<SvtListener*, void>
-{
-    SvtBroadcaster& mrBC;
-public:
-    EndListeningHandler( SvtBroadcaster& rBC ) : mrBC(rBC) {}
-
-    void operator() ( SvtListener* p )
-    {
-        p->EndListening(mrBC);
-    }
-};
-
 class NotifyHandler : std::unary_function<SvtListener*, void>
 {
     SvtBroadcaster& mrBC;
@@ -81,9 +69,15 @@ void SvtBroadcaster::Add( SvtListener* p )
 
 void SvtBroadcaster::Remove( SvtListener* p )
 {
-    if (mbAboutToDie || mbDisposing)
+    if (mbDisposing)
         return;
 
+    if (mbAboutToDie)
+    {
+        maDestructedListeners.push_back(p);
+        return;
+    }
+
     Normalize();
     std::pair<ListenersType::iterator,ListenersType::iterator> r =
         std::equal_range(maListeners.begin(), maListeners.end(), p);
@@ -107,8 +101,27 @@ SvtBroadcaster::~SvtBroadcaster()
     mbDisposing = true;
     Broadcast( SfxSimpleHint(SFX_HINT_DYING) );
 
-    // unregister all listeners.
-    std::for_each(maListeners.begin(), maListeners.end(), EndListeningHandler(*this));
+    // normalize the list of listeners than already asked for destruction
+    std::sort(maDestructedListeners.begin(), maDestructedListeners.end());
+    ListenersType::iterator itUniqueEnd = std::unique(maDestructedListeners.begin(), maDestructedListeners.end());
+    maDestructedListeners.erase(itUniqueEnd, maDestructedListeners.end());
+
+    // and the list of registered listeners too
+    Normalize();
+
+    // now when both lists are sorted, we can linearly unregister all
+    // listeners, with the exception of those that already asked to be removed
+    // during their own destruction
+    ListenersType::iterator dest(maDestructedListeners.begin());
+    for (ListenersType::iterator it(maListeners.begin()); it < maListeners.end(); ++it)
+    {
+        // skip the destructed ones
+        while (dest != maDestructedListeners.end() && (*dest < *it))
+            ++dest;
+
+        if (dest == maDestructedListeners.end() || *dest != *it)
+            (*it)->EndListening(*this);
+    }
 }
 
 void SvtBroadcaster::Broadcast( const SfxHint &rHint )
@@ -133,4 +146,10 @@ bool SvtBroadcaster::HasListeners() const
     return !maListeners.empty();
 }
 
+void SvtBroadcaster::PrepareForDestruction()
+{
+    mbAboutToDie = true;
+    maDestructedListeners.reserve(maListeners.size());
+}
+
 /* vim:set shiftwidth=4 softtabstop=4 expandtab: */
commit 5d96a8af9f1c90d7a9a5ff29a8294bc185e2aa9c
Author: Jan Holesovsky <kendy at collabora.com>
Date:   Sat Dec 21 01:41:18 2013 +0100

    Speedup destruction of sheets with too many listeners & broadcasters.
    
    Listeners and broadcasters are M:N relationship.  If you want to
    destruct them, you easily end up in O(M*N) situation; where for every
    listener, you iterate all broadcasters, to remove that one listener.
    
    To avoid that, announce to the broadcasters that they are going to die, and the
    listeners do not have to bother with removing themselves from the broadcaster.
    The broadcaster will not broadcast anything after the PrepareForDesctruction()
    call anyway.
    
    Change-Id: I68d78b23e73bcbb944de9139448b2c20dfa14f62

diff --git a/include/svl/broadcast.hxx b/include/svl/broadcast.hxx
index 6a6bc03..d122e02 100644
--- a/include/svl/broadcast.hxx
+++ b/include/svl/broadcast.hxx
@@ -60,8 +60,21 @@ public:
 
     bool HasListeners() const;
 
+    /**
+     * Listeners and broadcasters are M:N relationship.  If you want to
+     * destruct them, you easily end up in O(M*N) situation; where for every
+     * listener, you iterate all broadcasters, to remove that one listener.
+     *
+     * To avoid that, use this call to announce to the broadcaster it is going
+     * to die, and the listeners do not have to bother with removing
+     * themselves from the broadcaster - the broadcaster will not broadcast
+     * anything after the PrepareForDesctruction() call anyway.
+     */
+    void PrepareForDestruction() { mbAboutToDie = true; }
+
 private:
     ListenersType maListeners;
+    bool mbAboutToDie:1;
     bool mbDisposing:1;
     bool mbNormalized:1;
 };
diff --git a/sc/inc/column.hxx b/sc/inc/column.hxx
index 55b3847..b7f48db 100644
--- a/sc/inc/column.hxx
+++ b/sc/inc/column.hxx
@@ -491,6 +491,7 @@ public:
     SvtBroadcaster* GetBroadcaster( SCROW nRow );
     const SvtBroadcaster* GetBroadcaster( SCROW nRow ) const;
     void DeleteBroadcasters( sc::ColumnBlockPosition& rBlockPos, SCROW nRow1, SCROW nRow2 );
+    void PrepareBroadcastersForDestruction();
     bool HasBroadcaster() const;
 
     void Broadcast( SCROW nRow );
diff --git a/sc/source/core/data/column2.cxx b/sc/source/core/data/column2.cxx
index 307d42b..2092406b 100644
--- a/sc/source/core/data/column2.cxx
+++ b/sc/source/core/data/column2.cxx
@@ -1962,6 +1962,21 @@ void ScColumn::DeleteBroadcasters( sc::ColumnBlockPosition& rBlockPos, SCROW nRo
         maBroadcasters.set_empty(rBlockPos.miBroadcasterPos, nRow1, nRow2);
 }
 
+void ScColumn::PrepareBroadcastersForDestruction()
+{
+    sc::BroadcasterStoreType::iterator itPos = maBroadcasters.begin(), itPosEnd = maBroadcasters.end();
+    for (; itPos != itPosEnd; ++itPos)
+    {
+        if (itPos->type == sc::element_type_broadcaster)
+        {
+            sc::broadcaster_block::iterator it = sc::broadcaster_block::begin(*itPos->data);
+            sc::broadcaster_block::iterator itEnd = sc::broadcaster_block::end(*itPos->data);
+            for (; it != itEnd; ++it)
+                (*it)->PrepareForDestruction();
+        }
+    }
+}
+
 bool ScColumn::HasBroadcaster() const
 {
     sc::BroadcasterStoreType::const_iterator it = maBroadcasters.begin(), itEnd = maBroadcasters.end();
diff --git a/sc/source/core/data/table1.cxx b/sc/source/core/data/table1.cxx
index 4eed3b0..37417ac 100644
--- a/sc/source/core/data/table1.cxx
+++ b/sc/source/core/data/table1.cxx
@@ -341,6 +341,9 @@ ScTable::~ScTable()
     delete mpRangeName;
     delete pDBDataNoName;
     DestroySortCollator();
+
+    for (SCCOL k=0; k<=MAXCOL; k++)
+        aCol[k].PrepareBroadcastersForDestruction();
 }
 
 void ScTable::GetName( OUString& rName ) const
diff --git a/svl/source/notify/broadcast.cxx b/svl/source/notify/broadcast.cxx
index 24b5790..3575c7f 100644
--- a/svl/source/notify/broadcast.cxx
+++ b/svl/source/notify/broadcast.cxx
@@ -81,7 +81,7 @@ void SvtBroadcaster::Add( SvtListener* p )
 
 void SvtBroadcaster::Remove( SvtListener* p )
 {
-    if (mbDisposing)
+    if (mbAboutToDie || mbDisposing)
         return;
 
     Normalize();
@@ -94,10 +94,10 @@ void SvtBroadcaster::Remove( SvtListener* p )
         ListenersGone();
 }
 
-SvtBroadcaster::SvtBroadcaster() : mbDisposing(false), mbNormalized(false) {}
+SvtBroadcaster::SvtBroadcaster() : mbAboutToDie(false), mbDisposing(false), mbNormalized(false) {}
 
 SvtBroadcaster::SvtBroadcaster( const SvtBroadcaster &rBC ) :
-    maListeners(rBC.maListeners), mbDisposing(false), mbNormalized(rBC.mbNormalized)
+    maListeners(rBC.maListeners), mbAboutToDie(false), mbDisposing(false), mbNormalized(rBC.mbNormalized)
 {
     std::for_each(maListeners.begin(), maListeners.end(), StartListeningHandler(*this));
 }
@@ -113,6 +113,9 @@ SvtBroadcaster::~SvtBroadcaster()
 
 void SvtBroadcaster::Broadcast( const SfxHint &rHint )
 {
+    if (mbAboutToDie)
+        return;
+
     Normalize();
     ListenersType listeners(maListeners);
     std::for_each(listeners.begin(), listeners.end(), NotifyHandler(*this, rHint));


More information about the Libreoffice-commits mailing list