-
Notifications
You must be signed in to change notification settings - Fork 14
[WIP] feat: Improve performances #1595
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
We used to hydrate any document with the relationships existing in the provided schema, even though the relationship does not exist on the document. We now hydrate only if the relationship is set in the document. Note we can still force the hydratation through a `forceHydration` option, to ease migrations on apps with many relations. BREAKING CHANGE: the relationship hydration is made only if the relationship exists in the document, so the developer should not assume a `document.relationshipName` is always defined, anymore. As an alternative, it is now possible to pass `forceHydration` on cozy-client options to ease migration. However, please not this has performance impact, as it forces extra-check on store queries evaluation.
ee55d23
to
5807df1
Compare
|
||
return false | ||
}) | ||
for (const document of documentsToPersist) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it really better? OK you don't call a method if not needed, but now you loop twice
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is actually significantly better. After a big query retrieving 25K docs, the persistVirtualDocuments
takes:
- Before: 609ms
- After: 20ms
I wonder if this is because of the event loop overwhelmed by too many calls
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh shit, I didn't saw that the persistVirtualDocument
was async... This is why.
nit: You can now remove the check within the persistVirtualDocument
method since should only call it with the docs to persist ^^.
if (normalizedDoc.rev) { | ||
delete normalizedDoc.rev | ||
if (doc.rev) { | ||
delete doc.rev |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
setting to undefined is way more performant than calling delet(https://jsperf.app/delete-vs-undefined-vs-null/10 I don't like the void 0
stuff)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good to know! I did the change in another PR: 2b89a63
a2bc4d0
to
9e79d2c
Compare
This PR has been replaced by: #1596 |
db2aabf
to
b0714ba
Compare
No description provided.