From 62e8ea31c326bc707e80a7336e022cbb02d18955 Mon Sep 17 00:00:00 2001 From: Timothee Cour Date: Wed, 19 Feb 2020 17:48:06 -0800 Subject: [PATCH 1/4] hashing collision: use linear/pseudorandom probing mix --- lib/pure/collections/hashcommon.nim | 48 +++++++++++++++++++---------- lib/pure/collections/tables.nim | 6 ++-- 2 files changed, 34 insertions(+), 20 deletions(-) diff --git a/lib/pure/collections/hashcommon.nim b/lib/pure/collections/hashcommon.nim index bfc09ec3cc92..ab5a4a315e70 100644 --- a/lib/pure/collections/hashcommon.nim +++ b/lib/pure/collections/hashcommon.nim @@ -72,30 +72,46 @@ template getPerturb(t: typed, hc: Hash): UHash = # influence the recursion in nextTry earlier rather than later. translateBits(cast[uint](hc), numBitsMask) +template findCell(t: typed, hc, mustNextTry): int = + let m = maxHash(t) + var index: Hash = hc and m + var perturb: UHash = getPerturb(t, hc) + var depth = 0 + const depthThres = 20 # PARAM + while mustNextTry(t.data[index], index): + depth.inc + if depth <= depthThres: + index = (index + 1) and m + else: + index = nextTry(index, m, perturb) + index + template rawGetKnownHCImpl() {.dirty.} = - if t.dataLen == 0: - return -1 - var h: Hash = hc and maxHash(t) # start with real hash value - var perturb = t.getPerturb(hc) + if t.dataLen == 0: return -1 var deletedIndex = -1 - while true: - if isFilledAndValid(t.data[h].hcode): + template mustNextTry(cell, index): bool = + if isFilledAndValid(cell.hcode): # Compare hc THEN key with boolean short circuit. This makes the common case # zero ==key's for missing (e.g.inserts) and exactly one ==key for present. # It does slow down succeeding lookups by one extra Hash cmp&and..usually # just a few clock cycles, generally worth it for any non-integer-like A. # performance: we optimize this: depending on type(key), skip hc comparison - if t.data[h].hcode == hc and t.data[h].key == key: - return h - h = nextTry(h, maxHash(t), perturb) - elif t.data[h].hcode == deletedMarker: + if cell.hcode == hc and cell.key == key: + deletedIndex = -2 + false + else: + true + elif cell.hcode == deletedMarker: if deletedIndex == -1: - deletedIndex = h - h = nextTry(h, maxHash(t), perturb) - else: - break - if deletedIndex == -1: - result = -1 - h # < 0 => MISSING; insert idx = -1 - result + deletedIndex = index + true + else: false + + let index = findCell(t, hc, mustNextTry) + if deletedIndex == -2: + result = index + elif deletedIndex == -1: + result = -1 - index # < 0 => MISSING; insert idx = -1 - result else: # we prefer returning a (in fact the 1st found) deleted index result = -1 - deletedIndex diff --git a/lib/pure/collections/tables.nim b/lib/pure/collections/tables.nim index b79e396fd176..b0ffebde2a9e 100644 --- a/lib/pure/collections/tables.nim +++ b/lib/pure/collections/tables.nim @@ -272,10 +272,8 @@ proc enlarge[A, B](t: var Table[A, B]) = for i in countup(0, high(n)): let eh = n[i].hcode if isFilledAndValid(eh): - var j: Hash = eh and maxHash(t) - var perturb = t.getPerturb(eh) - while isFilled(t.data[j].hcode): - j = nextTry(j, maxHash(t), perturb) + template mustNextTry(cell, index): bool = isFilled(cell.hcode) + let j = findCell(t, eh, mustNextTry) when defined(js): rawInsert(t, t.data, n[i].key, n[i].val, eh, j) else: From ae94905e3c515058a8259fdd7969ff76dd59b4a5 Mon Sep 17 00:00:00 2001 From: Timothee Cour Date: Wed, 19 Feb 2020 20:45:30 -0800 Subject: [PATCH 2/4] fixup --- lib/pure/collections/hashcommon.nim | 31 +++++++---- lib/pure/collections/sharedtables.nim | 6 +-- lib/pure/collections/tableimpl.nim | 14 ++--- lib/pure/collections/tables.nim | 76 ++++++++++++++------------- tests/collections/ttables.nim | 34 ++++++++++++ tests/stdlib/tsharedtable.nim | 14 +++++ 6 files changed, 115 insertions(+), 60 deletions(-) diff --git a/lib/pure/collections/hashcommon.nim b/lib/pure/collections/hashcommon.nim index ab5a4a315e70..31df5171cf00 100644 --- a/lib/pure/collections/hashcommon.nim +++ b/lib/pure/collections/hashcommon.nim @@ -75,19 +75,33 @@ template getPerturb(t: typed, hc: Hash): UHash = template findCell(t: typed, hc, mustNextTry): int = let m = maxHash(t) var index: Hash = hc and m - var perturb: UHash = getPerturb(t, hc) + var perturb = getPerturb(t, hc) var depth = 0 - const depthThres = 20 # PARAM + + ## PARAM: this param can affect performance and could be exposed to users whoe + ## need to optimize for their specific key distribution. If clusters are to be + ## expected, it's better to set it low; for really random data, it's better to + ## set it high. We pick a sensible default that works across a range of key + ## distributions. + ## + ## depthThres=0 will just use pseudorandom probing + ## depthThres=int.high will just use linear probing + ## depthThres in between will switch + const depthThres = 20 + while mustNextTry(t.data[index], index): depth.inc if depth <= depthThres: + ## linear probing, cache friendly index = (index + 1) and m else: + ## pseudorandom probing, "bad" case was detected index = nextTry(index, m, perturb) index template rawGetKnownHCImpl() {.dirty.} = - if t.dataLen == 0: return -1 + if t.dataLen == 0: + return -1 var deletedIndex = -1 template mustNextTry(cell, index): bool = if isFilledAndValid(cell.hcode): @@ -97,15 +111,14 @@ template rawGetKnownHCImpl() {.dirty.} = # just a few clock cycles, generally worth it for any non-integer-like A. # performance: we optimize this: depending on type(key), skip hc comparison if cell.hcode == hc and cell.key == key: - deletedIndex = -2 + deletedIndex = -2 # found false - else: - true + else: true # keep going elif cell.hcode == deletedMarker: if deletedIndex == -1: - deletedIndex = index - true - else: false + deletedIndex = index # remember 1st tombstone found before continuing + true # keep going + else: false # not found let index = findCell(t, hc, mustNextTry) if deletedIndex == -2: diff --git a/lib/pure/collections/sharedtables.nim b/lib/pure/collections/sharedtables.nim index 27ac5e84f7e3..7eeec43f850c 100644 --- a/lib/pure/collections/sharedtables.nim +++ b/lib/pure/collections/sharedtables.nim @@ -50,10 +50,8 @@ proc enlarge[A, B](t: var SharedTable[A, B]) = for i in 0..`_. @@ -2238,45 +2239,46 @@ type # ------------------------------ helpers --------------------------------- -proc ctRawInsert[A](t: CountTable[A], data: var seq[tuple[key: A, val: int]], - key: A, val: int) = - let hc = hash(key) - var perturb = t.getPerturb(hc) - var h: Hash = hc and high(data) - while data[h].val != 0: h = nextTry(h, high(data), perturb) # TODO: handle deletedMarker - data[h].key = key - data[h].val = val +proc ctRawInsert[A](t: var CountTable[A], key: A, val: int) = + let hc = genHash(key) + template mustNextTry(cell, index): bool = cell.val != 0 + let h = findCell(t, hc, mustNextTry) + t.data[h].key = key + t.data[h].val = val proc enlarge[A](t: var CountTable[A]) = var n: seq[tuple[key: A, val: int]] newSeq(n, len(t.data) * growthFactor) - for i in countup(0, high(t.data)): - if t.data[i].val != 0: ctRawInsert(t, n, move t.data[i].key, move t.data[i].val) swap(t.data, n) + for i in countup(0, high(n)): + if n[i].val != 0: ctRawInsert(t, move n[i].key, move n[i].val) proc remove[A](t: var CountTable[A], key: A) = var n: seq[tuple[key: A, val: int]] newSeq(n, len(t.data)) + swap(t.data, n) var removed: bool - for i in countup(0, high(t.data)): - if t.data[i].val != 0: - if t.data[i].key != key: - ctRawInsert(t, n, move t.data[i].key, move t.data[i].val) + for i in countup(0, high(n)): + if n[i].val != 0: + if n[i].key != key: + ctRawInsert(t, move n[i].key, move n[i].val) else: removed = true - swap(t.data, n) if removed: dec(t.counter) proc rawGet[A](t: CountTable[A], key: A): int = if t.data.len == 0: return -1 - let hc = hash(key) - var perturb = t.getPerturb(hc) - var h: Hash = hc and high(t.data) # start with real hash value - while t.data[h].val != 0: # TODO: may need to handle t.data[h].hcode == deletedMarker? - if t.data[h].key == key: return h - h = nextTry(h, high(t.data), perturb) - result = -1 - h # < 0 => MISSING; insert idx = -1 - result + var found = false + template mustNextTry(cell, index): bool = + if cell.val == 0: false # not found + elif cell.key != key: true # keep going + else: + found = true + false + let hc = genHash(key) + let h = findCell(t, hc, mustNextTry) + result = if found: h else: -1 - h # < 0 => MISSING; insert idx = -1 - result template ctget(t, key, default: untyped): untyped = var index = rawGet(t, key) @@ -2358,7 +2360,7 @@ proc inc*[A](t: var CountTable[A], key: A, val: Positive = 1) = var index = rawGet(t, key) if index >= 0: inc(t.data[index].val, val) - if t.data[index].val == 0: dec(t.counter) + if t.data[index].val == 0: dec(t.counter) # how could this happen? else: insertImpl() diff --git a/tests/collections/ttables.nim b/tests/collections/ttables.nim index 9b7506d1a33c..6dc8821b187a 100644 --- a/tests/collections/ttables.nim +++ b/tests/collections/ttables.nim @@ -436,3 +436,37 @@ block: # https://github.com/nim-lang/Nim/issues/13496 testDel(): (let t = newOrderedTable[int, int]()) testDel(): (var t: CountTable[int]) testDel(): (let t = newCountTable[int]()) + +block: + # check CountTable using Table as groundtruth + proc main() = + var t: CountTable[string] + var t0: Table[string, int] + let n = 1000 + let n1 = n div 2 + template inc2(key: string, val = 1) = + t.inc(key, val) + if key in t0: t0[key]+=val else: t0[key]=val + + template del2(key) = + t.del(key) + t0.del(key) + + template check() = + for k,v in t: + doAssert t0[k] == v + for k,v in t0: + doAssert t[k] == v + + let m = 2 + for j in 0.. Date: Thu, 5 Mar 2020 19:17:45 -0800 Subject: [PATCH 3/4] fix test --- tests/stdlib/tsharedtable.nim | 19 +++++-------------- 1 file changed, 5 insertions(+), 14 deletions(-) diff --git a/tests/stdlib/tsharedtable.nim b/tests/stdlib/tsharedtable.nim index a6e1d7df8f08..93428615b7cc 100644 --- a/tests/stdlib/tsharedtable.nim +++ b/tests/stdlib/tsharedtable.nim @@ -66,10 +66,15 @@ block: # we use Table as groundtruth, it's well tested elsewhere doAssert t.mgetOrPut(i, -2) == -2 doAssert t.mget(i) == -2 + var numOK = 0 for i in 0.. 0 + doAssert numOK < n4 checkEquals() @@ -87,17 +92,3 @@ block: # we use Table as groundtruth, it's well tested elsewhere var t0: Table[int, int] testDel(t, t0) deinitSharedTable(t) - -block: # CHECKME:redundant w above? - let n = 100 - let n2 = n * 2 - for i in 0.. Date: Wed, 18 Mar 2020 02:27:05 -0700 Subject: [PATCH 4/4] address comments --- lib/pure/collections/hashcommon.nim | 22 +++++++++++----------- lib/pure/collections/tables.nim | 9 +++++---- 2 files changed, 16 insertions(+), 15 deletions(-) diff --git a/lib/pure/collections/hashcommon.nim b/lib/pure/collections/hashcommon.nim index 31df5171cf00..e591c6491b3f 100644 --- a/lib/pure/collections/hashcommon.nim +++ b/lib/pure/collections/hashcommon.nim @@ -78,24 +78,24 @@ template findCell(t: typed, hc, mustNextTry): int = var perturb = getPerturb(t, hc) var depth = 0 - ## PARAM: this param can affect performance and could be exposed to users whoe - ## need to optimize for their specific key distribution. If clusters are to be - ## expected, it's better to set it low; for really random data, it's better to - ## set it high. We pick a sensible default that works across a range of key - ## distributions. - ## - ## depthThres=0 will just use pseudorandom probing - ## depthThres=int.high will just use linear probing - ## depthThres in between will switch + # PARAM: `depthThres` can affect performance and could be exposed to users who + # need to optimize for their specific key distribution. If clusters are to be + # expected, it's better to set it low; for really random data, it's better to + # set it high. We pick a sensible default that works across a range of key + # distributions. + # + # depthThres=0 will just use pseudorandom probing + # depthThres=int.high will just use linear probing + # depthThres in between will switch const depthThres = 20 while mustNextTry(t.data[index], index): depth.inc if depth <= depthThres: - ## linear probing, cache friendly + # linear probing, cache friendly index = (index + 1) and m else: - ## pseudorandom probing, "bad" case was detected + # pseudorandom probing, "bad" case was detected index = nextTry(index, m, perturb) index diff --git a/lib/pure/collections/tables.nim b/lib/pure/collections/tables.nim index b464fd2be5f5..890ab5892f6f 100644 --- a/lib/pure/collections/tables.nim +++ b/lib/pure/collections/tables.nim @@ -2226,9 +2226,10 @@ type counter: int countDeleted: int # using this and creating tombstones as in `Table` would make `remove` - # amortized O(1) instead of O(n); this requires updating remove, - # and changing `val != 0` to what's used in Table. Or simply make CountTable - # a thin wrapper around Table (which would also enable `dec`, etc) + # amortized O(1) instead of O(n); this requires updating `remove`, + # and changing `val != 0` to what's used in Table. Or we could simply make + # CountTable a thin wrapper around Table (which would also enable lifting + # restrictions on CountTable, eg lack of `dec`, allowing `0` counts, etc). isSorted: bool CountTableRef*[A] = ref CountTable[A] ## Ref version of ## `CountTable<#CountTable>`_. @@ -2360,7 +2361,7 @@ proc inc*[A](t: var CountTable[A], key: A, val: Positive = 1) = var index = rawGet(t, key) if index >= 0: inc(t.data[index].val, val) - if t.data[index].val == 0: dec(t.counter) # how could this happen? + assert t.data[index].val != 0 else: insertImpl()