-
Notifications
You must be signed in to change notification settings - Fork 596
perf: introduce translator context #7535
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
base: main
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #7535 +/- ##
==========================================
- Coverage 71.57% 71.21% -0.37%
==========================================
Files 231 274 +43
Lines 42625 34898 -7727
==========================================
- Hits 30507 24851 -5656
+ Misses 10344 8256 -2088
- Partials 1774 1791 +17 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
wait #7534 |
|
/retest |
|
@kkk777-7 you might wanna merge the main branch to this branch to include the disk space cleaner. |
Signed-off-by: kkk777-7 <[email protected]>
Signed-off-by: kkk777-7 <[email protected]>
Signed-off-by: kkk777-7 <[email protected]>
9527aa1 to
ce0cfcc
Compare
internal/gatewayapi/testdata/extensions/httproute-with-custom-backend-mixed-multiple.out.yaml
Show resolved
Hide resolved
|
The gateway/internal/gatewayapi/resource/resource.go Lines 117 to 124 in 7cb5f72
|
This reverts commit 14a3784. Signed-off-by: kkk777-7 <[email protected]>
Signed-off-by: kkk777-7 <[email protected]>
f9b5a6a to
3d4c714
Compare
Signed-off-by: kkk777-7 <[email protected]>
|
Think out load: could we also add other resources(Namespace,Secret,ConfigMap, etc.) to the translator context to avoid linear lookups to improve CPU performance? This could increase memory usage - shouldn't be much increase since we just mirror the slices that already in resource using pointers, but we should benchmark both CPU and memory to ensure we don’t introduce significant overhead. |
we should do that. |
+1 |
|
|
Signed-off-by: kkk777-7 <[email protected]>
Signed-off-by: kkk777-7 <[email protected]>
Signed-off-by: kkk777-7 <[email protected]>
Signed-off-by: kkk777-7 <[email protected]>
| GatewayControllerName: string(rs.GatewayClass.Spec.ControllerName), | ||
| GatewayClassName: gwapiv1.ObjectName(rs.GatewayClass.Name), | ||
| GlobalRateLimitEnabled: true, | ||
| EndpointRoutingDisabled: false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TranslateGatewayAPIToXds sets EndpointRoutingDisabled: true.
so, added new bench test.
| - fqdn: | ||
| hostname: backend-v3.gateway-conformance-infra.svc.cluster.local | ||
| port: 8080 | ||
| - apiVersion: gateway.envoyproxy.io/v1alpha1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed because the resource was defined twice.
https://github.com/envoyproxy/gateway/blob/main/internal/gatewayapi/testdata/backendtrafficpolicy-dns-lookup-family.in.yaml#L123-L152
|
Hi @zirain, @zhaohuabing, got good results 🎉 |
What this PR does / why we need it:
Introduce Translator Context to improve GatewayAPI translator performance.
Now, Gateway API translator methods hold a lot of local maps to improve retrievals.
These can be all moved to a common preprocessing step, saved in the Translator context and reused across methods.
Also each resource is currently retrieved by performing a linear search over
resource.Resources.Therefore, for resources that are accessed frequently, maintaining a map is expected to improve CPU performance.
This PR's scope
Although the following items were initially considered, the improvements were minimal and would increase the complexity, so they were excluded from the scope. Ref : #7535 (comment)
Policy Target Gateway MapPolicy Target Route MapWhich issue(s) this PR fixes:
Fixes #6711
Release Notes: No
Benchmark Detail
gobench result
gatewayapi translator benchmark (vs latest main branch)