Address performance of existing unique-name generation (Part 2) (#18676)

As described in Issue 16849, the existing Tools::getUniqueName method
requires calling code to form a vector of existing names to be avoided.

This leads to poor performance both in the O(n) cost of building such a
vector and also getUniqueName's O(n) algorithm for actually generating
the unique name (where 'n' is the number of pre-existing names).

This has  particularly noticeable cost in documents with large numbers
of DocumentObjects because generating both Names and Labels for each new
object incurs this cost. During an operation such as importing this
results in an O(n^2) time spent generating names.

The other major cost is in the saving of the temporary backup file,
which uses name generation for the "files" embedded in the Zip file.
Documents can easily need several such "files" for each object in the
document.

This update includes the following changes to use the newly-added
UniqueNameManager as a replacement for the old Tools::getUniqueName
method and deletes the latter to remove any temptation to use it as
its usage model breeds inefficiency:

Eliminate Tools::getUniqueName, its local functions, and its unit tests.

Make DocumentObject naming use the new UniqueNameManager class.

Make DocumentObject Label naming use the new UniqueNameManager class.
This needs to monitor DocumentObject Labels for changes since this
property is not read-only. The special handling for the Label
property, which includes optionally forcing uniqueness and updating
links in referencing objects, has been mostly moved from
PropertyString to DocumentObject.

Add Document::containsObject(DocumentObject*) for a definitive
test of an object being in a Document. This is needed because
DocumentObjects can be in a sort of limbo (e.g. when they are in the
Undo/Redo lists) where they have a parent linkage to the Document but
should not participate in Label collision checks.

Rename Document.getStandardObjectName to getStandardObjectLabel
to better represent what it does.

Use new UniqueNameManager for Writer internal filenames within the zip
file.

Eliminate unneeded Reader::FileNames collection. The file names
already exist in the FileList collection elements. The only existing
use for the FileNames collection was to determine if there were any
files at all, and with FileList and FileNames being parallel
vectors, they both had the same length so FileList could be used
for this test..

Use UniqueNameManager for document names and labels. This uses ad hoc
UniqueNameManager objects created on the spot on the assumption that
document creation is relatively rare and there are few documents, so
although the cost is O(n), n itself is small.

Use an ad hoc UniqueNameManager to name new DymanicProperty entries.
This is only done if a property of the proposed name already exists,
since such a check is more-or-less O(log(n)), almost never finds a
collision, and avoids the O(n) building of the UniqueNameManager.
If there is a collision an ad-hoc UniqueNameManager is built
and discarded after use.
The property management classes have a bit of a mess of methods
including several to populate various collection types with all
existing properties. Rather than introducing yet another such
collection-specific method to fill a UniqueNameManager, a
visitProperties method was added which calls a passed function for
each property. The existing code (e.g. getPropertyMap) would be
simpler if they all used this but the cost of calling a lambda
for each property must be considered. It would clarify the semantics
of these methods, which have a bit of variance in which properties
populate the passed collection, e.g. when there are duplicate names..
Ideally the PropertyContainer class would keep a central directory of
all properties ("static", Dynamic, and exposed by ExtensionContainer and
other derivations) and a permanent UniqueNameManager. However the
Property management is a bit of a mess making such a change a project
unto itself.
This commit is contained in:
Kevin Martin
2025-02-24 11:23:53 -05:00
committed by GitHub
parent 2780c9b13e
commit 41f09db9e1
30 changed files with 581 additions and 551 deletions

View File

@@ -25,6 +25,11 @@
#include "PreCompiled.h"
#ifndef _PreComp_
#include <stack>
#include <memory>
#include <map>
#include <set>
#include <vector>
#include <string>
#endif
#include <App/DocumentObjectPy.h>
@@ -814,6 +819,69 @@ void DocumentObject::onBeforeChange(const Property* prop)
signalBeforeChange(*this, *prop);
}
std::vector<std::pair<Property*, std::unique_ptr<Property>>>
DocumentObject::onProposedLabelChange(std::string& newLabel)
{
// Note that this work can't be done in onBeforeChangeLabel because FeaturePython overrides this
// method and does not initially base-call it.
// We re only called if the new label differs from the old one, and our code to check duplicates
// may not work if this is not the case.
std::string oldLabel = Label.getStrValue();
assert(newLabel != oldLabel);
if (!isAttachedToDocument()) {
return {};
}
App::Document* doc = getDocument();
if (doc->isPerformingTransaction()
|| (doc->testStatus(App::Document::Restoring)
&& !doc->testStatus(App::Document::Importing))) {
return {};
}
static ParameterGrp::handle _hPGrp;
if (!_hPGrp) {
_hPGrp = App::GetApplication().GetParameterGroupByPath("User parameter:BaseApp/Preferences/Document");
}
if (doc && !newLabel.empty() && !_hPGrp->GetBool("DuplicateLabels") && !allowDuplicateLabel()
&& doc->containsLabel(newLabel)) {
// We must ensure the Label is unique in the document (well, sort of...).
std::string objName = getNameInDocument();
if (doc->haveSameBaseName(objName, newLabel)) {
// The base name of the proposed label equals the base name of the object Name, so we
// use the object Name, which could actually be identical to another object's Label, but
// probably isn't.
newLabel = objName;
}
else {
// Otherwise we generate a unique Label using newLabel as a prototype name. In doing so,
// we must also act as if the current value of the property is not an existing Label
// entry.
// We deregister the old label so it does not interfere with making the new label,
// and re-register it after. This is probably a bit less efficient that having a special
// make-unique-label-as-if-this-one-did-not-exist method, but such a method would be a real
// ugly wart.
doc->unregisterLabel(oldLabel);
newLabel = doc->makeUniqueLabel(newLabel);
doc->registerLabel(oldLabel);
}
}
// Despite our efforts to make a unique label, onBeforeLabelChange can change it.
onBeforeChangeLabel(newLabel);
if (oldLabel == newLabel || getDocument()->testStatus(App::Document::Restoring)) {
// Don't update label reference if we are restoring or if the label is unchanged.
// When importing (which also counts as restoring), it is possible the
// new object changes its label. However, we cannot update label
// references here, because object being restored is not based on
// dependency order. It can only be done in afterRestore().
//
// See PropertyLinkBase::restoreLabelReference() for more details.
return {};
}
return PropertyLinkBase::updateLabelReferences(this, newLabel.c_str());
}
void DocumentObject::onEarlyChange(const Property* prop)
{
if (GetApplication().isClosingAll()) {
@@ -836,6 +904,11 @@ void DocumentObject::onEarlyChange(const Property* prop)
/// get called by the container when a Property was changed
void DocumentObject::onChanged(const Property* prop)
{
if (prop == &Label && _pDoc && _pDoc->containsObject(this) && oldLabel != Label.getStrValue()) {
_pDoc->unregisterLabel(oldLabel);
_pDoc->registerLabel(Label.getStrValue());
}
if (isFreezed() && prop != &Visibility) {
return;
}