Skip to content

Conversation

@LunkLoafGrumble
Copy link
Contributor

Q A
Bug fix? yes
New feature? no
Tickets N/A
License MIT

This pull request will fix an issue for large objects (for example entities with a large relations) with in the FingerprintCalculator

While working with liveComponents I noticed my project getting slower and slower. After some digging I found out that in the calculateFingerprint function the data is serialized. When an entity with relations passes by it will serialize the entity and its child objects. This takes a considerable amount of time and is not needed.

@kevinmade
Copy link

I'm having the same issue.

Copy link
Member

@kbond kbond left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to me!

@weaverryan
Copy link
Member

Great catch on this!

These fingerprints need to be a deterministic string that represents "the input passed into a component" -

$fingerprint = $this->fingerprintCalculator->calculateFingerprint($mounted->getInputProps());
- and they need to always return the same string for the same input.

If you pass an object (e.g. a Post entity) into a component, the spl_object_id() of that might be different the next time the component is re-rendered via an Ajax call, right?

I'm open to ideas on how to fix this. In practice, if we added some special code for entity objects that, if persisted, used their id instead of serializing them, that might solve things. For example, if one of the items in $data is a Post entity, then instead of serializing that or spl_object_id(), we use App\Entity\Post:{THE_ID}.

@kbond
Copy link
Member

kbond commented Mar 8, 2023

These fingerprints need to be a deterministic string that represents

Oops, didn't realize this. Could spl_object_hash() be used? I don't know if this would still have a perf problem.

@weaverryan
Copy link
Member

I think spl_object_hash() has the same problem - not 100% certain on that, but pretty sure.

@kbond
Copy link
Member

kbond commented Mar 8, 2023

You're right. From php.net:

When an object is destroyed, its hash may be reused for other objects.

@kbond
Copy link
Member

kbond commented Mar 8, 2023

These fingerprints need to be a deterministic string that represents "the input passed into a component" -

We should have a test to prove this behaviour.

@kevinmade
Copy link

Perhaps it's an idea to add if (\is_object($value) && method_exists($value, 'getId')) { return $value->getId(); } instead of \is_object($value).

@LunkLoafGrumble LunkLoafGrumble requested a review from kbond March 9, 2023 08:31
Copy link
Member

@kbond kbond left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is probably the best solution.

@LunkLoafGrumble does this method improve the speed in your app?

Could you add a basic integration test for FingerprintCalculator that proves the generated fingerprint is the same two different object instances that are equal?

@LunkLoafGrumble
Copy link
Contributor Author

Using objectNormalizer greatly improves the performance over just serializing the object.

Copy link
Member

@kbond kbond left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few remaining minor things, then good to go by me!

@weaverryan
Copy link
Member

Let's give this version a try - thanks @LunkLoafGrumble!

@weaverryan
Copy link
Member

FYI - I'm reverting this in #832. But I'm hoping that it's not a problem anymore because usually zero LiveProps are used to calculate the fingerprint. But please tell me if you still have issues.

weaverryan added a commit that referenced this pull request May 1, 2023
…alculator (weaverryan)

This PR was merged into the 2.x branch.

Discussion
----------

[Live] Reverting back to simpler serialize for fingerprint calculator

| Q             | A
| ------------- | ---
| Bug fix?      | yes
| New feature?  | no
| Tickets       | Fix #831
| License       | MIT

This reverts #725. The problem is that using the serializer opens all kinds of cans of worms, including circular reference problems.

So, do we still have the big performance problem of #725? I'm not sure. We now only calculate the fingerprint based on LiveProps with `updateFromParent`.

Commits
-------

18fb8a0 [Live] Reverting back to simpler serialize for fingerprint calculator
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants