optimize FirstOutstanding in the sent packet history

[tobyxdd]

Closes #3462

[tobyxdd]

Working on the integration test failures

[tobyxdd]

@marten-seemann I need some help 🤨 still haven't figured out the cause yet

[tobyxdd]

nvm, found the problem.

[codecov]

Codecov Report

Merging #3467 (64dfed6) into master (f29dd27) will increase coverage by 0.02%.
The diff coverage is 95.92%.

@@            Coverage Diff             @@
##           master    #3467      +/-   ##
==========================================
+ Coverage   85.66%   85.69%   +0.02%     
==========================================
  Files         137      138       +1     
  Lines       10009    10039      +30     
==========================================
+ Hits         8574     8602      +28     
- Misses       1059     1060       +1     
- Partials      376      377       +1     

Impacted Files	Coverage Δ
internal/ackhandler/sent_packet_history.go	94.81% <95.65%> (-1.03%)	⬇️
internal/ackhandler/interfaces.go	100.00% <100.00%> (ø)
internal/ackhandler/sent_packet_handler.go	78.01% <100.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 f29dd27...64dfed6. Read the comment docs.

[marten-seemann]

Thank you! This looks pretty good.

It would probably be good to have a test that Iterate iterates in the right order, when skipping between the two lists.

[tobyxdd]

It would probably be good to have a test that Iterate iterates in the right order, when skipping between the two lists.

done

[tobyxdd]

@marten-seemann the latest commit 7cedeed tries to solve #300 using AVL tree for gaps.

I've already done most of the work but there's a nasty bug somewhere in handling the start/end gaps causing the tests to fail. I spent the entire night trying to debug without any luck... maybe you can take a look?

[marten-seemann]

@marten-seemann the latest commit 7cedeed tries to solve #300 using AVL tree for gaps.

I'll have a look at this next week. OOO this week.
Can you please separate this into a separate branch / PR though? Would be good to keep this PR clean.

[tobyxdd]

I've found the problem and fixed it. Can confirm that it works perfectly in real-world applications too.

With these 2 changes I can get a stable 400-500 Mbps on $5 AWS Lightsail - more than quadrupling the performance.

I will continue to check where the new bottlenecks are and try to optimize them in another PR. Maybe we should introduce WriteBatch to batchConn?

Can you please separate this into a separate branch / PR though?

Or we can just put them together as one big optimization PR?

[marten-seemann]

Where’s that tree implementation coming from? I’m not convinced that quic-go should be in the business of implementing a tree data structure from scratch.

[marten-seemann]

Or we can just put them together as one big optimization PR?

These are two completely different issues. They need to be resolved in separate PRs.

[tobyxdd]

Or we can just put them together as one big optimization PR?

These are two completely different issues. They need to be resolved in separate PRs.

OK then. I'll create another PR

[tobyxdd]

Where’s that tree implementation coming from? I’m not convinced that quic-go should be in the business of implementing a tree data structure from scratch.

It's from https://github.com/ross-oreto/go-tree/blob/master/btree.go but I've made various modifications to meet our use case

[tobyxdd]

I’m not convinced that quic-go should be in the business of implementing a tree data structure from scratch.

Don't we have multiple linked list implementation in the code already?

[tobyxdd]

frameSorter changes removed

[marten-seemann]

frameSorter changes removed

Thanks!

Don't we have multiple linked list implementation in the code already?

We do, but they’re autogenerated from a “generic” implementation copied from the standard library: https://github.com/lucas-clemente/quic-go/tree/master/internal/utils/linkedlist. It’s not pretty, not at all. But it’s a better solution than using an untyped data structure and doing type assertions all the time.
Can’t wait to replace these with proper generics (or more suitable data structures than linked lists)!

[marten-seemann]

This looks good, thanks for your work @tobyxdd!

One tiny request for a comment, then we're ready to merge.

[marten-seemann]

Thank you!