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

Optimise gcc.SendSideBWE #142

Closed
wants to merge 4 commits into from
Closed

Conversation

adityaa30
Copy link
Contributor

Description

Reference issue

Fixes #141

- Generate `rtcp.TransportLayerCC` packets with given seq numbers
- instead of passing each `cc.Acknowledgement` 1 by 1 in respective
  channel; pass them all at once to decrease the internal overhead of go
  channels
@codecov
Copy link

codecov bot commented Jun 13, 2022

Codecov Report

Merging #142 (a3bbf62) into master (079fa7c) will increase coverage by 0.75%.
The diff coverage is 94.23%.

@@            Coverage Diff             @@
##           master     #142      +/-   ##
==========================================
+ Coverage   79.95%   80.71%   +0.75%     
==========================================
  Files          58       58              
  Lines        2779     2779              
==========================================
+ Hits         2222     2243      +21     
+ Misses        449      430      -19     
+ Partials      108      106       -2     
Flag Coverage Δ
go 80.71% <94.23%> (+0.75%) ⬆️
wasm ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
pkg/gcc/rate_calculator.go 92.85% <90.00%> (+0.17%) ⬆️
pkg/gcc/arrival_group_accumulator.go 100.00% <100.00%> (ø)
pkg/gcc/delay_based_bwe.go 95.31% <100.00%> (-0.15%) ⬇️
pkg/gcc/kalman.go 94.33% <0.00%> (-5.67%) ⬇️
pkg/gcc/send_side_bwe.go 83.20% <0.00%> (+4.58%) ⬆️
pkg/gcc/rtt_estimator.go 100.00% <0.00%> (+10.71%) ⬆️
pkg/gcc/loss_based_bwe.go 94.54% <0.00%> (+14.54%) ⬆️
pkg/gcc/noop_pacer.go 35.00% <0.00%> (+35.00%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 079fa7c...a3bbf62. Read the comment docs.

Copy link
Contributor

@mengelbart mengelbart left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, thanks @adityaa30 !

@mengelbart
Copy link
Contributor

@adityaa30 would you mind applying this patch to fix the linter errors?

diff --git a/pkg/gcc/send_side_bwe_test.go b/pkg/gcc/send_side_bwe_test.go
index a178423..4d9c191 100644
--- a/pkg/gcc/send_side_bwe_test.go
+++ b/pkg/gcc/send_side_bwe_test.go
@@ -120,15 +120,16 @@ func BenchmarkSendSideBWE_WriteRTCP(b *testing.B) {
                        arrivalTime := int64(0)
 
                        for i := 0; i < b.N; i++ {
+                               //nolint:gosec
                                seqs := rand.Intn(count/2) + count // [count, count * 1.5)
                                for j := 0; j < seqs; j++ {
                                        seq++
 
-                                       if rand.Intn(5) == 0 {
+                                       if rand.Intn(5) == 0 { //nolint:gosec,staticcheck
                                                // skip this packet
                                        }
 
-                                       arrivalTime += int64(rtcp.TypeTCCDeltaScaleFactor * (rand.Intn(128) + 1))
+                                       arrivalTime += int64(rtcp.TypeTCCDeltaScaleFactor * (rand.Intn(128) + 1)) //nolint:gosec
                                        r.Record(5000, seq, arrivalTime)
                                }
 
@@ -141,5 +142,4 @@ func BenchmarkSendSideBWE_WriteRTCP(b *testing.B) {
                        require.NoError(b, bwe.Close())
                })
        }
-
 }

@adityaa30
Copy link
Contributor Author

@mengelbart Done

@mengelbart
Copy link
Contributor

Thanks, @adityaa30. I'm not sure why the WASM tests fail, I think there's a deadlock while closing the channels, but I'm not sure if it was introduced by this change. Anyway, I included your changes in #147 which builds on another optimization (#146) that reduces the overall number of goroutines and should fix the tests.

@adityaa30
Copy link
Contributor Author

Sure @mengelbart Thanks a lot

@mengelbart
Copy link
Contributor

Merged in #147

@mengelbart mengelbart closed this Jul 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Optimise SendSideBWE.WriteRTCP
3 participants