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

util/ring: TestRingBuffer takes unreasonably long time #65050

Closed
yuzefovich opened this issue May 12, 2021 · 3 comments · Fixed by #65323
Closed

util/ring: TestRingBuffer takes unreasonably long time #65050

yuzefovich opened this issue May 12, 2021 · 3 comments · Fixed by #65323
Labels
C-cleanup Tech debt, refactors, loose ends, etc. Solution not expected to significantly change behavior. E-easy Easy issue to tackle, requires little or no CockroachDB experience good first issue

Comments

@yuzefovich
Copy link
Member

From #65045 (comment):

TestRingBuffer - 55.76s

We should investigate why it is so and make it faster. Also see #63388 - hopefully we can revert that change too.

@yuzefovich yuzefovich added C-cleanup Tech debt, refactors, loose ends, etc. Solution not expected to significantly change behavior. E-easy Easy issue to tackle, requires little or no CockroachDB experience good first issue labels May 12, 2021
@XuanYang-cn
Copy link
Contributor

t.Parallel will help a bit.

On my machine, the test takes 11.41s, ok example.com/ring 11.241s

If add t.Parallel

 func TestRingBufferA(t *testing.T) {                                                                             
     t.Parallel()                                                                                                 
      for count := 1; count <= maxCount; count++ {                                                                 
         t.Run("Parallel", func(t *testing.T) {                                                                   
             t.Parallel()                                                                                         
             testRingBuffer(t, count)                                                                                        
         })                                                                                                       
      }                                                                                                            
  }  

The test takes 7.89s, ok example.com/ring 7.897s, about 30% speedup.

@yuzefovich
Copy link
Member Author

@XuanYang-cn thanks for looking into this! I agree, running the subtests in parallel is a good idea. I think it'd be reasonable to reduce the number of runs (i.e. maxCount) from 1000 maybe to 100. Do you wanna open a PR for these two ideas? I'll assign the issue to you for now.

@XuanYang-cn
Copy link
Contributor

@yuzefovich OK,I'll open a pr

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-cleanup Tech debt, refactors, loose ends, etc. Solution not expected to significantly change behavior. E-easy Easy issue to tackle, requires little or no CockroachDB experience good first issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants