| Age | Commit message (Collapse) | Author |
|---|
|
We are doing a lot of work in ScAttrArray::SetPatternAreaImpl,
where we move the mvData vector of CellAttributeHolder around,
so inline and reduce some of the work.
The move operator= can be simplified, leaving some of the work
to the destructor (which may well be elided when moving
arrays of elements around).
Reduces time from 50s to 39s
Change-Id: I358f48fa30f785f7b3f221079db93ab0288a7f14
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/169941
Tested-by: Jenkins
Reviewed-by: Noel Grandin <noel.grandin@collabora.co.uk>
|
|
With all the changes done for Items we can now do deeper
basic changes to the ItemSet itself with manageable risk.
I already did https://gerrit.libreoffice.org/c/core/+/166455
aka 'ITEM: Add measurements for SfxItemSet usages' to
get some statistical information about the fill/usage grade
of the ItemSet's fixed PtrArray to SfxPoolItems, check
that out to get an own picture.
Those results show that an average usage is between some
extremes ranging from 0% to 50%, but when using more checks
and using multiple files/interactions/edits in all
applications we end up with around typical 12%-19% of that
array used, the rest is nullptr's.
Thus I thought about one of the initial ideas of this
series of changes (ITEM), to use a std::unordered_map
(A) instead of that fixed array of SfxPoolItem Ptr's (B).
Tthat again was for a complete type-based rewrite, which
I once did a POC, but the code cannot be adapted to that,
just too much work.
Those are very different in architecture, (B) is done
since a long time (since ever), but as pointed out above,
(A) is now possible. There are many aspects to it, let's
grep some:
Speed (iterate): (A) and (B) are both linear. (A) has
less entries, but may touch different mem areas
(buckets). (B) is linear, but many empty spaces which
are usually uselessly iterated.
Speed (access Item by WhichID): (A) is hashed by
WhichID, so mostly linear for unordered_set/hash.
(B) is in a linear array, but has to calculate the
offset for each WhichID access.
So I guess speed will mostly equal out.
Memory: (A) will be dynamically allocated (buckets),
but stl is highly optimized and may even re-use areas,
has to provide some extra info but will need less
places for Items since it's dynamic and can start empty.
(B) will be allocated once (except AllItemSet) and may
even be 'derived' to the ItemSet (SfxItemSetFixed), but
has to allocate all space at once.
I can go in lots of more detail here, but due to the
results of the statistics I just made a test now,
including measuring some results (will include in
gerrit, not here). I used two pro versions for that.
That way I have some data now.
Result is:
- It is now possible to do this, it runs stable :-)
- Speed: As expected, mostly no change
- Memory: Depending on usage, 0% to 25% less,
usually around 8-10%
Side effects:
- SfxAllItemSet could be done without WhichRanges,
thus without expensive 'merges'
- SfxItemSetFixed is not needed. While the idea is
good, it needs a lot of extra stuff
- Access to Items is linear if set
- WhichRanges: Still needed, but for vaildity
checking/filtering of ItemSet content
- WhichRanges: Worth to think about if these are
needed at all, probably just exist for historical
reasons since allocation/number of added Items
was never ever dynamic -> just not allocatable
Putting the current version on gerrit, may still
trigger some UTs (checked SW/SC/SD...)
I did not like that functionality at ItemSet that
hands out a vector of the set items for cases where
to avoid iterating and deleting items at the same
time at an ItemSet, so changed to using a local
vector of remembered WhichIDs and deleting after
the iterator is done. I also saw some strange
usages of SfxItemIter in sw which i will have to
check.
Since there are still problems with UTs which I can
not reproduce locally I have now added asserts to
the case that an ItemSet gets changed with still
having active SfxItemIter(s). That is always an
error, but with the architecture of that fixed
array of ItemPtrs did not have devastating effects
(purely by coincidence).
With that asserts, UTs run well in SC and SD, but
in SW I get 11 (eleven!) asserts from the UTs, all
of them from 'ITEM: SfxItemSet ClearItem' BTW.
I guess these have to be fixed before thinking
about this change...
Good news is that all 11 cases were the same in SW,
in SwHistorySetAttrSet::SwHistorySetAttrSet which
does some strange things using two SfxItemIter in
parallel. Thus SW UTs are also clear and I see no
more errors caused by ItemSets being changed while
SfxItemIters are alive.
Bad news is that I still have errors to hunt...
NOTE: Have now cleaned all UTs, this showed that
there are some unexpected side-effects of the Items
being processed in another order when SfxItemIter
is used, also found one case where a
WhichRangesContainer is constructed for a
SfxItemSet using the set items from another ItemSet
and SfxItemIter to do so. There *might* be more
cases not covered by UTs.
NOTE: While speed stays the same and mem is reduced
up to 25% (see measurements in 1st comment) another
*important* aspect is that this frees the way for
using ItemSets *without* WhichRanges - these are
necessary mainly to create that fixed array of
pointers to the Items in a *manageable* size. With
a dynamic structure like unordered_set there is
in principle *no need* anymore to use WhichRanges
to pre-define the Items a Set could hold. There
is one exception: We have cases where one ItemSet
is set at another one with defined WhichRanges to
*filter* the contained Items - these would have to
be identified. This is a rare case and we would have
to check cases where an ItemSet gets set at another
ItemSet. This would be as if all ItemSets would be
AllItemSets in principle - much easier for everyone.
NOTE: Waited for 24.8 split just to not take
unnecessary risks.
Change-Id: I75b0ac1d8a4495a3ee93c1117bdf618702785990
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/166972
Reviewed-by: Armin Le Grand <Armin.Le.Grand@me.com>
Tested-by: Jenkins
|
|
Change-Id: I7a1c7dbe61e302584b977fdf202d041d987d0833
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/166777
Reviewed-by: Noel Grandin <noel.grandin@collabora.co.uk>
Tested-by: Jenkins
|
|
2411.71ms -> 2395.36ms
Change-Id: Ie6a3f281b56e827b77dddd1eb8ae6db9d2691c8d
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/166387
Tested-by: Jenkins
Reviewed-by: Caolán McNamara <caolan.mcnamara@collabora.com>
|
|
speed up the matching of duplicates in CellAttributeHelper by using
std::set and partial sorting by name
Takes my time from 33s to 6s
Change-Id: I06376c1e253981cb5a3020142d24fa5776513d4d
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/166262
Tested-by: Jenkins
Reviewed-by: Noel Grandin <noel.grandin@collabora.co.uk>
|
|
we know that ScPatternAttr uses a single, contiguous range of
item ids, so we can compute the item offset at compile time.
Shaves 1-2% off some workloads.
Change-Id: I623b8cb3e0d5d070118117196d2b48575f505725
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/165550
Tested-by: Jenkins
Reviewed-by: Noel Grandin <noel.grandin@collabora.co.uk>
|
|
and for the duration of Threaded calculation where there will be
no new formats required we can drive number formatting with the
unlocked RO policy.
Change-Id: Ic0e449acdcf834bc569d13b4a984f13c55316801
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/165160
Tested-by: Jenkins
Reviewed-by: Caolán McNamara <caolan.mcnamara@collabora.com>
|
|
keep a per-interpret-thread cache of the SvNumberFormatter
mapping while formula-group-threading, this is similar to:
commit c2d8341ee392949274b901abfd44d9645d2e4e36
Date: Tue Oct 15 08:32:22 2019 +0530
Cache last used number-format-type in interpreter-context
Change-Id: Ie99807eaf2f3260cec357f9d66307df0a69826e9
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/164227
Tested-by: Jenkins
Reviewed-by: Caolán McNamara <caolan.mcnamara@collabora.com>
|
|
Change-Id: I14ee125874b6f0f1ff5406a3eafea0b19df1a7f2
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/162082
Tested-by: Jenkins
Reviewed-by: Noel Grandin <noel.grandin@collabora.co.uk>
|
|
Change-Id: I73c8f506ed78238c80df45e087cb6b5bdc496ec9
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/162057
Tested-by: Jenkins
Reviewed-by: Caolán McNamara <caolan.mcnamara@collabora.com>
|
|
ScPatternAttr is traditionally derived from SfxPoolItem
(or better: SfxSetItem) and held in the ScDocumentPool
as Item.
This is only because of 'using' the 'poolable'
functionality of the Item/ItemSet/ItemPool mechanism.
Lots of hacks were added to sc and Item/ItemSet/
ItemPool to make that 'work' which shows already that
this relationship is not optimal.
It uses DirectPutItemInPool/DirectRemoveItemFromPool
to do so, also with massive overhead to do that (and
with not much success). The RefCnt in the SfxPoolItem
that is used for this never worked reliably, so the
SfxItemPool was (ab)used as garbage collector (all
Items added and never removed get deleted at last
for good when the Pool goes down). For this reasons
and to be able to further get ItemSets modernized
I changed this. I did two big changes here:
(1) No longer derive ScPatternAttr from SfxItemSet/
SfxSetItem, no longer hold as SfxPoolItem
(2) Add tooling to reliably control the lifetime of
ScPatternAttr instances and ther uniqueness/
reusage for memory reasons
It is now a regular non-derived class. The SfxItemSet
formally derived from SfxSetItem is now a member. The
RefCnt is now also a member (so independent from
size/data type of SfxPoolItem). All in all it's pretty
much the same size as before.
To support handling it I created a CellAttributeHelper
that is at/owned by ScDocument and takes over tooling
to handle the ScPatternAttr. It supports to guarantee
the uniqueness of incarnated ScPatternAttr instances for
a ScDocument by providing helpers like registerAndCheck
and doUnregister. It hosts the default CellAttribute/
ScPatternAttr. That default handling was anyways not
using the standard default-handling of Items/Pools.
I adapted whole SC to use that mainly by replacing calls
to DirectPutItemInPool with registerAndCheck and
DirectRemoveItemFromPool with doUnregister, BUT: This
was not sufficient, the RefCnt kept to be broken.
For that reason I decided to also do (2) in this change:
I added a CellAttributeHolder that owns/regulates the
lifetime of a single ScPatternAttr. Originally it also
contained the CellAttributeHolder, but after some
thoughts I decided that this is not needed - if there
is no ScPatternAttr set, no CellAttributeHolder is
needed for safe cleanup at destruction of the helper.
So I moved/added the CellAttributeHolder to ScPatternAttr
where it belongs more naturally anyways. The big plus is
that CellAttributeHolder is just one ptr, so not bigger
than having a simple ScPatternAttr*. That way, e.g.
ScAttrEntry in ScAttrArray did not 'grow' at all. In
principle all places where a ScPatternAttr* is used can
now be replaced by using a CellAttributeHolder, except
for construction. It is capable to be initialized with
either ScPatternAttr instances from the heap (it creates
a copy that then gets RefCounted) or allocated (it
supports ownership change at construction time).
Note that ScAttrEntry started to get more a C++ class
in that change, it has a constructor. I did not change
the SCROW member, but that should also be done.
Also made registerAndCheck/doUnregister private in
CellAttributeHelper and exclusively used by
CellAttributeHolder. That way the RefCnt works, and a
lot of code gets much simpler (check ScItemPoolCache,
it's now straightforward) and safer and ~ScPatternAttr()
uses now a hard
assert(!isRegistered());
which shows that RefCnt works now (the 1st time?).
There can be done more (see ToDo section below) but I
myself will concentrate on getting ItemSets forward.
This decoupling makes both involved mechanisms more safe,
less complex and more stable. It also opens up
possibilities to further optimize ScPatternAttr in SC
without further hacking Item/ItemSet/ItemPool stuff.
NOTE: ScPatternAttr *should* be renamed to 'CellAttribute'
which describes what it is. The experiencd devs may know
what it does, but it is a hindrance for understanding for
attacting new devs. I already used now names like
CellAttributeHelper/CellAttributeHolder etc., but
abstained from renaming ScPatternAttr, see ToDo list below.
SfxItemSet discussion:
ScPatternAttr still contains a SfxItemSet, or better, a
SfxSetItem. For that reason it still depends on access to
an SfxItemPool (so there is acces in CellAttributeHelper).
This is in principle not needed - no Item (in the range
[ATTR_PATTERN_START .. ATTR_PATTERN_END]) needs that.
In principle ScPatternAttr could now do it's own handling
of those needed Items, however this might be done (linear
array, hash-by-WhichID, ...). The Items get translated
to and from this to the rest of the office anyways.
Note that *merging* of SfxItemSets is *still* needed what
means to have WhichID slots in SfxItemState::DONTCARE,
see note in ScPatternAttr::ScPatternAttr about that. And
there is also the Surrogates stuff which would have to be
checked.
The other extreme is to use SfxItemSet *more*, e.g. directly
derive from SfxItemSet what would make stuff easier, maybe
even get back to using the 'regular' Items like all office,
I doubt that that would be much slower, so why...?
Also possible is to remove that range of Items exclusively
used by ScPatternAttr from ScDocumentPool *completely* and
create an own Pool for them, owned by CellAttributeHelper.
That Pool might even be static global, so all SC Docs could
share all those Items - maybe even the ScPatternAttr
themselves (except the default per document). That would
remove the dependency of ScPatternAttr from a Pool
completely.
ToDo-List:
- rename ScPatternAttr to CellAttribute or similar
- use SfxItemSetFixed with range [ATTR_PATTERN_START
.. ATTR_PATTERN_END] instead of regular SfxItemSet
(if the copy-construtor works now...?)
- maybe create own/separate Pool for exclusive Items
- make ScAttrEntry more a C++ class by moving SCROW
to the private section, add get/set methods and
adapt SC
Had to add some more usages of CellAttributeHolder
to the SC Sort mechanism, there were situations where
the sorted ScPatternAttr were replaced in the Table,
but the 'sorted' ones were just ScPatternAttr*, thus
deleting the valid ones in the Table already. Using
CellAttributeHolder makes this safe, too.
Added a small, one-entry cache to CellAttributeHelper
to buffer the last found buffered ScPattrnAttr. It
has a HitRate of ca. 5-6% and brings the UnitTest
testSheetCellRangeProperties from 0m48,710s to
0m37,556s. Not too massive, but erery bit counts :-)
Also shows that after that change optimizations in
the now split functionality is possible and easy.
Change-Id: I268a7b2a943ce5ddfe3c75b5e648c0f6b0cedb85
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/161244
Tested-by: Jenkins
Reviewed-by: Armin Le Grand <Armin.Le.Grand@me.com>
|
|
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>
|
|
To understand this, some look back in history will be needed to see
why it is as it is today. In some (reworked) comments 'poolable' is
described as flag to hold Items in the ItemPool, also always having
only one incarnation of each possible Item.
This is not the original intention, but a side-effect. The reason is
what the binary format in the office did: To save a document, the
Objects & the Pool were saved, *not* individual Items *together*
with the objects. The Pool was completely (binary) saved (and loaded)
in one run.
Temporary IDs were used to represent at the objects in file which
Items were referenced. This *required* to have only one incarnation
per item to have a minimal binary file size, thus this high effort
was put into this. At doc load, the pool was loaded, all Items were
set to RefCount 5000, the references from the objects were restored
and then for each Item the RefCount was lowered by 5000 again
and - if being zero - deleted. Items for UI were marked 'non-poolable'
to *not* safe them with the document, so poolable was a flag to decide
if that Info/Item was to be saved with the document - or more direct:
if it is Model Data.
Items are small, so if we prefer runtime it is okay to no longer being
strict with this, anyways does not happen often and has only marginal
memory effects - compared to runtime effects/savings.
Other problems which this caused: One example is that objects in the
UNDO stack were still in the pool, so e.g. deleted pictures were saved
with the document despite no longer being used (!). That is the reason
we have an UndoItemPool and a method MigrateItemPool to move stuff to
that Pool when objects go to the UNDO stack - all of this is also no
longer needed.
Cleaning this up means to ideally have all items in the SfxItemSet,
no longer at the Pool. The Pool should be reduced to a 'Default-Item-
Holder' and a 'Slot-to-whichId-mapper'.
This needs thorough cleanups/removals, but will be worth it because
that massive simplification(s) will increase safety an runtime and make
migrating to the goal of completely type-based ItemSet stuff easier for
the future. Hopefully only view code in the office working with items
will have to be changed for this.
In this 1st step I already found that some 'compromizes' will be
needed:
- There are still Items that have to be at the pool to make the
Surrogate-stuff working. This gives back all Items in a Pool of a type
and is used in ca. 80 cases. Each one looks at these Items *without*
context (e.g. a SfxItemSet at an Object would be a context), so if e.g.
a dialog is open that temporarily uses Items of that type you would
also get these - without knowing about it...
To make that work there is still a mechanism to have Items at the Pool,
but now just *registering* (and un-reg) them without any sort/search/
remove needs. Also only for Items that need that, so I evaluated the
GetItemSurrogates calls and added some asserts when GetItemSurrogates
tries to access an unregistered item type which needs to be added.
- Another caveat is that there are about 250 places that directly put
Items to the Pool (not all remove these, that is done at pool deletion,
so some kind of silent 'garbage-collection' is in place). To have an
overview I renamed the accessing methods to separate them from the same
functionality at the SfxItemSet, which had the same names. An
implementation does still add these directly to the pool, there is no
way to cleanup those usages for now. In principle all these should be
changed to hold the data at an SfxItemSet.
I am still hunting problems. But you can build the office, all apps
work (including chart) and you can do speed comparisons already.
There are test throwing errors, so I hunt these now. It is hard to
give an estimation about how much more changes/corrections will be
needed.
Completed adaptions to new registered Items at Pool, that reduces the
failing tests. Still many that I need to hunt.
Added stuff to work around that 'compromize' in ScDocumentPool: It
overloads ::PutImpl of the pool to implement special handling for
a single Item in SC, the ScPatternAttr. In former code that method
was used from SfxItemSet and ::PutImpl at the pool directly, so it
was only used in one place. I am not sure if it was used from
the SfxItemSet functionality, but better offer it for now. To not
waste too much runtime the callbacks depend on the boolean
'NewItemCallback' at the SfxPoolItem, it gets set for that single
Item in SC and only then the callbacks trigger. I hope to get rid
of those again, e.g. newItem_UseDirect is only needed since we have
no 'real' StaticPoolDefaults currently - another thing that needs to
be cleaned up in a next step.
Since usages of impl(Create|Cleanup)ItemEntry and
Direct(Put|Remove)ItemInPoolImpl got more and more similar I decided to
unify that: move impl(Create|Cleanup)ItemEntry to tooling, make it
globally available in svl and use it also directly for
Direct(Put|Remove)ItemInPoolImpl. This slightly increases the failing
tests again, but only since in Direct(Put|Remove)ItemInPoolImpl that
fallback (e.g. tryToGetEqualItem) was used before, thus this is the
same class of errors (SfxPoolItem ptr-compare) as the others which I
will need to find anyways. Also fixed some missing stuff.
Have now idenified and redirected all SfxPoolItem ptr-compares
to be able to debug these - one cause for the remaining errors is
probably that before with bPoolable those often were sufficient, but
are no longer. Used the [loplugin:itemcompare] and a local clang
build to do so, see https://gerrit.libreoffice.org/c/core/+/157172
Stabilized Direct(Put|Remove)ItemInPoolImpl forwards, added parameter
to implCreateItemEntry to signal that it gets called from DirectPool
stuff - currently needed. Hopefully when getting rid of that DirectPool
stuff we can remove that again
Added two more debug functionalities:
- Added a SerialNumber to allow targeted debugging for deterministic
cases
- Added registering & listing of still-allocated SfxPoolItems at
office shutdown
Found PtrComp error in thints.cxx - POC, thanks to
areSfxPoolItemPtrsEqual. Will hopefully help more with other tests
Found some wrong asserts/warnings where I was too careful and not
finding something/succeeding is OK, fixes some UnitTests for SC
For SC I now just tried to replace all areSfxPoolItemPtrsEqual with
the full-ptr-content compare SfxPoolItem::areSame. I also needed to
experiment/adapt the newItem_Callback solution but got it working.
Did that replacement now for SW too, found some places where the
direct ptr compare is OK.
Continued for the rest of occurrences, now all 160 places evaluated.
Also done some cleanups.
Massive cleanups of stuff no longer needed with this paradigm change.
Also decided to keep tryToGetEqualItem/ITEM_CLASSIC_MODE for now.
It is used for *one* Item (ScPatternAttr/ATTR_PATTERN) in SC that
already needs many exceptions. Also useful for testing if errors
come up on this change to test if it is related to this.
Added forwarding of target Pool for ::Clone in SvxSetItem and
SvxSetItem, simplified SfxStateCache::SetState_Impl and returned
to simple ptr compares in SfxPoolItem::areSame to not do the test
in areSfxPoolItemPtrsEqual.
Debugged through UITest_calc_tests9 and found that in tdf133629
where BoxStyle is applied to fully selected empty calc the Item-
reuse fallback has to be used not only for ATTR_PATTERN, see
comment @implCreateItemEntry. Maybe more...
Problem with test_tdf156611_insert_hyperlink_like_excel. Found that
in ScEditShell::GetFirstURLFieldFromCell the correct SvxURLField
is found and returned as ptr, but it's usage crashes. That is due to
the SfxItemSet aEditSet used there gets destroyed at function return
what again deletes the SvxFieldItem that is holding the SvxURLField
that gets returned.
This shows a more general problem: There is no 'SfxPoolItemHolder'
that safely holds a single SfxPoolItem - like a SfxItemSet for a
single Item (if Items would be shared_ptrs, that would be a safe
return value).
That will be needed in the future, but for now use another solution:
Since I see no reason why EE_FEATURE_FIELD should not be shareable
I wil change this for ow in the SfxItemInfo for EditCharAttribField.
That way the Item returned will be shared (RefCnt > 1) and thus not
be deleted.
I changed the return value for GetURLField() and
GetFirstURLFieldFromCell() in ScEditShell: At least for
GetFirstURLFieldFromCell the return type/value was not safe: The
SvxFieldItem accessed there and held in the local temporary
SfxItemSet may be deleted with it, so return value can be
corrupted/deleted. To avoid that, return a Clone of SvxFieldData
as a unique_ptr.
With all that UnitTest debugging and hunting and to get the paradigm
change working to no longer rely on shared/pooled items I lost a
little bit focus on speed, so I made an optimization round for the
two central methods implCreateItemEntry/implCleanupItemEntry to
get back to the speed improvements that I detected when starting this
change. It was mainly lost due to that 'strange' chained pool stuff
we have, so I added to detect the target pool (the one at which the
WhichID is registered) directly and only once. Next thing to cleanup
will/should be the pool and it's concept, all this is not needed
and really costs runtime.
Since implCreateItemEntry/implCleanupItemEntry are executed millions
of times, each cycle counts here.
Had an error in the last changes: pool::*_Impl methods use index
instead of WhichID - most of them. Another bad trap, I really need
to cleanup pool stuff next.
Change-Id: I6295f332325b33268ec396ed46f8d0a1026e2d69
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/157559
Tested-by: Jenkins
Reviewed-by: Armin Le Grand <Armin.Le.Grand@me.com>
|
|
since
commit 3537cef02c25c2c2459d7900eed13eeec533b7ae
Author: Tomaž Vajngerl <tomaz.vajngerl@collabora.co.uk>
Date: Tue May 16 22:10:10 2023 +0900
sc: factor out color from setting vcl::Font from a ItemSet
Change-Id: Ia8781d69ed4969eb9617c96c6991cc39ce162e49
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/154061
Tested-by: Jenkins
Reviewed-by: Noel Grandin <noel.grandin@collabora.co.uk>
|
|
Change-Id: I2544c7ece152323d84faafe1a544e4f89ca466d5
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/152014
Tested-by: Jenkins
Reviewed-by: Tomaž Vajngerl <quikee@gmail.com>
|
|
Change-Id: Id34bac78719943bd4c4cbfa60e0cb86b4ca570f2
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/153562
Tested-by: Jenkins
Reviewed-by: Noel Grandin <noel.grandin@collabora.co.uk>
|
|
Cache visibility for use in ScPatternAttr::IsVisible
Change-Id: Ia248a06b876b53115bbc3017d97538bbbc03830d
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/153482
Tested-by: Jenkins
Reviewed-by: Noel Grandin <noel.grandin@collabora.co.uk>
|
|
vcl::Font color parameter is deprecated so we need to handle the
color separately from font data. This refactors GetFont into 2
separate functions - fillFontOnly and fillColor, where fillFont
now does the same as previously GetFont function did.
All GetFont calls have been changed depending on if we need only
the font data or also color - where the color is now treated in
a different call. There are a couple of calls where fillFont was
used, because to change that needs a more complex refactoring.
Change-Id: I0a2ce50a0cb28d196fcff87e1e80099a2bb60a9e
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/151858
Tested-by: Jenkins
Reviewed-by: Tomaž Vajngerl <quikee@gmail.com>
|
|
For whatever reason e5fe8434110c1299527a0d03bf450e1b6d08ca57 "use a SIMD
friendly hashcode for ScPatternAttr" had changed ScPatternAttr::mxHashCode from
sizt_t to sal_Int32, better use unsigned sal_uInt32 to avoid
> /sc/source/core/data/patattr.cxx:1444:17: runtime error: signed integer overflow: 31 * 887503681 cannot be represented in type 'int'
during e.g. CppunitTest_chart2_geometry
(<https://ci.libreoffice.org//job/lo_ubsan/2802/>)
Change-Id: Ia561b245431d812f43586013477208426aa1cc11
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/152697
Tested-by: Jenkins
Reviewed-by: Stephan Bergmann <sbergman@redhat.com>
|
|
which shaves 5% off load time of a large sheet with lots of formats.
We are calculating a hash over a decent sized array of pointers, so
unroll the loop and reduce data-dependencies to give the compiler (and
the CPU) a chance to do work in parallel
Change-Id: I3d58fcf547c9280437295b9c9ec10aff16419afc
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/152443
Tested-by: Jenkins
Reviewed-by: Noel Grandin <noel.grandin@collabora.co.uk>
|
|
If the two item sets are equal, then trivially nothing has changed
between them.
Change-Id: Ib4c5922959603a0a28e11967a17ba4607e6a010f
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/131080
Tested-by: Jenkins
Reviewed-by: Luboš Luňák <l.lunak@collabora.com>
|
|
The LookupHashCode() way still loops over an array while calling
virtual functions for a type that is actually known. Overriding
instead a whole new Lookup() function that does the loop
avoids the virtual calls, allowing compiler optimizations such
as inlining, or class-specific optimizations like the hash code.
So this still improves performance with ScPatternAttr a bit,
for tdf#135215 it saves about 40% of load time, but this is
scraping the bottom of the barrel, as this is still a linear search
(the IsSortable() approach, if it worked, would be still 3x faster).
Change-Id: I8fe5f70cabb77e2f6619d169beee8a3b4da46213
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/130228
Tested-by: Jenkins
Reviewed-by: Luboš Luňák <l.lunak@collabora.com>
|
|
I made a mistake in 98d51bf8ce13bdac2d71f50f58d6d0ddb by using
SfxItemSet::Count(), which returns a number of existing items.
But EqualPatternSets() compares all items, even empty ones,
so I should have used TotalCount() (which is actually hardcoded
as a number in the code). Without this, the hash usually didn't
include all items, leading to matching hashes even though
the items clearly didn't match.
Change-Id: I1e25752d3ddca2b63c3c295a9a425965531cd4d3
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/130210
Tested-by: Jenkins
Reviewed-by: Noel Grandin <noel.grandin@collabora.co.uk>
Reviewed-by: Luboš Luňák <l.lunak@collabora.com>
|
|
See tdf#42949 for motivation
Change-Id: I867e1f7a2c44210de3281b36e22708a5d32ddb7f
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/129476
Tested-by: Jenkins
Reviewed-by: Thorsten Behrens <thorsten.behrens@allotropia.de>
|
|
As pointed out by 585e0ac43b9bd8a2f714903034e435c84ae3fc96, some
item types cannot be used as IsSortable(), because they are modified
after having been inserted in the pool, which breaks the sorted
order. But it's possible to at least somewhat improve performance
of these items by explicitly providing a hash code and using
that first for comparisons when looking up items, which may be
cheaper than calling operator==.
With ScPatternAttr such comparisons seem to take only 60% of the
original time, reducing loading time of some documents by 25%.
Change-Id: I41f4dda472fb6db066742976672f2c08b9aeef63
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/129667
Reviewed-by: Noel Grandin <noel.grandin@collabora.co.uk>
Reviewed-by: Luboš Luňák <l.lunak@collabora.com>
Tested-by: Jenkins
|
|
rather than on the heap, avoiding an allocation
Change-Id: I3f1504f9a2d4178a9ba59e98de182a0ab98cdce0
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/118356
Tested-by: Jenkins
Reviewed-by: Noel Grandin <noel.grandin@collabora.co.uk>
|
|
Change-Id: I77ba873ed236091443627e26e6127e89cd8e15bd
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/118360
Tested-by: Jenkins
Reviewed-by: Noel Grandin <noel.grandin@collabora.co.uk>
|
|
sc, scaddins, sccomp, scripting
Change-Id: Ia99fec9e238033821cb784810edd4762c09bd5db
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/112049
Tested-by: Jenkins
Reviewed-by: Ilmari Lauhakangas <ilmari.lauhakangas@libreoffice.org>
|
|
Change-Id: I78f837a1340be0ca5c49097f543a481b7b43a632
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/108367
Tested-by: Jenkins
Reviewed-by: Noel Grandin <noel.grandin@collabora.co.uk>
|
|
Change-Id: I8f37a8d1174ed816df971b8cee036d4e88d4a7fc
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/104526
Tested-by: Jenkins
Reviewed-by: Noel Grandin <noel.grandin@collabora.co.uk>
|
|
Change-Id: Ie06fef80990b539d5b6cc87c80d9bbd3e851766c
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/103299
Tested-by: Jenkins
Reviewed-by: Caolán McNamara <caolanm@redhat.com>
|
|
...now that macOS builds are guaranteed to have std::optional since
358146bbbd1b9775c12770fb5e497b6ec5adfc51 "Bump macOS build baseline to
Xcode 11.3 and macOS 10.14.4".
The change is done mostly mechanically with
> for i in $(git grep -Fl optional); do
> sed -i -e 's:<o3tl/optional\.hxx>\|\"o3tl/optional\.hxx\":<optional>:' \
> -e 's/\<o3tl::optional\>/std::optional/g' \
> -e 's/\<o3tl::make_optional\>/std::make_optional/g' "$i"
> done
> for i in $(git grep -Flw o3tl::nullopt); do
> sed -i -e 's/\<o3tl::nullopt\>/std::nullopt/g' "$i"
> done
(though that causes some of the resulting
#include <optional>
to appear at different places relative to other includes than if they had been
added manually), plus a few manual modifications:
* adapt bin/find-unneeded-includes
* adapt desktop/IwyuFilter_desktop.yaml
* remove include/o3tl/optional.hxx
* quote resulting "<"/">" as "<"/">" in officecfg/registry/cppheader.xsl
* and then solenv/clang-format/reformat-formatted-files
Change-Id: I68833d9f7945e57aa2bc703349cbc5a56b342273
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/89165
Tested-by: Jenkins
Reviewed-by: Stephan Bergmann <sbergman@redhat.com>
|
|
by avoid some of the EqualPatternSets memcmp cost by using a
hashcode, memcmp with lots of large and similar arrays ends up
blowing the L1/L2 cache
Takes the time from 64s to 59s for me.
Change-Id: I6961aa34f4e7457a70cb75a37dfae50d5f328589
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/85918
Tested-by: Jenkins
Reviewed-by: Noel Grandin <noel.grandin@collabora.co.uk>
|
|
and can then remove some casting
Change-Id: Id821c32ca2cbcdb7f57ef7a5fa1960042e630ffc
Reviewed-on: https://gerrit.libreoffice.org/85022
Tested-by: Jenkins
Reviewed-by: Caolán McNamara <caolanm@redhat.com>
Tested-by: Caolán McNamara <caolanm@redhat.com>
|
|
...with a boost::optional fallback for Xcode < 10 (as std::optional is only
available starting with Xcode 10 according to
<https://en.cppreference.com/w/cpp/compiler_support>, and our baseline for iOS
and macOS is still Xcode 9.3 according to README.md). And mechanically rewrite
all code to use o3tl::optional instead of boost::optional.
One immediate benefit is that disabling -Wmaybe-uninitialized for GCC as per
fed7c3deb3f4ec81f78967c2d7f3c4554398cb9d "Slience bogus
-Werror=maybe-uninitialized" should no longer be necessary (and whose check
happened to no longer trigger for GCC 10 trunk, even though that compiler would
still emit bogus -Wmaybe-uninitialized for uses of boost::optional under
--enable-optimized, which made me ponder whether this switch from
boost::optional to std::optional would be a useful thing to do; I keep that
configure.ac check for now, though, and will only remove it in a follow up
commit).
Another longer-term benefit is that the code is now already in good shape for an
eventual switch to std::optional (a switch we would have done anyway once we no
longer need to support Xcode < 10).
Only desktop/qa/desktop_lib/test_desktop_lib.cxx heavily uses
boost::property_tree::ptree::get_child_optional returning boost::optional, so
let it keep using boost::optional for now.
After a number of preceding commits have paved the way for this change, this
commit is completely mechanical, done with
> git ls-files -z | grep -vz -e '^bin/find-unneeded-includes$' -e '^configure.ac$' -e '^desktop/qa/desktop_lib/test_desktop_lib.cxx$' -e '^dictionaries$' -e '^external/' -e '^helpcontent2$' -e '^include/IwyuFilter_include.yaml$' -e '^sc/IwyuFilter_sc.yaml$' -e '^solenv/gdb/boost/optional.py$' -e '^solenv/vs/LibreOffice.natvis$' -e '^translations$' -e '\.svg$' | xargs -0 sed -i -E -e 's|\<boost(/optional)?/optional\.hpp\>|o3tl/optional.hxx|g' -e 's/\<boost(\s*)::(\s*)(make_)?optional\>/o3tl\1::\2\3optional/g' -e 's/\<boost(\s*)::(\s*)none\>/o3tl\1::\2nullopt/g'
(before committing include/o3tl/optional.hxx, and relying on some GNU features).
It excludes some files where mention of boost::optional et al should apparently
not be changed (and the sub-repo directory stubs). It turned out that all uses
of boost::none across the code base were in combination with boost::optional, so
had all to be rewritten as o3tl::nullopt.
Change-Id: Ibfd9f4b3d5a8aee6e6eed310b988c4e5ffd8b11b
Reviewed-on: https://gerrit.libreoffice.org/84128
Tested-by: Jenkins
Reviewed-by: Stephan Bergmann <sbergman@redhat.com>
|
|
Change-Id: I6a08a86262deae4bed3a05f77d3041a568f23595
Reviewed-on: https://gerrit.libreoffice.org/81853
Tested-by: Jenkins
Reviewed-by: Noel Grandin <noel.grandin@collabora.co.uk>
|
|
it turns out that
soffice --headless --convert-to pdf tdf89833-2.xlsm
semi-reliably fails with
stl concept checking "Error: attempt to compare a dereferenceable
iterator to a singular iterator"
because we are modifying ScPatternAttr all over the place, which messes
with the sorted list in the pool.
Change-Id: Id662ac32b024c2c60b32b6cf433c12deb614f0fa
Reviewed-on: https://gerrit.libreoffice.org/71677
Tested-by: Jenkins
Reviewed-by: Noel Grandin <noel.grandin@collabora.co.uk>
|
|
This takes the loaing time from 1m38 to 15s.
Speed up finding existing pool items in the pool by storing a
sorted_vector, sorted by a compare operator on the item.
memcmp is faster than operator< on std::vector because operator< seems
to be trying to use some kind of lexicographical compare
Change-Id: Id72527106d604adb8cd2d158bb42f89e2b16a85d
Reviewed-on: https://gerrit.libreoffice.org/71009
Tested-by: Jenkins
Reviewed-by: Noel Grandin <noel.grandin@collabora.co.uk>
|
|
Found with bin/find-unneeded-includes
Removal proposals are dealt with here,
and a bit of fallout management.
Change-Id: If2d73998c3d3d9fea50420688818cd7fe0f0a27c
Reviewed-on: https://gerrit.libreoffice.org/54463
Tested-by: Jenkins <ci@libreoffice.org>
Reviewed-by: Miklos Vajna <vmiklos@collabora.co.uk>
|
|
instead of using std:unique_ptr to allocate small objects on the heap
Change-Id: Ifd309a9bf331910354406b827b89a0363f3b7eda
Reviewed-on: https://gerrit.libreoffice.org/51945
Tested-by: Jenkins <ci@libreoffice.org>
Reviewed-by: Noel Grandin <noel.grandin@collabora.co.uk>
|
|
instead of a mix of short/sal_uLong/sal_uInt32
Change-Id: Ie5bd26e1a6f716c0c4e174a6d560827084b3f421
Reviewed-on: https://gerrit.libreoffice.org/45159
Tested-by: Jenkins <ci@libreoffice.org>
Reviewed-by: Noel Grandin <noel.grandin@collabora.co.uk>
|
|
Change-Id: Iaa326332f5806477dd81463e6b6004a962bac934
Reviewed-on: https://gerrit.libreoffice.org/45128
Reviewed-by: Noel Grandin <noel.grandin@collabora.co.uk>
Tested-by: Noel Grandin <noel.grandin@collabora.co.uk>
|
|
Change-Id: I4fd18096d7d22d8c146a2437906187d5df1cb268
Reviewed-on: https://gerrit.libreoffice.org/41447
Reviewed-by: Noel Grandin <noel.grandin@collabora.co.uk>
Tested-by: Noel Grandin <noel.grandin@collabora.co.uk>
|
|
Change-Id: Id31c8de69043d393f005f83d5c7eba878af5119c
Reviewed-on: https://gerrit.libreoffice.org/41149
Tested-by: Jenkins <ci@libreoffice.org>
Reviewed-by: Michael Stahl <mstahl@redhat.com>
|
|
Change-Id: I219dd03477862169cd50eecc14822f6a023f879a
|
|
Change-Id: Ic96fd3b56b2063df0882168a7d02725d3c50515f
Reviewed-on: https://gerrit.libreoffice.org/32961
Reviewed-by: Noel Grandin <noel.grandin@collabora.co.uk>
Tested-by: Noel Grandin <noel.grandin@collabora.co.uk>
|
|
Change-Id: Ia726ce9e9fcfe9a8b5bc4057f0176edf10b6ab3f
|
|
This should ensure that as long as the style does not change the cell
style name is the same after an import export cycle.
Each ScPatternAttr stores a unique ID and we store the ID to name
mapping during import. During export if we find a ScPatternAttr that has
a key that is also stored in the map we write back the style name from
the map.
To avoid name collisions we block the style names from the import for
the export.
The missing piece to make this completely awesome is now to make sure
that styles are sorted by name during export. That way we can reduce the
diff between import and export even more.
Change-Id: Ie4fe2aa00f07efec27ea129e314ac0b6b7e0d8c0
Reviewed-on: https://gerrit.libreoffice.org/29255
Tested-by: Jenkins <ci@libreoffice.org>
Reviewed-by: Markus Mohrhard <markus.mohrhard@googlemail.com>
|
|
The issue of 362d4f0cd4e50111edfae9d30c90602c37ed65a2 "Explicitly mark
overriding destructors as 'virtual'" appears to no longer be a problem with
MSVC 2013.
(The little change in the rewriting code of compilerplugins/clang/override.cxx
was necessary to prevent an endless loop when adding "override" to
OOO_DLLPUBLIC_CHARTTOOLS virtual ~CloseableLifeTimeManager();
in chart2/source/inc/LifeTime.hxx, getting stuck in the leading
OOO_DLLPUBLIC_CHARTTOOLS macro. Can't remember what that
isAtEndOfImmediateMacroExpansion thing was originally necessary for, anyway.)
Change-Id: I534c634504d7216b9bb632c2775c04eaf27e927e
|
|
Change-Id: I82c78dd880c98532db407b0183a43705be2de67c
Reviewed-on: https://gerrit.libreoffice.org/28777
Tested-by: Jenkins <ci@libreoffice.org>
Reviewed-by: Noel Grandin <noelgrandin@gmail.com>
|
