test(common): tradeoff running time vs. repro power#12378
Merged
coryan merged 3 commits intogoogleapis:mainfrom Aug 14, 2023
Merged
test(common): tradeoff running time vs. repro power#12378coryan merged 3 commits intogoogleapis:mainfrom
coryan merged 3 commits intogoogleapis:mainfrom
Conversation
|
🤖 I detect that the PR title and the commit message differ and there's only one commit. To use the PR title for the commit history, you can use Github's automerge feature with squashing, or use -- conventional-commit-lint bot |
coryan
changed the title
Contributor
|
SGTM. Let's see what the builds say. |
Contributor
|
/gcbrun |
coryan
reviewed
Aug 14, 2023
| using TimerFuture = future<StatusOr<std::chrono::system_clock::time_point>>; | ||
| auto worker = [&](CompletionQueue cq) { | ||
| for (int i = 0; i != 10000; ++i) { | ||
| for (int i = 0; i != 1000; ++i) { |
Contributor
There was a problem hiding this comment.
I think we should change the comments on line 81 to reflect the new expected values. Something like:
// The magic numbers in this test (number of iterations, thread, etc.) were originally tuned
// to repro #5141 in 99 out of 100 runs using the workstations available at the time. Since
// then, the number of iterations was reduced. The higher number was making the test too
// long with a the new, more deterministic albeit slower implementation of gRPC alarms.
// The new implementation expires the alarm in a I/O thread vs. sometimes using the thread
// scheduling the alarm. We expect a repro in 10 out of 100 runs.
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #12378 +/- ##
=======================================
Coverage 93.61% 93.61%
=======================================
Files 2029 2029
Lines 179856 179856
=======================================
+ Hits 168377 168379 +2
+ Misses 11479 11477 -2
☔ View full report in Codecov by Sentry. |
Contributor
|
/gcbrun |
coryan
approved these changes
Aug 14, 2023
drfloob
added a commit
to grpc/grpc
that referenced
this pull request
Aug 16, 2023
…hancements (#34056) This PR is mainly a set of improvements that allow the C++ Alarm to be migrated away from legacy iomgr. It cannot be landed without significant speedup, due to third-parties relying on a fast path for immediate timer execution with deadlines <= now. Previous EventEngine performance of bm_alarm, compared to baseline iomgr timers: *0.014%* This PR: *2.5%* Regarding previous failures to land this change: The cloud libraries team agreed to reduce the amount of stress in their alarm stress test googleapis/google-cloud-cpp#12378
eugeneo
pushed a commit
to eugeneo/grpc
that referenced
this pull request
Aug 21, 2023
…hancements (grpc#34056) This PR is mainly a set of improvements that allow the C++ Alarm to be migrated away from legacy iomgr. It cannot be landed without significant speedup, due to third-parties relying on a fast path for immediate timer execution with deadlines <= now. Previous EventEngine performance of bm_alarm, compared to baseline iomgr timers: *0.014%* This PR: *2.5%* Regarding previous failures to land this change: The cloud libraries team agreed to reduce the amount of stress in their alarm stress test googleapis/google-cloud-cpp#12378
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is a gRPC test that relied on the high performance of an uncommon (synthetic) gRPC Alarm usage pattern. Per the test comments, this change will reduce the likelihood of seeing a regression in gRPC from 99% of runs, to 10% of runs. I think this should still be sufficient to identify problems. Running this test with the next generation of gRPC Alarm implementations, in 100 runs, this
TimerCanceltest never exceeded 10 seconds in theasan-prdocker environment (most commonly: 5 to 7s).This change is