-
Notifications
You must be signed in to change notification settings - Fork 49
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: Disable round trip on write #245
perf: Disable round trip on write #245
Conversation
17d4e48
to
7baf9ad
Compare
@maxsmythe looks like GK and unit tests are failing |
This disables the round-tripping of cached data between serialized/deserialized JSON when caching in OPA. This reduces the amount of time a lock is held on OPA's cache, which helps mitigate the lock contention mentioned in open-policy-agent/gatekeeper#2060 Signed-off-by: Max Smythe <smythe@google.com>
7baf9ad
to
4789b1d
Compare
Codecov ReportBase: 50.22% // Head: 50.16% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #245 +/- ##
==========================================
- Coverage 50.22% 50.16% -0.06%
==========================================
Files 64 64
Lines 4255 4266 +11
==========================================
+ Hits 2137 2140 +3
- Misses 1879 1885 +6
- Partials 239 241 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
Yeah, turns out RoundTrip had a different signature than I thought. Hopefully the newest change works. |
@@ -18,7 +18,7 @@ require ( | |||
github.com/davecgh/go-spew v1.1.1 | |||
github.com/golang/glog v1.0.0 | |||
github.com/google/go-cmp v0.5.9 | |||
github.com/open-policy-agent/opa v0.42.2 | |||
github.com/open-policy-agent/opa v0.44.0 |
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.
This bump also fixes few CVEs. yay!
@@ -415,7 +416,22 @@ func (c *Client) AddData(ctx context.Context, data interface{}) (*types.Response | |||
} | |||
} | |||
|
|||
err = c.driver.AddData(ctx, name, key, processedData) | |||
// Round trip data to force untyped JSON, as drivers are not type-aware | |||
bytes, err := json.Marshal(processedData) |
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.
For my own knowledge, why does doing round trip here improves performance compare to in storage?
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.
Storage acquires a mutex, so a round trip there blocks all Rego evaluation. Round trips here can be done in parallel without blocking Rego evaluation.
TL;DR less CPU time spent with a mutex
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.
LGTM
This disables the round-tripping of cached data between serialized/deserialized JSON when caching in OPA. This reduces the amount of time a lock is held on OPA's cache, which helps mitigate the lock contention mentioned in
open-policy-agent/gatekeeper#2060
Signed-off-by: Max Smythe smythe@google.com