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

fix(quaint/DA): handle JSON parsing engines side so that we can correctly handle i64s #4883

Merged
merged 9 commits into from
May 24, 2024

Conversation

Druue
Copy link
Contributor

@Druue Druue commented May 23, 2024

unexclude common_types test for pg, neon, and PS

  • (D1 not relevant as it doesn't support JSON)

related prisma/prisma#23926

unexclude common_types test for pg, neon, and PS
@Druue Druue requested a review from a team as a code owner May 23, 2024 13:52
@Druue Druue requested review from Weakky and removed request for a team May 23, 2024 13:52
Copy link

codspeed-hq bot commented May 23, 2024

CodSpeed Performance Report

Merging #4883 will not alter performance

Comparing fix/23926 (c177bfd) with main (b4fe3e6)

Summary

✅ 11 untouched benchmarks

Copy link
Contributor

github-actions bot commented May 23, 2024

WASM Query Engine file Size

Engine This PR Base branch Diff
Postgres 2.153MiB 2.153MiB 387.000B
Postgres (gzip) 846.090KiB 846.061KiB 30.000B
Mysql 2.120MiB 2.119MiB 386.000B
Mysql (gzip) 831.839KiB 831.749KiB 93.000B
Sqlite 2.015MiB 2.015MiB 183.000B
Sqlite (gzip) 793.165KiB 793.081KiB 86.000B

@Druue Druue changed the title tech(quaint/DA): handle JSON parsing engines side so that we can correctly handle i64s fix(quaint/DA): handle JSON parsing engines side so that we can correctly handle i64s May 23, 2024
@Druue Druue added this to the 5.15.0 milestone May 23, 2024
@SevInf
Copy link
Contributor

SevInf commented May 23, 2024

I think we can also get rid of this block in driver-adapters crate

Copy link
Contributor

✅ WASM query-engine performance won't change substantially (0.987x)

Full benchmark report
DATABASE_URL="postgresql://postgres:postgres@localhost:5432/bench?schema=imdb_bench&sslmode=disable" \
node --experimental-wasm-modules query-engine/driver-adapters/executor/dist/bench.mjs
"
"
"
"
"
"
"
"
"
"
"
"
"
"
"
"
"
"
"
"
"
"
"
"
"
"
"
"
"
"
"
"
"
"
"
"
cpu: AMD EPYC 7763 64-Core Processor
runtime: node v18.20.2 (x64-linux)

benchmark                   time (avg)             (min … max)       p75       p99      p999
-------------------------------------------------------------- -----------------------------
• movies.findMany() (all - ~50K)
-------------------------------------------------------------- -----------------------------
Web Assembly: Baseline     373 ms/iter       (370 ms … 387 ms)    373 ms    387 ms    387 ms
Web Assembly: Latest       461 ms/iter       (456 ms … 466 ms)    465 ms    466 ms    466 ms
Web Assembly: Current      453 ms/iter       (448 ms … 458 ms)    457 ms    458 ms    458 ms
Node API: Current          203 ms/iter       (194 ms … 213 ms)    210 ms    213 ms    213 ms

summary for movies.findMany() (all - ~50K)
  Web Assembly: Current
   2.23x slower than Node API: Current
   1.21x slower than Web Assembly: Baseline
   1.02x faster than Web Assembly: Latest

• movies.findMany({ take: 2000 })
-------------------------------------------------------------- -----------------------------
Web Assembly: Baseline  14'866 µs/iter (14'420 µs … 19'416 µs) 14'724 µs 19'416 µs 19'416 µs
Web Assembly: Latest    18'471 µs/iter (17'988 µs … 21'629 µs) 18'605 µs 21'629 µs 21'629 µs
Web Assembly: Current   18'264 µs/iter (18'121 µs … 18'661 µs) 18'291 µs 18'661 µs 18'661 µs
Node API: Current        8'045 µs/iter   (7'853 µs … 8'531 µs)  8'123 µs  8'531 µs  8'531 µs

summary for movies.findMany({ take: 2000 })
  Web Assembly: Current
   2.27x slower than Node API: Current
   1.23x slower than Web Assembly: Baseline
   1.01x faster than Web Assembly: Latest

• movies.findMany({ where: {...}, take: 2000 })
-------------------------------------------------------------- -----------------------------
Web Assembly: Baseline   2'291 µs/iter   (2'204 µs … 3'321 µs)  2'283 µs  3'049 µs  3'321 µs
Web Assembly: Latest     2'881 µs/iter   (2'780 µs … 3'680 µs)  2'867 µs  3'521 µs  3'680 µs
Web Assembly: Current    2'841 µs/iter   (2'752 µs … 3'287 µs)  2'837 µs  3'201 µs  3'287 µs
Node API: Current        1'433 µs/iter   (1'329 µs … 1'828 µs)  1'434 µs  1'682 µs  1'828 µs

summary for movies.findMany({ where: {...}, take: 2000 })
  Web Assembly: Current
   1.98x slower than Node API: Current
   1.24x slower than Web Assembly: Baseline
   1.01x faster than Web Assembly: Latest

• movies.findMany({ include: { cast: true } take: 2000 }) (m2m)
-------------------------------------------------------------- -----------------------------
Web Assembly: Baseline     574 ms/iter       (568 ms … 589 ms)    581 ms    589 ms    589 ms
Web Assembly: Latest       789 ms/iter       (784 ms … 798 ms)    793 ms    798 ms    798 ms
Web Assembly: Current      776 ms/iter       (772 ms … 786 ms)    778 ms    786 ms    786 ms
Node API: Current          482 ms/iter       (479 ms … 489 ms)    483 ms    489 ms    489 ms

summary for movies.findMany({ include: { cast: true } take: 2000 }) (m2m)
  Web Assembly: Current
   1.61x slower than Node API: Current
   1.35x slower than Web Assembly: Baseline
   1.02x faster than Web Assembly: Latest

• movies.findMany({ where: {...}, include: { cast: true } take: 2000 }) (m2m)
-------------------------------------------------------------- -----------------------------
Web Assembly: Baseline  79'233 µs/iter (78'856 µs … 79'569 µs) 79'427 µs 79'569 µs 79'569 µs
Web Assembly: Latest       111 ms/iter       (110 ms … 111 ms)    111 ms    111 ms    111 ms
Web Assembly: Current      109 ms/iter       (109 ms … 110 ms)    109 ms    110 ms    110 ms
Node API: Current       65'109 µs/iter (62'956 µs … 66'500 µs) 66'400 µs 66'500 µs 66'500 µs

summary for movies.findMany({ where: {...}, include: { cast: true } take: 2000 }) (m2m)
  Web Assembly: Current
   1.68x slower than Node API: Current
   1.38x slower than Web Assembly: Baseline
   1.01x faster than Web Assembly: Latest

• movies.findMany({ take: 2000, include: { cast: { include: { person: true } } } })
-------------------------------------------------------------- -----------------------------
Web Assembly: Baseline   1'023 ms/iter   (1'009 ms … 1'045 ms)  1'038 ms  1'045 ms  1'045 ms
Web Assembly: Latest     1'326 ms/iter   (1'318 ms … 1'348 ms)  1'337 ms  1'348 ms  1'348 ms
Web Assembly: Current    1'304 ms/iter   (1'292 ms … 1'319 ms)  1'313 ms  1'319 ms  1'319 ms
Node API: Current          910 ms/iter       (873 ms … 937 ms)    935 ms    937 ms    937 ms

summary for movies.findMany({ take: 2000, include: { cast: { include: { person: true } } } })
  Web Assembly: Current
   1.43x slower than Node API: Current
   1.28x slower than Web Assembly: Baseline
   1.02x faster than Web Assembly: Latest

• movie.findMany({ where: { ... }, take: 2000, include: { cast: { include: { person: true } } } })
-------------------------------------------------------------- -----------------------------
Web Assembly: Baseline     145 ms/iter       (144 ms … 146 ms)    145 ms    146 ms    146 ms
Web Assembly: Latest       190 ms/iter       (189 ms … 193 ms)    193 ms    193 ms    193 ms
Web Assembly: Current      186 ms/iter       (185 ms … 187 ms)    187 ms    187 ms    187 ms
Node API: Current          113 ms/iter       (110 ms … 115 ms)    114 ms    115 ms    115 ms

summary for movie.findMany({ where: { ... }, take: 2000, include: { cast: { include: { person: true } } } })
  Web Assembly: Current
   1.65x slower than Node API: Current
   1.29x slower than Web Assembly: Baseline
   1.02x faster than Web Assembly: Latest

• movie.findMany({ where: { reviews: { author: { ... } }, take: 100 }) (to-many -> to-one)
-------------------------------------------------------------- -----------------------------
Web Assembly: Baseline   1'058 µs/iter     (998 µs … 1'755 µs)  1'049 µs  1'610 µs  1'755 µs
Web Assembly: Latest     1'391 µs/iter   (1'323 µs … 2'190 µs)  1'382 µs  2'018 µs  2'190 µs
Web Assembly: Current    1'378 µs/iter   (1'298 µs … 2'328 µs)  1'368 µs  2'260 µs  2'328 µs
Node API: Current          768 µs/iter     (708 µs … 1'356 µs)    787 µs    867 µs  1'356 µs

summary for movie.findMany({ where: { reviews: { author: { ... } }, take: 100 }) (to-many -> to-one)
  Web Assembly: Current
   1.79x slower than Node API: Current
   1.3x slower than Web Assembly: Baseline
   1.01x faster than Web Assembly: Latest

• movie.findMany({ where: { cast: { person: { ... } }, take: 100 }) (m2m -> to-one)
-------------------------------------------------------------- -----------------------------
Web Assembly: Baseline   1'023 µs/iter     (980 µs … 1'704 µs)  1'026 µs  1'423 µs  1'704 µs
Web Assembly: Latest     1'370 µs/iter   (1'305 µs … 2'192 µs)  1'371 µs  2'094 µs  2'192 µs
Web Assembly: Current    1'378 µs/iter   (1'304 µs … 2'184 µs)  1'368 µs  2'074 µs  2'184 µs
Node API: Current          786 µs/iter     (718 µs … 1'298 µs)    801 µs    895 µs  1'298 µs

summary for movie.findMany({ where: { cast: { person: { ... } }, take: 100 }) (m2m -> to-one)
  Web Assembly: Current
   1.75x slower than Node API: Current
   1.35x slower than Web Assembly: Baseline
   1.01x slower than Web Assembly: Latest

After changes in 374fc28

@Druue Druue requested a review from SevInf May 23, 2024 14:32
@@ -59,6 +59,7 @@ jobs:

- name: Run benchmarks
id: bench
if: false
Copy link
Contributor

Choose a reason for hiding this comment

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

Note for posteriority: temprarily disabled, as we can't run benchmarks against older version of WASM engine without using driver adapters too: https://github.com/prisma/team-orm/issues/1152

@Druue Druue merged commit f742678 into main May 24, 2024
182 checks passed
@Druue Druue deleted the fix/23926 branch May 24, 2024 13:06
SevInf added a commit that referenced this pull request May 24, 2024
…an correctly handle `i64`s (#4883)"

This reverts commit f742678.

Temporary reverting the change. It is interdependent with
prisma/prisma#24269 and neither PR works correctly without the other.
The plan was to let engines CI to temporarily go red and fix it
immediately by merging client PR. However, engine release pipeline is
broken for unrelated reason and this is not possible. Just to limit
amount of broken things in progress, we are reverting original PR. It is
expected we restore it with no changes once release pipeline is fixed.
SevInf pushed a commit that referenced this pull request May 24, 2024
…an correctly handle `i64`s (#4883)" (#4886)

This reverts commit f742678.

Temporary reverting the change. It is interdependent with
prisma/prisma#24269 and neither PR works correctly without the other.
The plan was to let engines CI to temporarily go red and fix it
immediately by merging client PR. However, engine release pipeline is
broken for unrelated reason and this is not possible. Just to limit
amount of broken things in progress, we are reverting original PR. It is
expected we restore it with no changes once release pipeline is fixed.
SevInf pushed a commit that referenced this pull request May 28, 2024
…ctly handle `i64`s (#4883)

* One half of the fix for: prisma/prisma#23926
* Unexcludes pg, neon, and PS for the through_relations::common_types test

* Instead of receiving pre-handled JSON by DAs, we now expect strings and will perform JSON parsing in Quaint.
* Removed special handling for "$__prisma_null" due to the aforementioned

* Temporarily disable wasm-benchmarks due to breaking change in engines <-> DA contract. To be re-enabled and re-evaluated in a follow-up PR per convo with @SevInf

---------

Co-authored-by: Serhii Tatarintsev <tatarintsev@prisma.io>
@SevInf SevInf mentioned this pull request May 28, 2024
SevInf pushed a commit that referenced this pull request May 29, 2024
* One half of the fix for: prisma/prisma#23926
* Unexcludes pg, neon, and PS for the through_relations::common_types test

* Instead of receiving pre-handled JSON by DAs, we now expect strings and will perform JSON parsing in Quaint.
* Removed special handling for "$__prisma_null" due to the aforementioned

* Temporarily disable wasm-benchmarks due to breaking change in engines <-> DA contract. To be re-enabled and re-evaluated in a follow-up PR per convo with @SevInf

---------

Co-authored-by: Sophie <29753584+Druue@users.noreply.github.com>
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.

2 participants