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(adapter-d1) Use max_bind_value = 100 on Cloudflare D1 #4878

Merged
merged 14 commits into from
May 31, 2024

Conversation

jkomyno
Copy link
Contributor

@jkomyno jkomyno commented May 20, 2024

This PR:

In TypeScript, it also performs a minor clean-up of query-engine/driver-adapters/executor/src/testd.ts after the recent Mobile test connector addition in #4793.

@jkomyno jkomyno self-assigned this May 20, 2024
Copy link

codspeed-hq bot commented May 20, 2024

CodSpeed Performance Report

Merging #4878 will not alter performance

Comparing fix/d1-custom-max-bind-values (150a462) with main (bc6a413)

Summary

✅ 11 untouched benchmarks

Copy link
Contributor

github-actions bot commented May 20, 2024

WASM Query Engine file Size

Engine This PR Base branch Diff
Postgres 2.043MiB 2.042MiB 1.100KiB
Postgres (gzip) 813.868KiB 813.788KiB 82.000B
Mysql 2.013MiB 2.012MiB 1.070KiB
Mysql (gzip) 800.253KiB 800.072KiB 186.000B
Sqlite 1.914MiB 1.914MiB 25.000B
Sqlite (gzip) 762.167KiB 762.790KiB -638.000B

Copy link
Contributor

github-actions bot commented May 20, 2024

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

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     370 ms/iter       (368 ms … 376 ms)    370 ms    376 ms    376 ms
Web Assembly: Latest       472 ms/iter       (468 ms … 486 ms)    475 ms    486 ms    486 ms
Web Assembly: Current      467 ms/iter       (463 ms … 471 ms)    470 ms    471 ms    471 ms
Node API: Current          206 ms/iter       (199 ms … 213 ms)    211 ms    213 ms    213 ms

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

• movies.findMany({ take: 2000 })
-------------------------------------------------------------- -----------------------------
Web Assembly: Baseline  14'764 µs/iter (14'408 µs … 16'644 µs) 14'685 µs 16'644 µs 16'644 µs
Web Assembly: Latest    18'846 µs/iter (18'424 µs … 21'262 µs) 18'970 µs 21'262 µs 21'262 µs
Web Assembly: Current   18'598 µs/iter (18'457 µs … 18'787 µs) 18'664 µs 18'787 µs 18'787 µs
Node API: Current        8'463 µs/iter  (8'059 µs … 11'367 µs)  8'517 µs 11'367 µs 11'367 µs

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

• movies.findMany({ where: {...}, take: 2000 })
-------------------------------------------------------------- -----------------------------
Web Assembly: Baseline   2'310 µs/iter   (2'188 µs … 3'255 µs)  2'298 µs  3'056 µs  3'255 µs
Web Assembly: Latest     2'960 µs/iter   (2'827 µs … 4'770 µs)  2'935 µs  4'522 µs  4'770 µs
Web Assembly: Current    2'940 µs/iter   (2'804 µs … 5'073 µs)  2'905 µs  4'802 µs  5'073 µs
Node API: Current        1'388 µs/iter   (1'312 µs … 1'903 µs)  1'397 µs  1'641 µs  1'903 µs

summary for movies.findMany({ where: {...}, take: 2000 })
  Web Assembly: Current
   2.12x slower than Node API: Current
   1.27x 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       (566 ms … 594 ms)    588 ms    594 ms    594 ms
Web Assembly: Latest       787 ms/iter       (785 ms … 791 ms)    790 ms    791 ms    791 ms
Web Assembly: Current      795 ms/iter       (784 ms … 821 ms)    810 ms    821 ms    821 ms
Node API: Current          472 ms/iter       (469 ms … 477 ms)    473 ms    477 ms    477 ms

summary for movies.findMany({ include: { cast: true } take: 2000 }) (m2m)
  Web Assembly: Current
   1.69x slower than Node API: Current
   1.39x slower than Web Assembly: Baseline
   1.01x slower than Web Assembly: Latest

• movies.findMany({ where: {...}, include: { cast: true } take: 2000 }) (m2m)
-------------------------------------------------------------- -----------------------------
Web Assembly: Baseline  79'406 µs/iter (78'199 µs … 82'577 µs) 80'134 µs 82'577 µs 82'577 µs
Web Assembly: Latest       113 ms/iter       (112 ms … 113 ms)    113 ms    113 ms    113 ms
Web Assembly: Current      112 ms/iter       (111 ms … 113 ms)    112 ms    113 ms    113 ms
Node API: Current       63'115 µs/iter (62'364 µs … 64'107 µs) 63'436 µs 64'107 µs 64'107 µs

summary for movies.findMany({ where: {...}, include: { cast: true } take: 2000 }) (m2m)
  Web Assembly: Current
   1.77x slower than Node API: Current
   1.41x 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'000 ms/iter     (993 ms … 1'014 ms)  1'002 ms  1'014 ms  1'014 ms
Web Assembly: Latest     1'300 ms/iter   (1'293 ms … 1'317 ms)  1'304 ms  1'317 ms  1'317 ms
Web Assembly: Current    1'305 ms/iter   (1'290 ms … 1'327 ms)  1'320 ms  1'327 ms  1'327 ms
Node API: Current          900 ms/iter       (876 ms … 942 ms)    914 ms    942 ms    942 ms

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

• movie.findMany({ where: { ... }, take: 2000, include: { cast: { include: { person: true } } } })
-------------------------------------------------------------- -----------------------------
Web Assembly: Baseline     143 ms/iter       (141 ms … 145 ms)    143 ms    145 ms    145 ms
Web Assembly: Latest       186 ms/iter       (185 ms … 189 ms)    187 ms    189 ms    189 ms
Web Assembly: Current      185 ms/iter       (183 ms … 188 ms)    186 ms    188 ms    188 ms
Node API: Current          112 ms/iter       (110 ms … 113 ms)    113 ms    113 ms    113 ms

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

• movie.findMany({ where: { reviews: { author: { ... } }, take: 100 }) (to-many -> to-one)
-------------------------------------------------------------- -----------------------------
Web Assembly: Baseline   1'055 µs/iter     (975 µs … 1'748 µs)  1'046 µs  1'658 µs  1'748 µs
Web Assembly: Latest     1'458 µs/iter   (1'362 µs … 2'208 µs)  1'444 µs  2'114 µs  2'208 µs
Web Assembly: Current    1'415 µs/iter   (1'351 µs … 2'013 µs)  1'417 µs  1'788 µs  2'013 µs
Node API: Current          787 µs/iter     (714 µs … 1'250 µs)    805 µs    855 µs  1'250 µs

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

• movie.findMany({ where: { cast: { person: { ... } }, take: 100 }) (m2m -> to-one)
-------------------------------------------------------------- -----------------------------
Web Assembly: Baseline   1'080 µs/iter     (977 µs … 2'021 µs)  1'044 µs  1'962 µs  2'021 µs
Web Assembly: Latest     1'416 µs/iter   (1'333 µs … 2'212 µs)  1'412 µs  2'117 µs  2'212 µs
Web Assembly: Current    1'397 µs/iter   (1'332 µs … 2'059 µs)  1'399 µs  1'771 µs  2'059 µs
Node API: Current          808 µs/iter     (719 µs … 1'260 µs)    825 µs    997 µs  1'260 µs

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

After changes in e6a2517

@jkomyno jkomyno marked this pull request as ready for review May 20, 2024 16:07
@jkomyno jkomyno requested a review from a team as a code owner May 20, 2024 16:07
@jkomyno jkomyno requested review from laplab and Weakky and removed request for a team and laplab May 20, 2024 16:07
@@ -1,6 +1,6 @@
use query_engine_tests::*;

#[test_suite(schema(autoinc_id), capabilities(CreateMany, AutoIncrement), exclude(CockroachDb))]
#[test_suite(schema(autoinc_id), capabilities(AutoIncrement), exclude(CockroachDb))]
Copy link
Contributor

Choose a reason for hiding this comment

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

Random find, a couple of line below D1 is excluded, just wanted to let you know, would it be interesting to change that?

#[connector_test(exclude(CockroachDb, Sqlite("cfd1")))]

@Jolg42
Copy link
Contributor

Jolg42 commented May 22, 2024

Note: I posted on Slack about the unrelated neon failures https://prisma-company.slack.com/archives/C058VM009HT/p1716389660069659

@Jolg42
Copy link
Contributor

Jolg42 commented May 31, 2024

It looks like QE: driver-adapter integration tests tests are failing with

https://github.com/prisma/prisma-engines/actions/runs/9315409327/job/25641524922?pr=4878#step:10:1444
DTS Build start
Error: src/testd.ts(140,82): error TS2339: Property 'maxBindValues' does not exist on type 'ConnectionInfo'.

Error: error occured in dts build
    at Worker.<anonymous> (/home/runner/work/prisma-engines/prisma-engines/query-engine/driver-adapters/node_modules/.pnpm/[email protected][email protected]/node_modules/tsup/dist/index.js:2693:26)
    at Worker.emit (node:events:517:28)
    at MessagePort.<anonymous> (node:internal/worker:256:53)
    at [nodejs.internal.kHybridDispatch] (node:internal/event_target:786:20)
    at exports.emitMessage (node:internal/per_context/messageport:23:28)

@jkomyno
Copy link
Contributor Author

jkomyno commented May 31, 2024

It looks like QE: driver-adapter integration tests tests are failing with

https://github.com/prisma/prisma-engines/actions/runs/9315409327/job/25641524922?pr=4878#step:10:1444
DTS Build start
Error: src/testd.ts(140,82): error TS2339: Property 'maxBindValues' does not exist on type 'ConnectionInfo'.

Error: error occured in dts build
    at Worker.<anonymous> (/home/runner/work/prisma-engines/prisma-engines/query-engine/driver-adapters/node_modules/.pnpm/[email protected][email protected]/node_modules/tsup/dist/index.js:2693:26)
    at Worker.emit (node:events:517:28)
    at MessagePort.<anonymous> (node:internal/worker:256:53)
    at [nodejs.internal.kHybridDispatch] (node:internal/event_target:786:20)
    at exports.emitMessage (node:internal/per_context/messageport:23:28)

Yeah, this was due to maxBindValues being added only in prisma/prisma#24242, which this PR depends on. One must remember to add DRIVER_ADAPTERS_BRANCH=integration/fix-sqlite-d1-max-bind-values to any latest commit, in order for tests to pass.

@Jolg42
Copy link
Contributor

Jolg42 commented May 31, 2024

@jkomyno Looks like some tests are failing 👀
In [planetscale (wasm) 4/4](https://github.com/prisma/prisma-engines/actions/runs/9316221282/job/25644013246?pr=4878#logs) there are these logs
https://github.com/prisma/prisma-engines/actions/runs/9316221282/job/25644013246?pr=4878#step:11:4317

failures:
    writes::top_level_mutations::update::json_update::update_json_adv

test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 1705 filtered out; finished in 1.32s


--- TRY 6 STDERR:        query-engine-tests::query_engine_tests writes::top_level_mutations::update::json_update::update_json_adv ---
thread 'writes::top_level_mutations::update::json_update::update_json_adv' panicked at query-engine/connector-test-kit-rs/query-tests-setup/src/query_result.rs:72:13:
{"errors":[{"error":"KnownError { message: \"Inconsistent column data: Conversion failed: Failed to parse incoming json from a driver adapter\", meta: Object {\"message\": String(\"Conversion failed: Failed to parse incoming json from a driver adapter\")}, error_code: \"P2023\" }","user_facing_error":{"is_panic":false,"message":"Inconsistent column data: Conversion failed: Failed to parse incoming json from a driver adapter","meta":{"message":"Conversion failed: Failed to parse incoming json from a driver adapter"},"error_code":"P2023"}}]}
stack backtrace:
   0: rust_begin_unwind

@jkomyno
Copy link
Contributor Author

jkomyno commented May 31, 2024

12 failing tests. It used to be neon, now it's planetscale and pg 🥲
This is likely not related to this PR. I'm verifying what's going on

@jkomyno
Copy link
Contributor Author

jkomyno commented May 31, 2024

I've updated both branches (TypeScript + Rust), let's see what's up

@jkomyno jkomyno merged commit 9599c67 into main May 31, 2024
208 checks passed
@jkomyno jkomyno deleted the fix/d1-custom-max-bind-values branch May 31, 2024 13:26
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.

3 participants