Skip to content

performance: improve for PathString impl for EvaluationContext#430

Merged
tusharmath merged 9 commits intotailcallhq:mainfrom
meskill:perf/path-string-eval-context
Oct 9, 2023
Merged

performance: improve for PathString impl for EvaluationContext#430
tusharmath merged 9 commits intotailcallhq:mainfrom
meskill:perf/path-string-eval-context

Conversation

@meskill
Copy link
Copy Markdown
Contributor

@meskill meskill commented Oct 6, 2023 •
edited
Loading

Summary:

Implement benchmark for impl PathString for EvaluationContext and optimize its code

Issue Reference(s):
Fixes #347
/claim #347

Build & Testing:

  • I ran cargo test successfully.
  • I have run ./lint.sh to address and fix linting issues.

Checklist:

  • I have added relevant unit & integration tests.
  • I have updated the documentation accordingly (if applicable).
  • I have performed a self-review of my own code.

How to review

I suggest to review the pr commit by commit:

  1. Make PathString trait generic over type of parameter to be able accept both &[String] and &[str]
  2. Decouple EvaluationContext from async_graphql::ResolverContext to be able to create test environment for it
  3. Implement benchmark for the impl
  4. Optimize and refactor the code of impl

Results

image

All results as text ❯ cargo bench --bench impl_path_string_for_evaluation_context -- --baseline init Compiling tailcall v0.1.0 (/home/nixos/Projects/open-source/rust/tailcall) Finished bench [optimized] target(s) in 33.22s Running benches/impl_path_string_for_evaluation_context.rs (target/release/deps/impl_path_string_for_evaluation_context-f4ac70d9689a5945) Gnuplot not found, using plotters backend input/value.root time: [31.628 ns 31.953 ns 32.409 ns] change: [-32.327% -30.975% -29.546%] (p = 0.00 < 0.05) Performance has improved. Found 2 outliers among 100 measurements (2.00%) 2 (2.00%) high severe

input/value.nested.existing
time: [65.308 ns 68.170 ns 71.823 ns]
change: [-11.594% -9.3241% -5.2798%] (p = 0.00 < 0.05)
Performance has improved.
Found 22 outliers among 100 measurements (22.00%)
16 (16.00%) high mild
6 (6.00%) high severe

input/value.missing time: [25.998 ns 27.113 ns 28.361 ns]
change: [-6.3436% -4.0818% -1.5430%] (p = 0.00 < 0.05)
Performance has improved.
Found 12 outliers among 100 measurements (12.00%)
4 (4.00%) high mild
8 (8.00%) high severe

input/value.nested.missing
time: [52.774 ns 53.297 ns 53.985 ns]
change: [-7.7919% -5.3156% -3.1589%] (p = 0.00 < 0.05)
Performance has improved.
Found 9 outliers among 100 measurements (9.00%)
3 (3.00%) high mild
6 (6.00%) high severe

input/args.root time: [23.415 ns 23.532 ns 23.648 ns]
change: [-83.952% -83.854% -83.757%] (p = 0.00 < 0.05)
Performance has improved.
Found 2 outliers among 100 measurements (2.00%)
2 (2.00%) high mild

input/args.nested.existing
time: [50.387 ns 51.123 ns 52.093 ns]
change: [-71.882% -71.237% -70.655%] (p = 0.00 < 0.05)
Performance has improved.
Found 4 outliers among 100 measurements (4.00%)
4 (4.00%) high severe

input/args.missing time: [13.291 ns 13.386 ns 13.498 ns]
change: [-89.743% -89.583% -89.384%] (p = 0.00 < 0.05)
Performance has improved.
Found 6 outliers among 100 measurements (6.00%)
2 (2.00%) high mild
4 (4.00%) high severe

input/args.nested.missing
time: [43.307 ns 43.736 ns 44.283 ns]
change: [-72.766% -71.938% -71.117%] (p = 0.00 < 0.05)
Performance has improved.
Found 18 outliers among 100 measurements (18.00%)
2 (2.00%) low severe
2 (2.00%) low mild
5 (5.00%) high mild
9 (9.00%) high severe

input/headers.existing time: [11.790 ns 11.873 ns 11.967 ns]
change: [-11.716% -9.2507% -7.0145%] (p = 0.00 < 0.05)
Performance has improved.
Found 2 outliers among 100 measurements (2.00%)
2 (2.00%) high severe

input/headers.missing time: [13.003 ns 13.618 ns 14.300 ns]
change: [-4.6410% -1.5067% +1.9316%] (p = 0.38 > 0.05)
No change in performance detected.
Found 6 outliers among 100 measurements (6.00%)
3 (3.00%) high mild
3 (3.00%) high severe

input/vars.existing time: [5.5251 ns 5.5561 ns 5.5948 ns]
change: [-89.906% -89.417% -88.978%] (p = 0.00 < 0.05)
Performance has improved.
Found 8 outliers among 100 measurements (8.00%)
4 (4.00%) high mild
4 (4.00%) high severe

input/vars.missing time: [6.0952 ns 6.4040 ns 6.7412 ns]
change: [-85.395% -84.837% -84.228%] (p = 0.00 < 0.05)
Performance has improved.
Found 6 outliers among 100 measurements (6.00%)
2 (2.00%) high mild
4 (4.00%) high severe

@algora-pbc algora-pbc bot mentioned this pull request Oct 6, 2023
Closed
meskill
meskill commented Oct 6, 2023
View reviewed changes
@meskill meskill force-pushed the perf/path-string-eval-context branch from 337bdaf to f295a78 Compare October 6, 2023 11:08
tusharmath
tusharmath reviewed Oct 7, 2023
View reviewed changes
tusharmath
tusharmath reviewed Oct 7, 2023
View reviewed changes
@tusharmath
Copy link
Copy Markdown
Contributor

tusharmath commented Oct 7, 2023

Can you share the improvement if any on value.

@meskill meskill force-pushed the perf/path-string-eval-context branch from f295a78 to 095edea Compare October 7, 2023 18:29
@codecov
Copy link
Copy Markdown

codecov bot commented Oct 7, 2023 •
edited
Loading

Codecov Report

Attention: 16 lines in your changes are missing coverage. Please review.

Comparison is base (89afa31) 85.15% compared to head (2ad5eec) 84.98%.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main     #430      +/-   ##
==========================================
- Coverage   85.15%   84.98%   -0.17%     
==========================================
  Files          49       50       +1     
  Lines        4467     4490      +23     
==========================================
+ Hits         3804     3816      +12     
- Misses        663      674      +11
Files Coverage Δ
src/blueprint/into_schema.rs 97.95% <100.00%> (ø)
src/has_headers.rs 100.00% <ø> (ø)
src/lambda/expression.rs 84.12% <100.00%> (ø)
src/lambda/lambda.rs 98.57% <100.00%> (ø)
src/mustache.rs 100.00% <100.00%> (ø)
src/request_template.rs 99.69% <100.00%> (ø)
src/json/json_like.rs 64.68% <80.00%> (ø)
src/lambda/evaluation_context.rs 92.30% <89.28%> (-4.21%) ⬇️
src/lambda/resolver_context_like.rs 50.00% <50.00%> (ø)
src/path_string.rs 75.00% <72.72%> (-1.67%) ⬇️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@meskill
Copy link
Copy Markdown
Contributor Author

meskill commented Oct 7, 2023

Can you share the improvement if any on value.

Here is screenshot from the latest run

image

Also you can find the whole result under spoiler at the end in the pr description

@meskill
Copy link
Copy Markdown
Contributor Author

meskill commented Oct 7, 2023

Code coverage check has failed. Is it important here? Most of the changes I've made seems like didn't have tests in the first place

@tusharmath
Copy link
Copy Markdown
Contributor

tusharmath commented Oct 9, 2023

@meskill I think I missed out on some conflict resolutions. Can you please have a look. I think overall it looks good.

@meskill
Copy link
Copy Markdown
Contributor Author

meskill commented Oct 9, 2023

@meskill I think I missed out on some conflict resolutions. Can you please have a look. I think overall it looks good.

Yeah, I've fixed the problem

@tusharmath tusharmath changed the title Improve perf of PathString impl for EvaluationContext performance: improve perf of PathString impl for EvaluationContext Oct 9, 2023
@tusharmath tusharmath changed the title performance: improve perf of PathString impl for EvaluationContext performance: improve for PathString impl for EvaluationContext Oct 9, 2023
@tusharmath tusharmath merged commit 9c50345 into tailcallhq:main Oct 9, 2023
@meskill meskill deleted the perf/path-string-eval-context branch October 9, 2023 11:23
digital-phoenix pushed a commit to digital-phoenix/tailcall that referenced this pull request Oct 12, 2023
Merge commit '234cd25e61d23bd2300a6e15bf0b9b4014a0073c' into main
47b23ed 
* commit '234cd25e61d23bd2300a6e15bf0b9b4014a0073c': (25 commits)
  fix: cache control headers (tailcallhq#465)
  chore: update cron freq for PR staleness
  feat: group by endpoint (tailcallhq#451)
  chore: add codecov config
  CI: ignore build when no changes for executable (tailcallhq#454)
  feature: add `tc init` command (tailcallhq#403)
  fix: deploy (tailcallhq#457)
  fix(deps): update rust crate regex to 1.10.0 (tailcallhq#456)
  fix: cargo test fail in Sonoma or later of macos (tailcallhq#453)
  chore: update examples (tailcallhq#452)
  performance: improve for PathString impl for EvaluationContext (tailcallhq#430)
  chore: close stale branches after 2 days
  fix: restricted url for graphiQL (tailcallhq#447)
  performance: minor optimisation (tailcallhq#444)
  chore:update readme (tailcallhq#446)
  chore: update stale bot schedule
  chore: update stale bot message
  chore: use title case for job names in CI (tailcallhq#437)
  chore: remove write permissions
  chore: update stale YML permissions
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tusharmath tusharmath tusharmath approved these changes

Assignees

No one assigned

Labels

🙋 Bounty claim

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Optimize impl PathString for EvaluationContext

2 participants

@meskill @tusharmath