Hi Noel,
Thanks for your work on this - pushing the thread to the public list
which is perhaps a better place for it.
I think my interest here is not per-se about broadcaster vs. listener -
but mostly around the complexity of setting up and/or removing listeners
from a broadcaster as/when the sheet is manipulated - creating and
destroying columns eg. which reference another cell - there can be some
performance explosions shuffling things out of std::vectors.
Kohei poked at a chunk of profiles like this in the past; probably
worth getting his input; any thoughts ?
Thanks,
Michael.
On 29/09/18 10:22, Michael Meeks wrote:
> Hi Noel,
>
> On 28/09/18 16:08, Noel Grandin wrote:
>> On 2018/09/27 5:06 PM, Noel Grandin wrote:
>>> I had a quick look at ScFormulaCell with gdb's pahole script, and it
>>> strikes me that
>>>
>>> (a) the biggest chunk is the SvtListener base class (64 bytes!)
>>
>> the attached patch might help
>
> Interesting. I wonder what the pathological usage patterns of this are.
> My hope is that (these days) if we have a 1m long formula-group that has
> some $a$1 term in it - that we create a single group dependency /
> listener for the whole formula-group - but, of course quite possibly we
> add ~1 million single-cell reference dependency. At which point using
> the set makes sense as that is manipulated.
>
> Of course - ultimately, it is just bad design to use a bigger structure
> when we don't need to. My hope is that as we re-work the dependency
> engine to use FormulaGroups - that we end up in a situation that the
> vast majority of cells simply have no content in their SvtListener and
> we can replace it with single pointer to a heap allocated listener
> instead that is always NULL; or - even move to looking those up in a
> smallish hash of single-cell dependencies.
>
> Anyhow - worth checking the corner-cases there, to see if our
> dependency work has simplified and evaporated out lots of single-cell
> dependencies to the point that the vector might work without exploding
> with some 1m^2 insert/delete behavior =)
>
> Also - we should prolly discuss this on the public dev list; any
> objections ? =)
--
michael.meeks@xxxxxxxxxxxxx <><, GM Collabora Productivity
Hangout: mejmeeks@xxxxxxxxx, Skype: mmeeks
(M) +44 7795 666 147 - timezone usually UK / Europe
>From 5e0f91830d08aaf641cb3f993cd5cc753b149076 Mon Sep 17 00:00:00 2001
From: Noel Grandin <noel.grandin@xxxxxxxxxxxxxxx>
Date: Fri, 28 Sep 2018 17:07:10 +0200
Subject: [PATCH] WIP use std::vector in SvtListener
because
- we mostly listen to zero or one broadcasters
- this reduces the size of ScFormulaCell from 160 bytes to 128 bytes
- has mostly positive (but small) effects on make sc.perfcheck
Change-Id: I0095b646177bdd23c64462afbb924943d598e6bc
---
include/svl/listener.hxx | 4 ++--
svl/source/notify/listener.cxx | 21 +++++++++++----------
2 files changed, 13 insertions(+), 12 deletions(-)
diff --git a/include/svl/listener.hxx b/include/svl/listener.hxx
index 5f2850ef7f19..649463bb0a83 100644
--- a/include/svl/listener.hxx
+++ b/include/svl/listener.hxx
@@ -21,14 +21,14 @@
#include <svl/svldllapi.h>
-#include <unordered_set>
+#include <vector>
class SvtBroadcaster;
class SfxHint;
class SVL_DLLPUBLIC SvtListener
{
- typedef std::unordered_set<SvtBroadcaster*> BroadcastersType;
+ typedef std::vector<SvtBroadcaster*> BroadcastersType;
BroadcastersType maBroadcasters;
const SvtListener& operator=(const SvtListener &) = delete;
diff --git a/svl/source/notify/listener.cxx b/svl/source/notify/listener.cxx
index 89306a534a0d..4736a5c205ff 100644
--- a/svl/source/notify/listener.cxx
+++ b/svl/source/notify/listener.cxx
@@ -43,20 +43,21 @@ SvtListener::~SvtListener() COVERITY_NOEXCEPT_FALSE
bool SvtListener::StartListening( SvtBroadcaster& rBroadcaster )
{
- std::pair<BroadcastersType::iterator, bool> r =
- maBroadcasters.insert(&rBroadcaster);
- if (r.second)
- {
- // This is a new broadcaster.
- rBroadcaster.Add(this);
- }
- return r.second;
+ BroadcastersType::iterator it = std::lower_bound(maBroadcasters.begin(), maBroadcasters.end(), &rBroadcaster);
+ if (it != maBroadcasters.end() && *it == &rBroadcaster)
+ // already listening to this broadcaster.
+ return false;
+
+ // This is a new broadcaster.
+ maBroadcasters.insert(it, &rBroadcaster); // maintain sorting
+ rBroadcaster.Add(this);
+ return true;
}
bool SvtListener::EndListening( SvtBroadcaster& rBroadcaster )
{
- BroadcastersType::iterator it = maBroadcasters.find(&rBroadcaster);
- if (it == maBroadcasters.end())
+ BroadcastersType::iterator it = std::lower_bound(maBroadcasters.begin(), maBroadcasters.end(), &rBroadcaster);
+ if (it == maBroadcasters.end() || *it != &rBroadcaster)
// Not listening to this broadcaster.
return false;
--
2.17.1
_______________________________________________
LibreOffice mailing list
LibreOffice@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/libreoffice
