Optimize Object Hydration performance on large result sets

[tucksaun]

Hi folks!

While assessing a client's project performance I found some optimizations regarding the ORM hydration process that improves speed when working with large result sets and collections.

Expand to get some context on how we reached this.

The context is about online carts on a B2B e-commerce platform with a high number of items and complex personalization logic. We reached the point in the optimization process where most of the time was spent in the ORM (after serialization) when fetching a cart.

This makes sense because of the volume and the logic at stake.
Denormalization or caching does not make sense for us because of the low read/write ratio and the business logic involved so we first went with the 2-step hydration technic.

While analyzing profiling results we more or less came to the same conclusion as #8390: the hydration process converts data over and over again even if the entity is already hydrated>

But after trying to work around performance for specific DBAL types that were costly to convert (namely Symfony's UUID and Dunglas's JSON document) and digging deeper into profiles I concluded something could be done directly in the Hydrator for the benefit of everyone instead of writing our specific optimized hydrator:

This PR acts according to one consideration: most parts of the hydration process are not slow per se but are repeated for every row and column. By applying some small optimizations, moving some code before the first iteration, or some code at the very last moment, we can improve the overall process.
The more rows and columns, the more the process is accelerated.

For examples:

• type conversion can be costly but only identifiers are required to determine if the entity is already hydrated or not: the actual data can be converted later on in the process only if needed using only the columns required for the current entity (not the data for other entities in relations);
• we can prepare the mapping information in a way that prevents repeating some part of the process thousands of times;
• and we can avoid using the reflection to retrieve relations properties if they are available in another mapping.

With this patch, I manage to reach the following results:

Metric	Before	After	Notes
->getQuery()->getResult()	726.39 ms	358.67 ms	-51%, best of 20 iterations, query result cache enabled, 11 826 entities created
Doctrine\ORM\PersistentCollection::*	164 634	77 345
TypedNoDefaultReflectionProperty::getValue	162 770	125 669
Doctrine\ORM\Internal\Hydration\AbstractHydrator::resultSetMapping	441 742	19 745
Doctrine\ORM\Internal\Hydration\AbstractHydrator::hydrateColumnInfo	946 970	337
Doctrine\DBAL\Types\IntegerType::convertToPHPValue	406 982	279 366
Doctrine\DBAL\Types\StringType::convertToPHPValue	181 207	63 473
Doctrine\DBAL\Types\BooleanType::convertToPHPValue	75 163	7 865
Doctrine\DBAL\Types\DateTimeImmutableType::convertToPHPValue	25 118	789
App\DBAL\Types\UuidType::convertToPHPValue	13 056	413

I hope we can manage to get this work so that everyone enjoys better performance 🤗
If you are willing to move in this direction, we can probably adapt ArrayHydrator the same way.

[beberlei]

Hey @tucksaun, this looks like fantastic work! Thank you! It will take some time to review the PR through as its touching essentially everything about hydrators, so I am not sure when we will get to it at this time.

[sips-richard]

I found this PR in relation to an issue we've been having when hydrating a graph with a couple left joins. Specifically that Doctrine calls convertToPHPValue() on all the NULL values for each column in any empty left join records, prior to hydrating the entities and then discarding those empty records. We noticed this because we have a custom data type that throws an exception if it encounters a NULL (the fix for now was to eliminate the check).

But I was wondering whether it is necessary for the calls to convertToPHPValue() up front when ultimately the record ID might be NULL and the empty data set gets thrown in the trash, or could the code could be reworked to only convert the values for columns belonging to entities that will actually be hydrated?

[tucksaun]

Hey @tucksaun, this looks like fantastic work! Thank you! It will take some time to review the PR through as its touching essentially everything about hydrators, so I am not sure when we will get to it at this time.

@beberlei sure, I understand as this is quite a change.
If it can help you: I tried to keep a "logical" order in commits so that maybe it is easier to follow.
Also, feel free to ping me if you want to chat about it if you want.

I will try to maintain this PR as up-to-date as possible in the meantime.

[tucksaun]

I found this PR in relation to an issue we've been having when hydrating a graph with a couple left joins. Specifically that Doctrine calls convertToPHPValue() on all the NULL values for each column in any empty left join records, prior to hydrating the entities and then discarding those empty records. We noticed this because we have a custom data type that throws an exception if it encounters a NULL (the fix for now was to eliminate the check).

But I was wondering whether it is necessary for the calls to convertToPHPValue() up front when ultimately the record ID might be NULL and the empty data set gets thrown in the trash, or could the code could be reworked to only convert the values for columns belonging to entities that will actually be hydrated?

@sips-richard I would say that the hydrator has to maintain the call to convertToPHPValue because the logic of what is done with a NULL value in the database might be reflected differently in PHP depending on the needs and the type implementation.
But if I got your problem correctly, the number of calls might be lower with this patch.

[greg0ire]

Hi! git rebase --exec vendor/bin/phpcbf should fix some (but not all) cs issues :)

[tucksaun]

@greg0ire I though I already fixed them all and only casually rebased this PR today so thank you for the heads up about he CS issues.
They should be fixed now.

[derrabus]

Don't suppress Psalm errors via comments. We usually configure this in psalm.xml.dist.

[tucksaun]

@derrabus I don't manage to suppress it for this specific method inside psalm.xml.
And to suppress it for the whole file looks a bit too wide to me.

Do you have any recommendations?
Or should I add this to the baseline?
Or does it mean this is a legitimate use case to you?

[derrabus]

Suggested change

	$resultKey = isset($resultSetMapping->indexByMap['scalars'])
	? $row[$resultSetMapping->indexByMap['scalars']]
	: $this->resultCounter - 1;
	$resultKey = $resultSetMapping->indexByMap['scalars'] ?? $this->resultCounter - 1;

[tucksaun]

hum does not look correct to me as we are accessing $row, not only $resultSetMapping->indexByMap ? 🤔

[derrabus]

Suggested change

	protected function gatherRowIDs(array $data, array &$id, array &$nonemptyComponents)
	private function gatherRowIDs(array $data, array &$id, array &$nonemptyComponents): array

[derrabus]

Suggested change

	protected function gatherRowDataFromScalarsMapping(array $data)
	private function gatherRowDataFromScalarsMapping(array $data): array

[derrabus]

Suggested change

	protected function gatherRowDataFromNewObjectsMapping(array $data)
	private function gatherRowDataFromNewObjectsMapping(array $data): array

[sips-richard]

I found this PR in relation to an issue we've been having when hydrating a graph with a couple left joins. Specifically that Doctrine calls convertToPHPValue() on all the NULL values for each column in any empty left join records, prior to hydrating the entities and then discarding those empty records. We noticed this because we have a custom data type that throws an exception if it encounters a NULL (the fix for now was to eliminate the check).
But I was wondering whether it is necessary for the calls to convertToPHPValue() up front when ultimately the record ID might be NULL and the empty data set gets thrown in the trash, or could the code could be reworked to only convert the values for columns belonging to entities that will actually be hydrated?

@sips-richard I would say that the hydrator has to maintain the call to convertToPHPValue because the logic of what is done with a NULL value in the database might be reflected differently in PHP depending on the needs and the type implementation. But if I got your problem correctly, the number of calls might be lower with this patch.

@tucksaun Absolutely this will speed up our code, but I will try to clarify on our problem scenario because I think a change in the internals might be beneficial here.

Our ORM query included several left joins. Internally Doctrine was passing all the null values for any non-existent left join entities to the convertToPHPValue() method and only determining afterward that there was no relationship and then throwing these values away.

The convertToPHPValue method on the type for one of our columns did not permit null values and would throw an exception (because this would never happen in normal operation), but because Doctrine is hydrating null values for the empty left joins it was.

My question therefore is: can the logic be reworked in such a way that Doctrine could determine whether or not the ID columns represent a joined entity or an empty left join up front, prior to any hydration.

It would save a lot of function calls where there are empty left joins for a start.

[NoiseByNorthwest]

It looks very close to the issue I've described here #8390
And I've shared a custom hydrator which fixes the useless "repeated rows hydration" with a simple caching logic.

Does my workaround make sense ? Should it be implemented into ORM ?

[github-actions]

There hasn't been any activity on this pull request in the past 90 days, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 7 days.
If you want to continue working on it, please leave a comment.

[github-actions]

This pull request was closed due to inactivity.