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

Revert "qe: add json serialization span in binary engine" #4824

Merged

Conversation

aqrln
Copy link
Member

@aqrln aqrln commented Apr 10, 2024

Reverts #4339

This works with normal binary engine but breaks tracing with data proxy (i.e. mini-proxy and Accelerate) because these changes are not compatible with TracingConfig::Captured and sending traces as part of the response.

With the widened scope of the prisma:engine span, it is not closed by the time the traces are captured to be added to the response. In addition, it is fundamentally impossible to send the added prisma:engine:response_json_serialization with captured traces as it will pose a chicken and egg problem: the information about the time it took to prepare the JSON response would need to be a part of the same response.

While it is possible to add some conditional logic to keep the new (consistent with the Node-API) behaviour for the normal binary engine (without data proxy / accelerate) with traces in stdout but revert to the old behaviour when captured traces in HTTP response are enabled, for now it is easier to just revert the commit. We don't use binary engine except to power Accelerate nowadays much anyway.

@aqrln aqrln requested a review from a team as a code owner April 10, 2024 19:28
@aqrln aqrln requested review from Weakky and removed request for a team April 10, 2024 19:28
Copy link
Contributor

WASM Query Engine file Size

Engine This PR Base branch Diff
Postgres 2.122MiB 2.122MiB 0.000B
Postgres (gzip) 835.680KiB 835.682KiB -2.000B
Mysql 2.092MiB 2.092MiB 0.000B
Mysql (gzip) 823.415KiB 823.416KiB -1.000B
Sqlite 1.987MiB 1.987MiB 0.000B
Sqlite (gzip) 783.935KiB 783.936KiB -1.000B

Copy link

codspeed-hq bot commented Apr 10, 2024

CodSpeed Performance Report

Merging #4824 will degrade performances by 5.45%

Comparing revert-4339-qe-add-json-seriaization-span-in-binary-engine (2f7b510) with main (6144ce0)

Summary

❌ 1 regressions
✅ 10 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark main revert-4339-qe-add-json-seriaization-span-in-binary-engine Change
large_read 7.7 ms 8.1 ms -5.45%

Copy link
Contributor

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

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.1 (x64-linux)

benchmark                   time (avg)             (min … max)       p75       p99      p999
-------------------------------------------------------------- -----------------------------
• movies.findMany() (all - ~50K)
-------------------------------------------------------------- -----------------------------
Web Assembly: Baseline     371 ms/iter       (367 ms … 374 ms)    373 ms    374 ms    374 ms
Web Assembly: Latest       459 ms/iter       (457 ms … 463 ms)    462 ms    463 ms    463 ms
Web Assembly: Current      456 ms/iter       (452 ms … 469 ms)    465 ms    469 ms    469 ms
Node API: Current          199 ms/iter       (196 ms … 202 ms)    201 ms    202 ms    202 ms

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

• movies.findMany({ take: 2000 })
-------------------------------------------------------------- -----------------------------
Web Assembly: Baseline  14'987 µs/iter (14'742 µs … 16'841 µs) 14'952 µs 16'841 µs 16'841 µs
Web Assembly: Latest    18'676 µs/iter (18'336 µs … 21'503 µs) 18'622 µs 21'503 µs 21'503 µs
Web Assembly: Current   18'400 µs/iter (18'174 µs … 18'811 µs) 18'447 µs 18'811 µs 18'811 µs
Node API: Current        8'284 µs/iter  (7'914 µs … 10'197 µs)  8'304 µs 10'197 µs 10'197 µs

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

• movies.findMany({ where: {...}, take: 2000 })
-------------------------------------------------------------- -----------------------------
Web Assembly: Baseline   2'354 µs/iter   (2'196 µs … 3'966 µs)  2'356 µs  3'743 µs  3'966 µs
Web Assembly: Latest     2'914 µs/iter   (2'796 µs … 4'599 µs)  2'894 µs  3'541 µs  4'599 µs
Web Assembly: Current    2'869 µs/iter   (2'768 µs … 3'687 µs)  2'849 µs  3'674 µs  3'687 µs
Node API: Current        1'408 µs/iter   (1'298 µs … 4'899 µs)  1'408 µs  1'893 µs  4'899 µs

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

• movies.findMany({ include: { cast: true } take: 2000 }) (m2m)
-------------------------------------------------------------- -----------------------------
Web Assembly: Baseline     570 ms/iter       (564 ms … 579 ms)    579 ms    579 ms    579 ms
Web Assembly: Latest       787 ms/iter       (783 ms … 792 ms)    790 ms    792 ms    792 ms
Web Assembly: Current      783 ms/iter       (778 ms … 790 ms)    787 ms    790 ms    790 ms
Node API: Current          469 ms/iter       (460 ms … 489 ms)    475 ms    489 ms    489 ms

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

• movies.findMany({ where: {...}, include: { cast: true } take: 2000 }) (m2m)
-------------------------------------------------------------- -----------------------------
Web Assembly: Baseline  78'959 µs/iter (78'344 µs … 79'537 µs) 79'337 µs 79'537 µs 79'537 µs
Web Assembly: Latest       111 ms/iter       (110 ms … 113 ms)    111 ms    113 ms    113 ms
Web Assembly: Current      109 ms/iter       (109 ms … 110 ms)    110 ms    110 ms    110 ms
Node API: Current       60'982 µs/iter (59'399 µs … 62'335 µs) 61'702 µs 62'335 µs 62'335 µs

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

• movies.findMany({ take: 2000, include: { cast: { include: { person: true } } } })
-------------------------------------------------------------- -----------------------------
Web Assembly: Baseline   1'016 ms/iter   (1'006 ms … 1'037 ms)  1'029 ms  1'037 ms  1'037 ms
Web Assembly: Latest     1'303 ms/iter   (1'295 ms … 1'316 ms)  1'310 ms  1'316 ms  1'316 ms
Web Assembly: Current    1'311 ms/iter   (1'302 ms … 1'328 ms)  1'326 ms  1'328 ms  1'328 ms
Node API: Current          890 ms/iter       (873 ms … 911 ms)    906 ms    911 ms    911 ms

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

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

summary for movie.findMany({ where: { ... }, take: 2000, include: { cast: { include: { person: true } } } })
  Web Assembly: Current
   1.64x slower than Node API: Current
   1.27x 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'049 µs/iter   (1'001 µs … 1'702 µs)  1'048 µs  1'578 µs  1'702 µs
Web Assembly: Latest     1'391 µs/iter   (1'326 µs … 1'880 µs)  1'389 µs  1'803 µs  1'880 µs
Web Assembly: Current    1'380 µs/iter   (1'323 µs … 1'862 µs)  1'383 µs  1'748 µs  1'862 µs
Node API: Current          780 µs/iter     (736 µs … 1'265 µs)    796 µs    835 µs  1'265 µs

summary for movie.findMany({ where: { reviews: { author: { ... } }, take: 100 }) (to-many -> to-one)
  Web Assembly: Current
   1.77x slower than Node API: Current
   1.32x 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'050 µs/iter   (1'012 µs … 1'860 µs)  1'052 µs  1'424 µs  1'860 µs
Web Assembly: Latest     1'380 µs/iter   (1'309 µs … 2'317 µs)  1'377 µs  2'050 µs  2'317 µs
Web Assembly: Current    1'371 µs/iter   (1'306 µs … 2'499 µs)  1'374 µs  1'829 µs  2'499 µs
Node API: Current          772 µs/iter     (714 µs … 1'205 µs)    785 µs    830 µs  1'205 µs

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

After changes in 2f7b510

@jkomyno
Copy link
Contributor

jkomyno commented Apr 11, 2024

Merging this in agreement with @aqrln.

@jkomyno jkomyno merged commit 8b36be3 into main Apr 11, 2024
181 of 182 checks passed
@jkomyno jkomyno deleted the revert-4339-qe-add-json-seriaization-span-in-binary-engine branch April 11, 2024 08:41
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