-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Fix Perf Test Regression #7677
base: master
Are you sure you want to change the base?
Fix Perf Test Regression #7677
Conversation
Signed-off-by: Cassandra Coyle <cassie@diagrid.io>
/ok-to-perf |
👋 @cicoyle, my apologies but I can't perform this action for you because your username is not in the allowlist in the file |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #7677 +/- ##
==========================================
+ Coverage 61.39% 61.80% +0.41%
==========================================
Files 265 245 -20
Lines 22609 22420 -189
==========================================
- Hits 13880 13856 -24
+ Misses 7579 7400 -179
- Partials 1150 1164 +14 ☔ View full report in Codecov by Sentry. |
/ok-to-perf |
Dapr perf testCommit ref: 8659561 ✅ Build succeeded
✅ Infrastructure deployed
❌ Perf tests failedPlease check the logs for details on the error. |
Signed-off-by: Cassandra Coyle <cassie@diagrid.io>
Signed-off-by: Cassandra Coyle <cassie@diagrid.io>
/ok-to-perf |
Dapr perf testCommit ref: 453f17f ✅ Build succeeded
✅ Infrastructure deployed
❌ Perf tests failedPlease check the logs for details on the error. |
Signed-off-by: Cassandra Coyle <cassie@diagrid.io>
/ok-to-perf |
Dapr perf testCommit ref: 0801a83 ✅ Build succeeded
✅ Infrastructure deployed
❌ Perf tests failedPlease check the logs for details on the error. |
Signed-off-by: Cassandra Coyle <cassie@diagrid.io>
/ok-to-perf |
Dapr perf testCommit ref: 25fea4b ✅ Build succeeded
✅ Infrastructure deployed
❌ Perf tests failedPlease check the logs for details on the error. |
/ok-to-perf |
Dapr perf testCommit ref: 25fea4b ✅ Build succeeded
✅ Infrastructure deployed
❌ Perf tests failedPlease check the logs for details on the error. |
Signed-off-by: Cassandra Coyle <cassie@diagrid.io>
…rialization from release1.13 Signed-off-by: Cassandra Coyle <cassie@diagrid.io>
/ok-to-perf |
Dapr perf testCommit ref: d17d61a ✅ Build succeeded
✅ Infrastructure deployed
✅ Perf tests succeeded
|
Dapr perf testCommit ref: 3957199 ✅ Build succeeded
✅ Infrastructure deployed
❌ Perf tests failedPlease check the logs for details on the error. |
@@ -39,7 +39,7 @@ const ( | |||
appName = "perf-actor-reminder-service" | |||
|
|||
// Target for the QPS | |||
targetQPS = 56 | |||
targetQPS = 50 |
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.
Lowering this to acct for json being made the default, which isn't as performant as using protobufs for storage - the diff is quite noticeable with the target app CPU time. Using json requires the app to marshal and unmarshal the data increasing the target app CPU time vs using protobufs which significantly lowers the target app CPU time.
This target app CPU time diff btw protobuf & json is:
JSON: 7m CPU time
Protobuf: 1m CPU time
Signed-off-by: Cassie Coyle <cassie.i.coyle@gmail.com>
/ok-to-perf |
Dapr perf testCommit ref: aefbb15 ✅ Build succeeded
✅ Infrastructure deployed
❌ Perf tests failedPlease check the logs for details on the error. |
Signed-off-by: Cassandra Coyle <cassie@diagrid.io>
/ok-to-perf |
Dapr perf testCommit ref: 01aa657 ✅ Build succeeded
✅ Infrastructure deployed
❌ Perf tests failedPlease check the logs for details on the error. |
Looks like |
/ok-to-perf |
Dapr perf testCommit ref: d3224b2 ✅ Build succeeded
✅ Infrastructure deployed
✅ Perf tests succeeded
|
Maybe that unit test for placement membership is flaky? |
/ok-to-perf |
Dapr perf testCommit ref: d3224b2 ✅ Build succeeded
✅ Infrastructure deployed
✅ Perf tests succeeded
|
Signed-off-by: Cassandra Coyle <cassie@diagrid.io>
Signed-off-by: Cassandra Coyle <cassie@diagrid.io>
/ok-to-perf |
Dapr perf testCommit ref: 55f567c ✅ Build succeeded
✅ Infrastructure deployed
❌ Perf tests failedPlease check the logs for details on the error. |
@cicoyle you can use |
/ok-to-perf actor_reminder |
Dapr perf testCommit ref: f6690c0 ✅ Build succeeded
✅ Infrastructure deployed
❌ Perf tests failedPlease check the logs for details on the error. |
/ok-to-perf actor_reminder |
1 similar comment
/ok-to-perf actor_reminder |
Dapr perf testCommit ref: 03e41c1 ✅ Build succeeded
✅ Infrastructure deployed
❌ Perf tests failedPlease check the logs for details on the error. |
Dapr perf testCommit ref: 03e41c1 ✅ Build succeeded
✅ Infrastructure deployed
❌ Perf tests failedPlease check the logs for details on the error. |
Looks like there is a panic with |
Reverting the changes from this PR updating actor code: update master w/ release-1.13 branch changes resulted in the perf tests passing again, specifically the
TestActorReminderRegistrationPerformance
test.The changes involve reverting the: json serialization and api level.