Versioning performance with a lot of relationships

[belendel]

Hi,

It seems the recursive DeepCopy used when saving a new version has a serious performance impact when you have a lot of relationships. The following is a blackfire profile for a batch edit of 25 objects:

https://blackfire.io/profiles/9902f36c-0857-4511-ac3a-54bf25e10a29/graph

When I make a shallow copy it's significantly faster. Is there any reason for traversing all of the related objects? Of course direct relationships have to be included because they are shown in the version tab but any deeper than this is not included in the version data anyway.

[brusch]

Related to #4154

[weisswurstkanone]

Can you confirm that you are on the latest release? With 6.1.0 we added a ReplaceFilter which optimize the whole stuff.

See #4667

[weisswurstkanone]

Not directly related but for completeness: #5858

[brusch]

With #4667 the issue with many relations should be resolved.
#5858 brings also some additional improvements, especially for Data Objects.

One thing that can be further improved is that we're setting children to null, before doing the deep copy.

[weisswurstkanone]

Not setting them to null will still not cause that they get traversed, right?
Reason is that they are treated just like a relation (replaced with an ElementDescriptor)

[kubaplas]

Hi,
I can contribute my profile also: https://blackfire.io/profiles/8252677b-5d00-406f-a1f1-53f8bbe2066a/graph
Somewhat complex data model with classification store, but most of the fields are empty.
Using dev-master (80b6afa)

[weisswurstkanone]

see #5917

[belendel]

Can you confirm that you are on the latest release? With 6.1.0 we added a ReplaceFilter which optimize the whole stuff.

See #4667

This was tested with 6.4.0. I believe it still traverses the whole tree even with the ReplaceFilter. I can do a new profile with the latest version if you want.

[weisswurstkanone]

thx!

[kubaplas]

@weisswurstkanone I think that #5917 is not related to this task - I've made profiles for #5917 with versioning disabled.

[weisswurstkanone]

It is related to the general performance of versioning :-) Just wanted to link them together.

[weisswurstkanone]

not directly related to what you describe but may help as well:
#5922

[weisswurstkanone]

@belendel any new findings ?

[belendel]

@weisswurstkanone I will upgrade our pimcore installation tomorrow and run some new profiles and try to look into the issue a bit myself. I will keep you updated.

[weisswurstkanone]

great, thanks a lot!

[belendel]

Good morning!

I did a bit of investigation and it seems the issue wasn't actually in the relationships, but in the Pimcore\Model\DataObject\Concrete::o_class property. This is an object of type Pimcore\Model\DataObject\ClassDefinition.

The class definition I did my tests with has a large amount of fields, including a classification store. The performance didn't improve after updating to version 6.5.3. I ran a profile on a custom route where I update the same object 3 times with a random value. You can find the first profile here:

https://blackfire.io/profiles/ef60826a-3349-405e-b8aa-e8e9fa167cd9/graph

I assume the ClassDefinition doesn't need to be included in the copy since it isn't used for the version data. So I added a SetNullFilter like this:

$copier->addFilter(new \DeepCopy\Filter\SetNullFilter(), new \DeepCopy\Matcher\PropertyTypeMatcher('Pimcore\Model\DataObject\ClassDefinition'));

Profile after changes:

https://blackfire.io/profiles/eb229b13-336c-4c10-8862-19eb0c122055/graph

This sped up the saving almost 4 times. Let me know if you have any questions!

[weisswurstkanone]

Thanks for investigating and sharing 👍

I believe we already adressed this. Milestone is 6.6

See #5922

Can you confirm if this yields the same results?

[belendel]

Same issue:

https://blackfire.io/profiles/9760fdda-43d1-4a0a-8e33-cd92e67b0222/graph

I am not as familiar with the code as you guys, but I don't see how this change will filter out the $o_class property.

1. The PimcoreClassDefinitionMatcher only matches when the class property is found as FieldDefinition on the ClassDefinition. I assume the actual $o_class property is not a FieldDefinition.
2. Data\CustomVersionMarshalInterface isn't actually implemented anywhere:https://github.com/pimcore/pimcore/search?q=CustomVersionMarshalInterface&unscoped_q=CustomVersionMarshalInterface

Is there a reason to not just set the $o_class property to null? I assume it's not necessary for the versioning.

[weisswurstkanone]

Nulling it out is already done via (since 6.5.2)
#5858

#5922 then makes sure that o_class doesn't get filled in again.

[belendel]

I think the problem is in DeepCopy\DeepCopy::copyObject()

You will find the following code:

foreach (ReflectionHelper::getProperties($reflectedObject) as $property) {
            $this->copyObjectProperty($newObject, $property);
        }

It does the foreach on the original object. Not the cloned object. Setting it to null in __clone() doesn't solve the issue in this case.

[weisswurstkanone]

Good catch!

[dpfaffenbauer]

Shouldn't a Filter work to not copy over o_class?

[weisswurstkanone]

That is what he suggested

[dpfaffenbauer]

oops, sorry, missed that.

[weisswurstkanone]

Created a PR
#5981

Thanks @belendel

[belendel]

Great! Thanks a lot 👍

[brusch]

@belendel @weisswurstkanone #5981 fixed the issue, right? So we can close this ticket?
Thanks for your feedback!

[weisswurstkanone]

@belendel ok from your side ?

[belendel]

Sorry, missed your message. Everything is ok now! You can close the ticket.

[weisswurstkanone]

thx @belendel