From f79b79f8267f7e028e9edad02e463987a2fbb8af Mon Sep 17 00:00:00 2001 From: KonradStaniec Date: Thu, 24 Mar 2022 15:56:00 +0100 Subject: [PATCH] Fix defect in uTP buffer (#493) --- eth/utp/growable_buffer.nim | 57 +++++++++++++++++++------------------ tests/utp/test_buffer.nim | 50 +++++++++++++++++++++++++++++++- 2 files changed, 78 insertions(+), 29 deletions(-) diff --git a/eth/utp/growable_buffer.nim b/eth/utp/growable_buffer.nim index 0babfaf..dfefb7a 100644 --- a/eth/utp/growable_buffer.nim +++ b/eth/utp/growable_buffer.nim @@ -19,32 +19,32 @@ export options type GrowableCircularBuffer*[A] = object items: seq[Option[A]] - mask: int + mask: uint32 # provided size will always be adjusted to next power of two -proc init*[A](T: type GrowableCircularBuffer[A], size: Natural = 16): T = - let powOfTwoSize = nextPowerOfTwo(size) +proc init*[A](T: type GrowableCircularBuffer[A], size: uint32 = 16): T = + let powOfTwoSize = nextPowerOfTwo(int(size)) T( - items: newSeq[Option[A]](size), - mask: powOfTwoSize - 1 + items: newSeq[Option[A]](powOfTwoSize), + mask: uint32(powOfTwoSize - 1) ) -proc get*[A](buff: GrowableCircularBuffer[A], i: Natural): Option[A] = +proc get*[A](buff: GrowableCircularBuffer[A], i: uint32): Option[A] = buff.items[i and buff.mask] -proc putImpl[A](buff: var GrowableCircularBuffer[A], i: Natural, elem: Option[A]) = +proc putImpl[A](buff: var GrowableCircularBuffer[A], i: uint32, elem: Option[A]) = buff.items[i and buff.mask] = elem -proc put*[A](buff: var GrowableCircularBuffer[A], i: Natural, elem: A) = +proc put*[A](buff: var GrowableCircularBuffer[A], i: uint32, elem: A) = buff.putImpl(i, some(elem)) -proc delete*[A](buff: var GrowableCircularBuffer[A], i: Natural) = +proc delete*[A](buff: var GrowableCircularBuffer[A], i: uint32) = buff.putImpl(i, none[A]()) -proc hasKey*[A](buff: GrowableCircularBuffer[A], i: Natural): bool = +proc hasKey*[A](buff: GrowableCircularBuffer[A], i: uint32): bool = buff.get(i).isSome() -proc exists*[A](buff: GrowableCircularBuffer[A], i: Natural, check: proc (x: A): bool): bool = +proc exists*[A](buff: GrowableCircularBuffer[A], i: uint32, check: proc (x: A): bool): bool = let maybeElem = buff.get(i) if (maybeElem.isSome()): let elem = maybeElem.unsafeGet() @@ -52,32 +52,33 @@ proc exists*[A](buff: GrowableCircularBuffer[A], i: Natural, check: proc (x: A): else: false -proc `[]`*[A](buff: var GrowableCircularBuffer[A], i: Natural): var A = +proc `[]`*[A](buff: var GrowableCircularBuffer[A], i: uint32): var A = ## Returns contents of the `var GrowableCircularBuffer`. If it is not set, then an exception ## is thrown. buff.items[i and buff.mask].get() proc len*[A](buff: GrowableCircularBuffer[A]): int = - buff.mask + 1 + int(buff.mask) + 1 + +proc calculateNextMask(currentMask: uint32, index:uint32): uint32 = + # Increase mask,so that index will fit in buffer size i.e mask + 1 + if currentMask == uint32.high: + return currentMask + + var newSize = currentMask + 1 + while true: + newSize = newSize * 2 + if newSize == 0 or index < newSize: + break + return newSize - 1 # Item contains the element we want to make space for # index is the index in the list. -proc ensureSize*[A](buff: var GrowableCircularBuffer[A], item: Natural, index: Natural) = - # Increase size until is next power of 2 which consists given index - proc getNextSize(currentSize: int, index: int): int = - var newSize = currentSize - while true: - newSize = newSize * 2 - if not (index >= newSize): - break - newSize - +proc ensureSize*[A](buff: var GrowableCircularBuffer[A], item: uint32, index: uint32) = if (index > buff.mask): - let currentSize = buff.mask + 1 - let newSize = getNextSize(currentSize, index) - let newMask = newSize - 1 - var newSeq = newSeq[Option[A]](newSize) - var i = 0 + let newMask = calculateNextMask(buff.mask, index) + var newSeq = newSeq[Option[A]](int(newMask) + 1) + var i = 0'u32 while i <= buff.mask: let idx = item - index + i newSeq[idx and newMask] = buff.get(idx) diff --git a/tests/utp/test_buffer.nim b/tests/utp/test_buffer.nim index 29849ba..c583d6f 100644 --- a/tests/utp/test_buffer.nim +++ b/tests/utp/test_buffer.nim @@ -22,6 +22,18 @@ suite "Utp ring buffer": buff.len() == 4 buff.get(0).isNone() + test "Buffer should be initialised to next power of two": + var elemsCounter = 0 + let buff = GrowableCircularBuffer[int].init(size = 17) + check: + buff.len() == 32 + + for i in buff.items: + inc elemsCounter + + check: + elemsCounter == 32 + test "Adding elements to buffer": var buff = GrowableCircularBuffer[int].init(size = 4) buff.put(11, 11) @@ -53,7 +65,7 @@ suite "Utp ring buffer": test "Checking if element exists and has some properties": var buff = GrowableCircularBuffer[TestObj].init(size = 4) let text = "test" - let textIdx = 11 + let textIdx = 11'u32 check: not buff.exists(textIdx, x => x.foo == text) @@ -124,3 +136,39 @@ suite "Utp ring buffer": buff.get(15) == none[int]() buff.get(16) == none[int]() buff.get(17) == some(17) + + test "Ensuring proper wrap around when adding elements to buffer": + var buff = GrowableCircularBuffer[int].init(size = 2) + + buff.ensureSize(65535, 0) + buff.put(65535, 65535) + buff.ensureSize(0, 1) + buff.put(0, 0) + + # index 2 will not fit in buffer of size 2 so it will need to be expanded to 4 + buff.ensureSize(1, 2) + buff.put(1, 1) + + check: + buff.len() == 4 + + buff.ensureSize(2, 3) + buff.put(2, 2) + # index 4 will not fit in buffer size 4 so it will need to expanded + buff.ensureSize(3, 4) + buff.put(3, 3) + + # all elements should be available thorugh old indexes + check: + buff.get(65535) == some(65535) + buff.get(0) == some(0) + buff.get(1) == some(1) + buff.get(2) == some(2) + buff.get(3) == some(3) + buff.len() == 8 + + var elemsCounter = 0 + for elem in buff.items: + inc elemsCounter + check: + elemsCounter == 8