Skip to content

Conversation

@janisz
Copy link
Contributor

@janisz janisz commented Feb 3, 2025

Description

This PR creates a wrapper for a default grpc codec that will use VTproto functions when possible to un/marshal objects. This will reduce usage of reflection. Since we use bytes pool we cannot use unsafe unmarshaller as we do not own the byte slice see doc (like we do in pg stores).

Reference implementation: https://github.com/grpc/grpc-go/blob/v1.70.0/encoding/proto/proto.go#L33

old.txtnew.txt                │
                       │    sec/opsec/op     vs baseProtoUnmarshal/small-8   46.08µ ±  1%   42.39µ ± 18%        ~ (p=0.143 n=10)
ProtoUnmarshal/big-8     3.489m ± 63%   2.771m ± 64%        ~ (p=0.353 n=10)
geomean                  401.0µ         342.8µ        -14.53%

Refs:

Testing

  • inspected CI results

Automated testing

  • modified existing tests
  • contributed no automated tests

How I validated my change

CI

@rhacs-bot
Copy link
Contributor

rhacs-bot commented Feb 3, 2025

Images are ready for the commit at cf188e4.

To use with deploy scripts, first export MAIN_IMAGE_TAG=4.7.x-599-gcf188e4495.

@janisz janisz requested review from mtodor and rhybrillou February 3, 2025 13:53
@codecov
Copy link

codecov bot commented Feb 3, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 49.04%. Comparing base (2d087e9) to head (4288be8).
Report is 3 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master   #14083   +/-   ##
=======================================
  Coverage   49.04%   49.04%           
=======================================
  Files        2514     2515    +1     
  Lines      182854   182898   +44     
=======================================
+ Hits        89687    89710   +23     
- Misses      86045    86059   +14     
- Partials     7122     7129    +7     
Flag Coverage Δ
go-unit-tests 49.04% <ø> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

@janisz janisz added auto-retest PRs with this label will be automatically retested if prow checks fails proto-v2 protobuff switch related PR labels Feb 3, 2025
@rhacs-bot
Copy link
Contributor

rhacs-bot commented Feb 3, 2025

Images are ready for the commit at b73166f.

To use with deploy scripts, first export MAIN_IMAGE_TAG=4.7.x-600-gb73166f3e6.

Copy link
Contributor

@rhybrillou rhybrillou left a comment

Choose a reason for hiding this comment

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

Let's hold that one until after the 4.7 code freeze. Looks good otherwise.

@rhacs-bot
Copy link
Contributor

rhacs-bot commented Feb 4, 2025

Images are ready for the commit at a398944.

To use with deploy scripts, first export MAIN_IMAGE_TAG=4.8.x-5-ga398944912.

@janisz janisz added the hold label Feb 6, 2025
@janisz janisz changed the title perf(grpc): use vtproto functions as a default codec ROX-27956: use vtproto functions as a default codec Feb 6, 2025
@janisz janisz requested a review from rhybrillou February 6, 2025 11:35
@janisz janisz force-pushed the use_vt_proto_for_grpc branch from b73166f to a398944 Compare February 13, 2025 12:09
@stackrox stackrox deleted a comment from rhacs-bot Feb 13, 2025
@stackrox stackrox deleted a comment from rhacs-bot Feb 13, 2025
@stackrox stackrox deleted a comment from rhacs-bot Feb 13, 2025
@janisz janisz removed the hold label Feb 13, 2025
@rhacs-bot
Copy link
Contributor

rhacs-bot commented Feb 13, 2025

Images are ready for the commit at 3e2a2df.

To use with deploy scripts, first export MAIN_IMAGE_TAG=4.8.x-8-g3e2a2dfe45.

@janisz janisz mentioned this pull request Feb 13, 2025
5 tasks
@janisz janisz force-pushed the use_vt_proto_for_grpc branch from a398944 to 3e2a2df Compare February 13, 2025 16:34
@janisz janisz requested a review from a team as a code owner February 13, 2025 16:34
@janisz janisz changed the base branch from master to add_k6_grpc_tests February 13, 2025 16:34
@janisz janisz added the ci-performance-tests Runs the K6 based performance tests label Feb 13, 2025
@stackrox stackrox deleted a comment from rhacs-bot Feb 13, 2025
@stackrox stackrox deleted a comment from rhacs-bot Feb 13, 2025
@rhacs-bot
Copy link
Contributor

/retest

2 similar comments
@rhacs-bot
Copy link
Contributor

/retest

@rhacs-bot
Copy link
Contributor

/retest

@rhacs-bot
Copy link
Contributor

rhacs-bot commented Feb 13, 2025

Images are ready for the commit at b113454.

To use with deploy scripts, first export MAIN_IMAGE_TAG=4.8.x-20-gb113454207.

@janisz janisz added ci-performance-tests Runs the K6 based performance tests and removed ci-performance-tests Runs the K6 based performance tests labels Feb 14, 2025
@janisz janisz changed the base branch from add_k6_grpc_tests to master February 14, 2025 10:24
@janisz janisz force-pushed the use_vt_proto_for_grpc branch from 3e2a2df to b113454 Compare February 14, 2025 10:57
@github-actions
Copy link
Contributor

github-actions bot commented Feb 14, 2025

Performance test results

Summary

  █ main dashboard

  data_received..................: 2.5 MB 85 kB/s
  data_sent......................: 196 kB 6.8 kB/s
  group_duration.................: avg=581.31ms min=559.19ms med=572.03ms max=904.62ms p(90)=588.23ms p(95)=592.28ms
  http_req_blocked...............: avg=231.79µs min=300ns    med=451ns    max=104.07ms p(90)=591ns    p(95)=626ns   
  http_req_connecting............: avg=115.05µs min=0s       med=0s       max=51.77ms  p(90)=0s       p(95)=0s      
  http_req_duration..............: avg=64.24ms  min=51.79ms  med=57.21ms  max=167.7ms  p(90)=99.94ms  p(95)=103.23ms
    { expected_response:true }...: avg=67.67ms  min=55.69ms  med=59.21ms  max=167.7ms  p(90)=101.36ms p(95)=104.72ms
  ✓ { lib:true }.................: avg=0s       min=0s       med=0s       max=0s       p(90)=0s       p(95)=0s      
  http_req_failed................: 22.22% ✓ 100       ✗ 350
  http_req_receiving.............: avg=319.12µs min=22.19µs  med=37.58µs  max=103.07ms p(90)=323.25µs p(95)=413.07µs
  http_req_sending...............: avg=70.82µs  min=36.69µs  med=71.13µs  max=184.62µs p(90)=89.6µs   p(95)=94.24µs 
  http_req_tls_handshaking.......: avg=115.86µs min=0s       med=0s       max=52.13ms  p(90)=0s       p(95)=0s      
  http_req_waiting...............: avg=63.85ms  min=51.72ms  med=57.11ms  max=167.58ms p(90)=99.6ms   p(95)=103ms   
  http_reqs......................: 450    15.479587/s
  iteration_duration.............: avg=581.39ms min=559.27ms med=572.11ms max=904.8ms  p(90)=588.3ms  p(95)=592.35ms
  iterations.....................: 50     1.719954/s
  vus............................: 1      min=1       max=1
  vus_max........................: 1      min=1       max=1

Sources

@rhacs-bot
Copy link
Contributor

rhacs-bot commented Feb 14, 2025

Images are ready for the commit at 4288be8.

To use with deploy scripts, first export MAIN_IMAGE_TAG=4.8.x-28-g4288be8b34.

@janisz
Copy link
Contributor Author

janisz commented Feb 17, 2025

Comparison with test run on #14265

- grpc_req_duration: avg=444.81ms min=66.02ms  med=203.47ms max=1.79s    p(90)=1.28s    p(95)=1.33s   
+ grpc_req_duration: avg=139.77ms min=37.38ms  med=58.54ms  max=421.75ms p(90)=380.58ms p(95)=390.98ms

Signed-off-by: Tomasz Janiszewski <tomek@redhat.com>
@janisz janisz force-pushed the use_vt_proto_for_grpc branch from b113454 to 4288be8 Compare February 17, 2025 11:20
@janisz janisz enabled auto-merge (squash) February 17, 2025 11:21
@janisz janisz merged commit c096d55 into master Feb 17, 2025
83 of 91 checks passed
@janisz janisz deleted the use_vt_proto_for_grpc branch February 17, 2025 12:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

auto-retest PRs with this label will be automatically retested if prow checks fails ci-performance-tests Runs the K6 based performance tests proto-v2 protobuff switch related PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants