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

perf: use serde_json_borrow for Synth output #3073

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

Conversation

meskill
Copy link
Contributor

@meskill meskill commented Oct 25, 2024

Summary:
Replace using only async_graphql_value::ConstValue with serde_json_borrow::Value for the Sync output. Evaluation and store left intact and that uses ConstValue as before.

Issue Reference(s):
Fixes #... (Replace "..." with the issue number)

Build & Testing:

  • I ran cargo test successfully.
  • I have run ./lint.sh --mode=fix to fix all linting issues raised by ./lint.sh --mode=check.

Checklist:

  • I have added relevant unit & integration tests.
  • I have updated the documentation accordingly.
  • I have performed a self-review of my code.
  • PR follows the naming convention of <type>(<optional scope>): <title>

@github-actions github-actions bot added ci: benchmark Runs benchmarks type: performance Improved performance. labels Oct 25, 2024
@meskill meskill added ci: benchmark Runs benchmarks and removed ci: benchmark Runs benchmarks labels Oct 25, 2024
@meskill
Copy link
Contributor Author

meskill commented Oct 25, 2024

Wrk changes

posts title

before

Running 30s test @ http://localhost:8000/graphql
  4 threads and 100 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency     2.47ms   13.49ms 360.82ms   99.24%
    Req/Sec    17.39k     1.85k   22.15k    86.81%
  2075331 requests in 30.09s, 10.40GB read
Requests/sec:  68973.83
Transfer/sec:    354.02MB

serde_json_borrow

Running 30s test @ http://localhost:8000/graphql
  4 threads and 100 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency     2.50ms   14.78ms 397.01ms   99.22%
    Req/Sec    18.43k     2.19k   21.42k    89.15%
  2203809 requests in 29.23s, 11.05GB read
  Socket errors: connect 0, read 0, write 0, timeout 100
Requests/sec:  75406.44
Transfer/sec:    387.04MB

big query

before

Running 30s test @ http://localhost:8000/graphql
  4 threads and 100 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency     9.46ms    4.02ms 100.68ms   85.24%
    Req/Sec     2.70k   332.08     5.14k    86.24%
  322219 requests in 30.09s, 15.94GB read
Requests/sec:  10708.26
Transfer/sec:    542.35MB

serde_json_borrow

Running 30s test @ http://localhost:8000/graphql
  4 threads and 100 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency     9.07ms    3.22ms  90.35ms   77.74%
    Req/Sec     2.79k   323.80     3.60k    82.50%
  333021 requests in 30.08s, 16.47GB read
Requests/sec:  11072.28
Transfer/sec:    560.79MB

@meskill
Copy link
Contributor Author

meskill commented Oct 25, 2024

Flamegraphs

posts title

insert_key from 4.44% to 1.3%

image

big query

insert_key from 4.96% to 0.94%

image

@meskill
Copy link
Contributor Author

meskill commented Oct 25, 2024

The performance difference is noticeable, but for the big queries the impact is lower showing only around 3% improvement.

And this pr needs several more fixes to work because right now it's only possible to run the benchmarks and tests are failing:

  • implement clone_from for case of object. The clone_from is introduced to be able to clone from one JsonLike to another JsonLike
  • implement merge for instrospection query because we have different JsonLike entries now and it should be merged differently like it was with ConstValue
  • fix the tests related to changes in exec signature

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci: benchmark Runs benchmarks type: performance Improved performance.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant