Skip to content
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

Improve speed of CoroutineExecutorService within StateFragmentLocalTest #1765

Open
BenHenning opened this issue Sep 2, 2020 · 2 comments
Open
Labels
enhancement End user-perceivable enhancements. Impact: Low Low perceived user impact (e.g. edge cases). Work: Medium The means to find the solution is clear, but it isn't at good-first-issue level yet. Z-ibt Temporary label for Ben to keep track of issues he's triaged.

Comments

@BenHenning
Copy link
Member

#1764 introduced a CoroutineExecutorService to stabilize tests in #1630. Unfortunately, something about the final implementation of the executor service is particularly inefficient, resulting in tests that previously took 1-2 seconds now taking 11-20 seconds. See: #1630 (comment). While the total test suite time is well below the limit where it would cause issues in CI, this is generally a bad state to be in. Someone will need to dig into why the executor service isn't running optimally, and find ways to optimize it (note that the original implementation didn't have this issue, but it wasn't checked in due to having many other problems that were discovered & fixed as part of writing the initial test suite for the service).

@BenHenning BenHenning added Type: Improvement Priority: Nice-to-have This work item is nice to have for its milestone. labels Sep 2, 2020
@BenHenning BenHenning added this to the Backlog milestone Sep 2, 2020
@BenHenning
Copy link
Member Author

BenHenning commented Sep 2, 2020

I suspect that this is in part caused by CoroutineExecutorService performing operations (especially in the context of its test suite) in a way that is not cooperative with cancellability, so the timeout functions aren't actually early exiting as expected.

@BenHenning
Copy link
Member Author

It turns out this is even lower priority: StateFragmentLocalTest actually isn't affected after adding a tear-down to flush all tasks from the dispatcher queue (it seems Glide was trying to terminate previous operations that were blocked on the dispatchers wrapping up background work). Keeping this open since the executor service tests do still need to be improved (and likely the implementation of the executor service itself).

@Broppia Broppia added issue_type_infrastructure Impact: Low Low perceived user impact (e.g. edge cases). labels Jul 29, 2022
@BenHenning BenHenning added the Z-ibt Temporary label for Ben to keep track of issues he's triaged. label Sep 16, 2022
@BenHenning BenHenning removed this from the Backlog milestone Sep 16, 2022
@seanlip seanlip added bug End user-perceivable behaviors which are not desirable. and removed issue_type_infrastructure labels Mar 28, 2023
@MohitGupta121 MohitGupta121 added the Work: Medium The means to find the solution is clear, but it isn't at good-first-issue level yet. label Jun 16, 2023
@seanlip seanlip added enhancement End user-perceivable enhancements. and removed bug End user-perceivable behaviors which are not desirable. Type: Improvement labels Jun 16, 2023
@seanlip seanlip removed the Priority: Nice-to-have This work item is nice to have for its milestone. label Jun 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement End user-perceivable enhancements. Impact: Low Low perceived user impact (e.g. edge cases). Work: Medium The means to find the solution is clear, but it isn't at good-first-issue level yet. Z-ibt Temporary label for Ben to keep track of issues he's triaged.
Development

Successfully merging a pull request may close this issue.

4 participants