Skip to content
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

wip: make json serialization faster #4753

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

Weakky
Copy link
Member

@Weakky Weakky commented Mar 5, 2024

Overview

Related to prisma/prisma#22596

Reduce JSON serialization overhead by:

  • Avoiding JSON serialization/deserialization when possible
  • Reducing allocations & de-allocations
    • JSON relations values are now coerced in-place instead of re-allocating PrismaValues
    • Virtual selections were refactored to avoid unnecessary allocations in the serializer

Notes:

  • The connector is now responsible for:

    • Data coercions
    • In-memory record processing (ordering, pagination, distinct, etc..)
    • JSON keys order preservation
  • The serializer is now only responsible, for join queries to discriminate values for the JSON protocol.

JSON keys order preservation is still costing us quite a lot of time, because it forces us to re-allocate all maps. I'm planning to open discussions as to whether we need to keep guaranteeing that the order of keys must be preserved.

Copy link
Contributor

github-actions bot commented Mar 5, 2024

WASM Size

Engine This PR Base branch Diff
Postgres 1.950MiB 2.068MiB -121.248KiB
Postgres (gzip) 769.415KiB 816.225KiB -46.811KiB
Mysql 1.917MiB 2.038MiB -123.311KiB
Mysql (gzip) 755.223KiB 802.603KiB -47.380KiB
Sqlite 1.816MiB 1.929MiB -115.772KiB
Sqlite (gzip) 716.861KiB 762.287KiB -45.426KiB

Copy link

codspeed-hq bot commented Mar 5, 2024

CodSpeed Performance Report

Merging #4753 will not alter performance

Comparing experiment/faster-json-serialization (91e6d58) with main (efd2449)

Summary

✅ 11 untouched benchmarks

@Weakky Weakky changed the title wip: broken experiment to make serialization faster wip: make json serialization faster Mar 11, 2024
@Weakky Weakky force-pushed the experiment/faster-json-serialization branch from 1ebfa03 to a268935 Compare March 13, 2024 10:09
@Weakky Weakky force-pushed the experiment/faster-json-serialization branch from a268935 to 456f88e Compare March 13, 2024 10:11
@semoal
Copy link

semoal commented Mar 13, 2024

JSON keys order preservation is still costing us quite a lot of time, because it forces us to re-allocate all maps. I'm planning to open discussions as to whether we need to keep guaranteeing that the order of keys must be preserved.

According to the JSON RFC specification 84 there’s no restriction on the order of the keys.

For example on Golang doesn’t care about the order of the keys and you always get a different ordering.:

json.Marshal([]byte, interface{}) 

@Weakky
Copy link
Member Author

Weakky commented Mar 15, 2024

Hey @semoal,

Thanks for your comment. I'm confused, you're the one who recently created an issue about having issues with the keys of json objects being out of order prisma/prisma#22479. Or did I misunderstand your issue?

I wholeheartedly agree with you that we should not have to preserve that order. But you and other users have reported that it's breaking your app. This is not to accuse you of anything, to be clear. But this suggests that, despite the RFC, doing this change now would break a bunch of user's apps. So we need to take this into account before making this change without a major update.

@semoal
Copy link

semoal commented Mar 15, 2024

Yes, I've opened the issue but I understood that we were wrong assuming that.
Right now, it is already a braking change since relationJoins, so I vote to always improve stability and performance over failures when making something.

@Weakky
Copy link
Member Author

Weakky commented Mar 15, 2024

Right now, it is already a breaking change since relationJoins

That's correct but this breaking change is currently behind a preview feature, which doesn't count as a "stable" breaking change. It's in fact the whole goal of preview features. We did plan on fixing that bug if it affected you, despite the RFC.

With that being said, thanks a lot for chiming in and clarifying your thoughts. This may make the decision of not preserving object keys order easier.

Have a great day 👍

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.

None yet

2 participants