Fix performance issue when using notify tracking policy and calling `flush($entity)` multiple times #5579

xhuberty · 2015-12-29T15:05:11Z

Hi,

First of all I want to apologize for the poor quality of my English.

We had some performance issues in one of our project and we decide to use the Notify tracking policy.
This tracking policy is really usefull for batch and import. For your information we improved the performance of more than 80%.

During our test we discover that flush operations were cleaning the $entityChangeset of the UnitOfWork.
This is a real issue for us because we are using flush with null and entity as parameter.
Example:

$object1->setData($data);
$object2->setData($data);
$this->entityManager->flush($object1);
$this->entityManager->flush($object2);

With the notify tracking policy this code above will only save the change of the object1. All the other changes are lost.

Ocramius · 2015-12-29T16:04:50Z

@xhuberty the change looks good, but it really needs to be isolated in private API (split into private methods).

Ocramius · 2015-12-29T16:09:00Z

lib/Doctrine/ORM/UnitOfWork.php

+        if ($entity === null) {
+            $this->entityChangeSets = $this->scheduledForSynchronization = array();
+        } elseif (is_object($entity)) {
+            $oid = spl_object_hash($entity);


Will need separate tests to verify that the other changesets are still there

xhuberty · 2015-12-29T17:11:53Z

I think I made the modification you ask for except the one concerning the unit test.

Ocramius · 2015-12-29T19:19:21Z

lib/Doctrine/ORM/UnitOfWork.php

        $this->orphanRemovals = array();
+
+        if ($entity === null) {


Please reverse this (yoda)

Sorry but I don't understand what you want. I used the exact same code as the one of the beginning of the commit method.

Ocramius · 2015-12-29T19:21:26Z

@xhuberty we'd need tests before merging, heh

xhuberty · 2015-12-30T09:56:40Z

@Ocramius can you please tell me exactly what unit test you need/want? I tought the unit test will already covered my modifications execpt for the 2 mtethods I have created.

xhuberty · 2015-12-30T10:54:52Z

I made the change with the Yoda condition.

I will regroup all the commit in one when everything will be ok.

Ocramius · 2015-12-30T12:03:32Z

@xhuberty a test should ideally have a UoW with 3 entries ($entity1, $entity2, $entity3), and then you should commit($entity1) or commit([$entity1, $entity2]) and verify that $entity2 (first case) and $entity3 changesets are still there.

Ocramius · 2015-12-30T12:26:50Z

lib/Doctrine/ORM/UnitOfWork.php

@@ -3111,7 +3133,19 @@ public function registerManaged($entity, array $id, array $data)
     */
    public function clearEntityChangeSet($oid)
    {
-        $this->entityChangeSets[$oid] = array();
+        if (isset($this->entityChangeSets[$oid])) {


no need for an isset() check

xhuberty · 2016-01-05T09:34:34Z

I made the unit test, and I have regroup all the commit in one.

stloyd · 2016-01-05T10:20:41Z

tests/Doctrine/Tests/ORM/UnitOfWorkTest.php

+        $this->_unitOfWork->persist($entity2);
+
+        $this->_unitOfWork->commit($entity1);
+        $this->assertEquals(1, count($this->_unitOfWork->getEntityChangeSet($entity2)));


Use assertCount() instead.

xhuberty · 2016-01-05T15:08:16Z

Use of assertCount and fix indentation issue.

Everything regoup in the same commit.

xhuberty · 2016-01-11T13:38:08Z

Hi guys,

Just wanting to know if I need to change something else for this PR to be merged?

And if it's going to be merged when do you thing this will be released? We really need this modification for one of our projects. We are using a fork for the moment but this is not a viable option for the future.

Best regards.

Ocramius · 2016-01-11T16:29:08Z

@xhuberty I won't get at merging/reviewing stuff for a while for now. This will likely be part of 2.6 though (adding milestone)

Ocramius · 2016-01-11T16:30:15Z

tests/Doctrine/Tests/ORM/UnitOfWorkTest.php

+        $this->assertCount(1, $persister->getInserts());
+
+        // Create and Set first entity
+        $entity1 = new NotifyChangedEntity;


These assertions should be moved to their own test case, with an @group 5579 annotation to identify the issue it's related to

…tity

xhuberty · 2016-01-14T09:35:36Z

@Ocramius thanks a lot for your answer.

driq · 2016-05-09T13:45:48Z

Hi @xhuberty thank you for writing the patch and @Ocramius for reviewing it.

I'm running into the same issue. Just wanted to let you know that hoping to see this land in 2.6!

driq · 2016-05-09T14:03:32Z

FYI:

I noticed similar behaviour in Doctrine\ODM\MongoDB\UnitOfWork::commit($document = null, array $options = array()) on line 440.
Here, the whole state of the UnitOfWork is cleared even after only a single object is committed.

Ocramius · 2017-02-19T23:22:49Z

lib/Doctrine/ORM/UnitOfWork.php

+     * @param $class
+     * @param string $oid
+     */
+    private function clearScheduledForSynchronization($class, $oid)


Type hint needed

Ocramius · 2017-02-19T23:23:00Z

lib/Doctrine/ORM/UnitOfWork.php

+    }
+
+    /**
+     * @param $class


Type hint needed, return type needed

Ocramius · 2017-02-19T23:24:20Z

lib/Doctrine/ORM/UnitOfWork.php

+    }
+
+    /**
+     * @param null|object|array $entity


Oh dang, I really hate this sort of API signature :-(

Ocramius · 2017-02-19T23:24:41Z

lib/Doctrine/ORM/UnitOfWork.php

+    /**
+     * @param null|object|array $entity
+     */
+    private function postCommitClear($entity = null)


Parameter is not optional

Ocramius · 2017-02-19T23:25:00Z

lib/Doctrine/ORM/UnitOfWork.php

        $this->orphanRemovals = array();
+
+        if (null === $entity) {
+            $this->entityChangeSets = $this->scheduledForSynchronization = array();


Can use short array notation

Ocramius · 2017-02-19T23:25:48Z

lib/Doctrine/ORM/UnitOfWork.php

+            $oid = spl_object_hash($object);
+            $class = $this->em->getClassMetadata(get_class($object));
+            $this->clearEntityChangeSet($oid);
+            $this->clearScheduledForSynchronization($class, $oid);


Why isn't this method call internally calling $this->clearEntityChangeSet()?

Ocramius · 2017-02-19T23:26:59Z

tests/Doctrine/Tests/ORM/UnitOfWorkTest.php

+        $this->_unitOfWork->persist($entity2);
+
+        $this->_unitOfWork->commit($entity1);
+        $this->assertCount(1, $this->_unitOfWork->getEntityChangeSet($entity2));


Why is a count used here, instead of simply comparing the changeset? After all, we're checking for a single entry, so a count() operation doesn't seem to make sense to me

Ocramius · 2017-02-19T23:27:20Z

tests/Doctrine/Tests/ORM/UnitOfWorkTest.php

+
+        $this->_unitOfWork->commit([$entity1,$entity2]);
+        $this->assertCount(1, $this->_unitOfWork->getEntityChangeSet($entity3));
+


whitespace to be removed here

Ocramius · 2017-02-19T23:27:40Z

tests/Doctrine/Tests/ORM/UnitOfWorkTest.php

+        $this->_unitOfWork->persist($entity3);
+
+        $this->_unitOfWork->commit([$entity1,$entity2]);
+        $this->assertCount(1, $this->_unitOfWork->getEntityChangeSet($entity3));


Should probably check that there's no changeset for $entity1 and $entity2 here.

Ocramius · 2017-02-19T23:28:00Z

tests/Doctrine/Tests/ORM/UnitOfWorkTest.php

+        $entity3->setData('thedata');
+        $this->_unitOfWork->persist($entity3);
+
+        $this->_unitOfWork->commit([$entity1,$entity2]);


Why committing $entity1 if it was already committed? (also, space after ,)

Ocramius · 2017-08-18T07:31:21Z

Manually rebased, cleaned up some details and split the tests and packaged for master. Thanks @xhuberty!

…tanding of the code

…s for the tracking policy changeset clearing

…e reverted to `public`

xhuberty force-pushed the notifyTrackingPolicy branch from 458f64d to 6d50a66 Compare December 29, 2015 15:20

Ocramius added the Improvement label Dec 29, 2015

Ocramius added this to the 2.6.0 milestone Dec 29, 2015

Ocramius reviewed Dec 29, 2015
View reviewed changes

xhuberty force-pushed the notifyTrackingPolicy branch from 6d50a66 to f0d15df Compare December 29, 2015 17:10

Ocramius reviewed Dec 29, 2015
View reviewed changes

Ocramius added the Missing Tests label Dec 29, 2015

Ocramius removed this from the 2.6.0 milestone Dec 29, 2015

Ocramius reviewed Dec 30, 2015
View reviewed changes

xhuberty force-pushed the notifyTrackingPolicy branch from 1811e86 to a91b5bb Compare December 31, 2015 14:02

stloyd reviewed Jan 5, 2016
View reviewed changes

xhuberty force-pushed the notifyTrackingPolicy branch from a91b5bb to f09689c Compare January 5, 2016 10:53

xhuberty force-pushed the notifyTrackingPolicy branch from 1073571 to 1fda01d Compare January 8, 2016 10:36

Ocramius added this to the 2.6.0 milestone Jan 11, 2016

Ocramius removed the Missing Tests label Jan 11, 2016

Ocramius reviewed Jan 11, 2016
View reviewed changes

Fix issue when using notify tracking policy with multiple flush on en…

ed3a7d3

…tity

xhuberty force-pushed the notifyTrackingPolicy branch from 1fda01d to ed3a7d3 Compare January 12, 2016 14:01

lcobucci assigned Ocramius Feb 19, 2017

lcobucci added the Requires merge/review label Feb 19, 2017

Ocramius suggested changes Feb 19, 2017

View reviewed changes

Ocramius assigned lcobucci and xhuberty and unassigned Ocramius Feb 19, 2017

Ocramius removed the Requires merge/review label Feb 19, 2017

Ocramius changed the title ~~Fix issue when using notify tracking policy with multiple flush on entities~~ Fix performance issue when using notify tracking policy and calling flush($entity) multiple times Aug 18, 2017

Ocramius added a commit that referenced this pull request Aug 18, 2017

#5579 cleaning up test case, adding assertions critical to the unders…

df1577d

…tanding of the code

Ocramius added a commit that referenced this pull request Aug 18, 2017

#5579 isolating multi-entity-commit and single-entity-commit scenario…

9707701

…s for the tracking policy changeset clearing

Ocramius added a commit that referenced this pull request Aug 18, 2017

#5579 cleaning up postCommitClear implementation

2921f06

Ocramius added a commit that referenced this pull request Aug 18, 2017

#5579 s/clear/cleanup

9d7be0f

Ocramius added a commit that referenced this pull request Aug 18, 2017

#5579 correcting visibility of clearEntityChangeSet, which should b…

ff15a2b

…e reverted to `public`

Ocramius added a commit that referenced this pull request Aug 18, 2017

#5579 correcting assertion static/instance method usage

1bf8465

Ocramius added a commit that referenced this pull request Aug 18, 2017

#5579 completely removing a changeset when cleaning up

496c22d

Ocramius closed this in 60e29b4 Aug 18, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix performance issue when using notify tracking policy and calling `flush($entity)` multiple times #5579

Fix performance issue when using notify tracking policy and calling `flush($entity)` multiple times #5579

xhuberty commented Dec 29, 2015

Ocramius commented Dec 29, 2015

Ocramius Dec 29, 2015

xhuberty commented Dec 29, 2015

Ocramius Dec 29, 2015

xhuberty Dec 30, 2015

Ocramius commented Dec 29, 2015

xhuberty commented Dec 30, 2015

xhuberty commented Dec 30, 2015

Ocramius commented Dec 30, 2015

Ocramius Dec 30, 2015

xhuberty commented Jan 5, 2016

stloyd Jan 5, 2016

xhuberty commented Jan 5, 2016

xhuberty commented Jan 11, 2016

Ocramius commented Jan 11, 2016

Ocramius Jan 11, 2016

xhuberty commented Jan 14, 2016

driq commented May 9, 2016

driq commented May 9, 2016

Ocramius Feb 19, 2017

Ocramius Feb 19, 2017

Ocramius Feb 19, 2017

Ocramius Feb 19, 2017

Ocramius Feb 19, 2017

Ocramius Feb 19, 2017

Ocramius Feb 19, 2017

Ocramius Feb 19, 2017

Ocramius Feb 19, 2017

Ocramius Feb 19, 2017

Ocramius commented Aug 18, 2017


		$this->_unitOfWork->commit([$entity1,$entity2]);
		$this->assertCount(1, $this->_unitOfWork->getEntityChangeSet($entity3));

Fix performance issue when using notify tracking policy and calling flush($entity) multiple times #5579

Fix performance issue when using notify tracking policy and calling flush($entity) multiple times #5579

Conversation

xhuberty commented Dec 29, 2015

Ocramius commented Dec 29, 2015

Choose a reason for hiding this comment

xhuberty commented Dec 29, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ocramius commented Dec 29, 2015

xhuberty commented Dec 30, 2015

xhuberty commented Dec 30, 2015

Ocramius commented Dec 30, 2015

Choose a reason for hiding this comment

xhuberty commented Jan 5, 2016

Choose a reason for hiding this comment

xhuberty commented Jan 5, 2016

xhuberty commented Jan 11, 2016

Ocramius commented Jan 11, 2016

Choose a reason for hiding this comment

xhuberty commented Jan 14, 2016

driq commented May 9, 2016

driq commented May 9, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ocramius commented Aug 18, 2017

Fix performance issue when using notify tracking policy and calling `flush($entity)` multiple times #5579

Fix performance issue when using notify tracking policy and calling `flush($entity)` multiple times #5579