refactor(quinn-proto): Use ArrayRangeSet in favor of RangeSet#2445
refactor(quinn-proto): Use ArrayRangeSet in favor of RangeSet#2445matheus23 wants to merge 1 commit into
ArrayRangeSet in favor of RangeSet#2445Conversation
This seems to improve performance by ~8-10% in local benchmarks for us.
djc
left a comment
There was a problem hiding this comment.
Nice and simple, and seems plausible?
|
I would hold off on merging this too eagerly. For me this improves some benchmarks by quite a bit, but others also regress. Mostly this tells me that overall CPU use seems to depend on the range set performance characteristics. |
|
Another concern that bears some thought is the hazard of a CPU DoS attack. A peer can exert substantial control over the number of ranges in these fields, and the B tree structure is less sensitive to total size, though how much this matters depends on the exact accesses we're performing. |
|
#2422 will also improve some B-tree operations, which might tilt the balance. |
Could consider an enum that tries to make it an array range set, but switches over to a btree range set if the number of ranges grows beyond some small constant? |
|
That should work, yeah, at the cost of a little more complexity and boilerplate. |
This seems to improve performance by ~8-10% in local benchmarks for me.