Fix performance regression with ScPatternAttr/SC

Due to the paradigm item change the test
make CppunitTest_sc_tablesheetobj
with CPPUNIT_TEST_NAME
sc_apitest::ScTableSheetObj::testSheetCellRangeProperties
got much slower. Unfortunately it did not break, so got unnoted.

I took a look now. First I intended to add some hashing in an
std::unordered_set using that hash values at ScPatternAttr, but
that is not even possible due to other data in that item that needs
to be compared. I had the impression that it was 'somehow' hashed
before, but after debugging the version before that change I
noted that also the list of existing items was linearly compared
to the new entry, using the operator==.
Thus the problem was not due to not hashing, but due to the
ScPatternAttr::operator==. That uses the hash (not changed),
but no longer finds equal entries.
This is because the hash code is made up from the SfxPoolItemPtrs
in the SfxItemSet, so when all are equal we can be sure the SfxItemSet
content is equal.
To use this the other way around is *not* correct: Even with
not all ptrs equal the SfxItemSets can still be equal, simply
by one SfxItemSet using another, but identical incarnation of
an item. Thuis means that ScPatternAttr::operator== does not
detect all cases of equality.
This worked in most cases before since most items were
'pooled' and thus much effort was used to ensure their uniqueness,
but even before the paradigm item change an item type could be
flagged as non-poolable. In that case, this already could fail
but with no too bad consequences (just one more copy of
an ScPatternAttr would stay).
So I fixed that mainly in adapting and optimizing
ScPatternAttr::operator==. The results are (same machine, same
compiler, dbg version, metioned test):

Version before item paradigm change:
user    0m50,778s

Version after item paradigm change:
user    20m13,944s

Version with memcmp:
user    0m48,845s

Version with hash:
user    0m48,710s

Since that hash does nothing else than to buffer the comparison of
those item pointers I just tried to use memcmp instead, as is already
used in other places (same file, ScPatternAttr::FastEqualPatternSets,
also SfxItemSet::operator==). As can be seen above it makes practically
no difference (memcomp even slightly faster).
Since that hash is only used in ScPatternAttr::operator== and is same
speed (memcomp linearly compares 56 SfxPoolItem ptrs) I decided to
remove it. It needs quite some spaces to be reset and re-calculated
which are not needed anymore. The calculation is based on dividing
the number of items by 4, so we are good with 56, but if someone has/
will adapt the items used by ScPatternAttr it is easy to forget to
adapt this, and not easy to change the alghorithm  when it's not a
multiple of 4.

I also optimized/overhauled SfxItemSet::operator== (or better: the
SfxItemSet::Equals used by it). It is now better readable, too.

I also adapted ScAttrArray::AddCondFormat to not always incarnate/
delete ScPatternAttr instances, only when needed. This also helps
a bit and could be done in more places.

All in all it is really necessary to cleanup SC's usage of
ScPatternAttr - there are quite some possibilities to make that
cleaner and faster. In principle it's a huge 'compromize' to use
item functionailty to have a possibly 'cheap' maximum shared
SfxItemSet at a Cell.

Decided to make SfxItemSet::operator== even faster for the case
of unequal ranges by iterating over ranges of local SfxItemSet
and incremented offset. Still accesses to 2nd SfxItemSet will be
the expensive ones using the iterated WhichID.

Added two more things to SfxItemSet::operator==: We can return
early when both have no items set. For the unequal-ranges compare
I added an early-exit when Count() items were compared.

Looked at the errors that indeed do trigger the assert in
ScPatternAttr::operator== and hint to incarnations of ScPatternAttr
that do not use the needed range ATTR_PATTERN_START, ATTR_PATTERN_END.
Hunted down to come from ScViewFunc::ApplyAttributes and there from
some Dialogs, so seems some SC dialogs do not work with the correct
range schema for that item.
I tried code in ScViewFunc::ApplyAttributes to fix it, that works. I
also tried to hunt that down to the Dialogs that use the wrong schema
(TotalCount @SfxItemSet is 61 BTW). While it would be possible to do
so, it's no guarentee to have this fixed.
Thus I looked at ScPatternAttr::ScPatternAttr and added correciton
code when one with the wrong range schema gets created, this is
luckily not often needed and transfers the existing items with low
costs.
Maybe we should add a warning there if used, so at least new
implementations of stuff or old ones (the Dialogs) can be corrected?

Change-Id: I31da73ae30786bd6c5a08a5e3b7df8fe279af261
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/159592
Tested-by: Jenkins
Reviewed-by: Noel Grandin <noel.grandin@collabora.co.uk>
Reviewed-by: Armin Le Grand <Armin.Le.Grand@me.com>

0 files changed, 0 insertions, 0 deletions