Fix #14971 OrderedTable del and pop are slow

[hashbackup]

• OrderedTable deletes were O(n),now are O(1)
• singly-linked list changed to doubly-linked
• new table is no longer created on del
• uses about 40% less memory if del is used
• uses 8 bytes more per entry for back link
• added itemMoved callback when backshifting after delete
• itemMoved is a no-op on all but OrderedTable
• for OrderedTable, itemMoved adjusts the linked list
• added new tests
• modified delImplIdx to maybe be clearer

[hashbackup]

Notes about the change

Existing OrderedTable

• O(n) to delete 1 item
• takes 0.001s to add 100K items
• takes 60s to del 100K items
• other modules take 0.01-0.04s to delete 100K items
• doubles memory use after 1 delete

Possible solutions

1. Existing code is problematic because
of backshifting on delete in Table - a generally
good thing, but not for how OT was designed.

a) keep existing slow code, document that it's slow
b) disable deletes to avoid performance issues
c) switch to doubly-linked list, adjust it when
backshifting on deletes in Table (chosen)

Pros:

• changes more confined to del
• existing Table code is fastest to create tables
• no external dependencies
• new design is faster than alternatives

Cons:

• uses more memory than CompactDict
• space increase objections because of compiler / JSON

2. LruCache uses a doubly-linked list design not
affected by backshifting.

Pros:

• small amount of code: < 100 lines
• takes 0.003s to delete 100K items vs 60s for existing
• uses Nim Table and DoublyLinkedList
• scales well (linearly)
• smaller than existing OT if deletes are used

Cons:

• 15x slower to create 100K item table: .02s vs .0013s
• 30% larger than existing if deletes are never used

3. CompactDict based on Python dict

Pros:

• smallest storage per item
• 40% smaller than existing if no deletes
• 60% smaller than existing with deletes
• scales well (linearly)
• deletes are 2x faster than inserts
• maintains order when deleting

Cons:

• 300 lines of unique code
• does not use existing Table code
• random probe sequence is not cache friendly
• 7x slower to create 100K item table: .0071s vs .0013s

4. OLTab by @c-blake http://github.com/c-blake/adix

Pros:

• scales linearly
• drop-in compatible with Nim's tables, like Compactdict
• 7 per-table options to control capacity, max probe depth, min free slots, growth factor
• detailed statistics for users to understand performance

Cons:

• lots of new code if it is added to stdlib instead of replacing some existing table code
• does not guarantee to maintain order on deletes

Performance scaling for new OrderedTable and alternatives
The performance tests below create a large insert-ordered table[int, int] then delete every item added. Times are in seconds

Type	Items	Time	Ratio	MB	Ratio
OrderedTable	1M	0.23		84
OrderedTable	2M	0.48	2.1	168	2
OrderedTable	4M	0.97	2	336	2
OrderedTable	8M	1.96	2	672	2
LruCache	1M	0.40		100
LruCache	2M	0.82	2	199	2
LruCache	4M	1.64	2	397	2
LruCache	8M	3.42	2.1	793	2
Compact	1M	0.25		52
Compact	2M	0.60	2.4	116	2.2
Compact	4M	1.24	2	238	2
Compact	8M	2.74	2.2	396	1.6
OLTab	1M	0.27		65
OLTab	2M	0.55	2	130	2
OLtab	4M	1.04	1.9	242	1.9
OLtab	8M	2.7	2.6	517	2.1

Performance comparison by # items

Type	Items	Time	MB
OrderedTable	1M	0.23	84
LruCache		0.40	100
Compact		0.25	52
OLTab		0.27	65
OrderedTable	2M	0.48	168
LruCache		0.82	199
Compact		0.60	116
OLTab		0.55	130
OrderedTable	4M	0.97	336
LruCache		1.64	397
Compact		1.24	238
OLtab		1.04	242
OrderedTable	8M	1.96	672
LruCache		3.42	793
Compact		2.74	396
OLtab		2.7	517

[Araq]

Sorry, -1 from me.

del is clearly documented as O(N) and most code out there does not call del at all. In fact, deletion is generally much more uncommon than insertions. So in the end for most programs the 8 bytes per entry are worse than the hypothetical O(N) vs O(1) for an uncommon operation. This is espcially troublesome as json.nim uses OrderedTable in its implementation and many applications nowadays process enormous amounts of JSON.

I would really prefer to see a Python-like compact table implementation instead.

[hashbackup]

Background on how this PR came about:

tiny_sqlite recently added a small 100-item cache for prepared statements, using OrderedDict as a fifo. Python uses an LRU cache. So as an experiment I made a quick hack to simulate LRU: on a successful hit to the OrderedDict cache, it deleted the item and re-inserted it. It worked, but increased runtime from 16s to 22s for 7M queries. Using LruCache instead, runtime was about 16.3s, so the tiny_sqlite author added LruCache to tiny_sqlite. With the OrderedDict changes in this PR, tiny_sqlite runtime is 16.3s, like LruCache.

I'm curious myself about how these different hash tables compare when creating lots of smaller tables and will post test results today.

[hashbackup]

This test creates a small 1K-entry table 1M times to see how different dict/table implementations perform. No deletes. Compiler is 1.3 with --gc:arc unless otherwise listed.

when defined(compact):
  import compactdict
when defined(lru):
  import lrucache
else:
  import tables
import times

const
  loops = 1_000_000
  els = 1000

var t0 = cpuTime()

for j in 0 ..< loops:
  when defined(table):
    var ot = initTable[int, int](els)
  when defined(ordered):
    var ot = initOrderedTable[int, int](els)
  when defined(lru):
    var ot = newLruCache[int, int](els)
  when defined(compact):
    var ot = initDict[int, int](els)

  for i in 0 ..< els:
    ot[i] = i

echo "Time: ", cpuTime() - t0

See note at the end on how table sizes were computed.

Type	Time	Memory	Options
Table	13.5s	53305
Ordered	14.6s	69708
Ordered (PR)	16.9s	86094
LruCache	90.7s	102105
Compact	38s	8267	had to disable =destroy=
OLTab	21.5s	8270	1024 slots
OLTab	17.3s	12366	2048 slots

I'm not exactly sure what to make of these results; here are my thoughts:

The existing OrderedTable is 31% larger than Table. This makes sense because Table has 2 integers per entry, OT has 3 (the link for ordering). This PR adds a back link to each entry so is 25% larger than the existing OT and 16% slower since it has to update 2 links instead of just 1. One possibility is to use 1/2/4/8-byte integers for the link fields, saving space but limiting the number of items.

The LruCache results are puzzling because it is very simple and uses Nim's Table and DoublyLinkedList. From what I can tell there is no big initialization step when a new LruCache is created, and while LruCache was 2x slower than OrderedTable in large table tests, I can't explain why it is 5x slower in a small table test. Maybe @jackhftang can shed some light on this. OrderedTable has a good design in that the list is embedded in the hash table entries. So to add a new item, there is a memory access to store the item in the table, plus two to update the list. LruCache's DoublyLinkedListlist is not part of the table, which helps explaining why it is 2x slower, but doesn't explain 5x for the small table test.

In large table tests, Compactdict did very well on memory usage and while slower than Nim's tables, it wasn't by a lot. In the large table test with 1M items, it had the same runtime as OrderedTable but used less memory. But in the small table test, Compactdict at 38s is 2x as slow as OrderedTable. I haven't reviewed the Compactdict code; it may be slow to create new tables, explaining the difference between small and large table tests. I had to disable =destroy= or it SEGV'd. It also didn't work well with --gc:arc, using 8G of RAM, ie, nothing ever got freed I think. It certainly creates small dictionaries for this test.

OLTab from @c-blake creates very small tables like Compactdict but is much faster and doesn't have problems with --gc:arc. Combined with being actively involved here, it would be better IMO to use his OLTab for a compact table vs Compactdict. I will try to add large table test results for OLTab. A couple of outstanding questions: 1) does OLTab support duplicate keys, and do we care if it doesn't? 2) does it maintain order over deletes? (Python's compactdict does, not sure about Nim Compactdict)

Small table sizes: because these tables are small, it doesn't work to use OS RSS numbers. Instead, a separate test was run that creates 10K tables and stores them in an array. The OS RSS is collected, 640000 is subtracted (minimal Nim program), 160000 is subtracted for the array, then divide by 10K.

[c-blake]

I did a compact, insertion-ordered table over at https://github.com/c-blake/adix/blob/master/adix/olset.nim. The first 250 lines are the algorithm, the last part is more the Nim stdlib sets/tables like API. It could be simpler, but it does both Robin Hood and regular linear probing on the "hash table part". It could also be more optimized as mentioned in some comments there.

I agree if we also provide a CompactTable then it matters much less whether every operation on OrderedTable is efficient, as long as that is clearly documented. Maybe emphasizing O(n) *per call* in the documentation would be more clear?

Undiscussed so far, but related to @Araq's complaint about most client code not using deletion, is whether insertion order needs to be maintained once deletions are in the mix. The doubly linked list approach of course lets one do that since order is completely independent of the hash layout. However, the "easy" way to have an efficient delete in the "compact"-style is to swap the delete victim with the very end of the dense seq (updating the "hash part") and then just shrink that seq by 1. This maintains density of the dense seq, but disrupts the order of that dense seq relative to insert order. While keeping that order is possible with complexity like "holes" or other contortions, that may also be a "software contract not worth signing". Or maybe it is? I'd prefer not to "sign it" without strong motivation, personally.

Another related but wholly distinct question is whether OrderedSet or CompactSet would be useful. The way I structured my stuff in adix, I get tables from sets very easily and just doing the full Cartesian Product is easy, but I think that layering is a little slower than how, say, CritBitTree works with its optionally void generic "value" parameter (of course that only has string keys which is slightly different).

[hashbackup]

Another difference I should mention: the old OT code deleted all duplicates of a key. The new code only deletes the first item with that key and a loop is needed to delete all duplicates. This was changed for 2 reasons:

• the regular hash table only deletes one item per call
• deleting duplicates slows down every successful delete because you have to do another search to see if there's a duplicate

Duplicates in a hash table are not a great idea, but I think they are used in the compiler. It may be that special tables for the compiler, JSON, etc. would make sense, with OrderedTable having good all-around performance for general use but maybe not as fast or small as more specialized tables for specialized uses where deletes never occur, table sizes are limited, dups are allowed, etc.

[c-blake]

In the past, others have found stdlib's multi-key semantics (e.g. nim-lang/RFCs#200) confusing. I agree having OrderedTable just like Table except that "batch vs single" delete is kind of a landmine/footgun. It looks like that could be fixed with a simple break or similar, though.

The way OrderedTable.del is written now is barely better than some impl-unaware start from scratch and insert all key != deleteTarget. The current way doesn't allocate another block of memory/have to do hash-pathway lookups. I guess that's something.

It's surprising enough that if we don't do something like this PR then the documentation should really hammer the reader over the head with the fact..Italics/emphasis/really spelled out. This PR shows there is nothing preventing efficiency in principle. Most people just "assume" that in a fast language that the efficient thing will be done, not that a general purpose OrderedTable will have operations whose efficiency is held hostage to memory optimization for special use cases in JSON operations. So, if we go that way, to overcome that strong assumption, more explanation than "O(n) complexity" is needed, IMO. But I will stop repeating myself on this.

Another possibility is using 4 byte pointers instead of 8 byte pointers so the double links take up the same space. That would limit us to 4 GiEntry OrderedTable, but that may be plenty. Or a maybe a new generic parameter to set the integer size of links defaulting to 4, so only super-size-er code needs to alter it?

Also/or maybe @hashbackup should do some JSON benchmarking with his version? That extra 8 bytes per slot could be pretty small in a JSON context with all the string data flying around. Cache sizes and the timing impact may all vary, but the memory impact should be pretty similar across deployment platforms.

Just trying to provide constructive feedback. I think CompactTable is probably the best way to go to maintan insertion order as long as we don't promise any such order with del ops in the mix.

[hashbackup]

I updated the memory sizes on the 2nd set of tests for small tables and added OLTab from c-blake's adix collection.

[Araq]

While keeping that order is possible with complexity like "holes" or other contortions, that may also be a "software contract not worth signing". Or maybe it is? I'd prefer not to "sign it" without strong motivation, personally.

This is a good point and Python's del is order preserving, making it O(N) too. (!!!)

[hashbackup]

I don't understand this:

This is a good point and Python's del is order preserving, making it O(N) too. (!!!)

If you look at #14971, Python adds and deletes 100K items from an ordered dictionary in 0.08s, whereas it takes OrderedTable 60s to do the same thing. Ie, the existing OrderedTable is 750x slower than Python @ 100K items and becomes quickly unusable above that. The existing OT is O(n) to delete one item; it's O(m*n) to delete m items. Python is nothing like this.

[Araq]

He, good point. I stand corrected. Should study its source code.

[planetis-m]

Python does the "fast"del function where it replaces the hole with the last item.

[hashbackup]

I've been messing with the table code to see about changing links to different sizes. I noticed there is a sort function for OrderedTables that reorders items by key rather than insertion order and that would have to be updated for this PR. Changed it to a draft.

[c-blake]

In CPython Objects/dictobject.c:delitem_common, deletion puts a tombstone (DKIX_DUMMY) into the index part and just a "hole" into the dense sequence (NULL key/val). The location of said hole is not registered anywhere. So, wasted space/sparsity in the dense array part of the two-level structure must grow without bound (unless they do some O(nEntries) re-org, such as when there are too many tombstones, for example).

It seems kind of a stretched argument to me to say that wasted space (and time iterating) is better than permuting the order during deletes..unless as I've already mentioned, there is an important algorithm that needs this kind of mouthful property of "insert order, ignoring deletes along the way except to remove the items". Of course, my alternative is a little bit of a mouthful, too, "insertion ordered unless you delete in which case all bets are off". ;-)

To make these claims less abstract, here:

import time

d = dict()

t0 = time.time()
for i in range(10_000_000): d[i] = 2
print(time.time() - t0, "to build")

t0 = time.time()
sum = 0
for i in range(10_000_000): pass #XXX this should be `i in d.keys()`
print(time.time() - t0, "to iterate")

t0 = time.time()
for i in range(10_000_000):
    if i % 1000 != 0: del d[i] # delete 99.9%
print(time.time() - t0, "to delete 99.9%")

t0 = time.time()
sum = 0
for k in d.keys(): pass
dt = time.time() - t0
print(dt, "to iterate over 0.1%")
print(dt*1000, "implied time to iterate w/hostile keys")

I get these results:

1.0783791542053223 to build
0.3181750774383545 to iterate
1.3579120635986328 to delete 99.9%
0.010917901992797852 to iterate over 0.1%
10.917901992797852 implied time to iterate w/hostile keys

Instead of multiplying by 1000, I could also have calculated the amortized "time per iteration" by dividing by len(d). but the answer is the same - 10.918/0.318 = 34x slower than you might expect for that post-delete iteration. Other Python slowness probably masks some of the algorithmic problem here (as well as the fact that memory latency limits are worse than memory bandwidth).

I also don't see any "counter" of the DKIX_DUMMY tombstones..only "used slots". So, I doubt they do the right thing there which is to rehash into a fresh table of the appropriate size. They may block an infinite loop from an almost-all-tombstone table, but I doubt they block performance degradation, the way done in glib hash tables, for example. It's a suboptimal hash table implementation in several ways not something to be imitated, IMO.

[hashbackup]

I'm confused again. It happens! :-) When you did this:

for i in range(10_000_000): pass
print(time.time() - t0, "to iterate")

I thought it was going to be so you could subtract out Python's null loop iteration time. Did you mean to do for k in d.keys(): pass after building the table?

[c-blake]

Apologies I started from a sum += d[i] benchmark that I reduced to simply pass. Anyway, the effect is the same, but worse. Changing that time to iterate loop to for i in d.keys(): pass gives:

1.06075119972229 to build
0.22281384468078613 to iterate
1.3322453498840332 to delete 99.9%
0.010533809661865234 to iterate over 0.1%
10.533809661865234 implied time to iterate w/hostile keys

So, now it's 10.5338/0.2228 = 47x slower than you might expect, not 34x.

[c-blake]

Anyway, the point was "What's more important? Iteration performance or insertion-order with deletes in the mix?" Beats me. Python tilts one way, but we could tilt the other, unless someone gives strong reasons why with deletes in the mix pure insertion order is what you really need. And, of course, all that extra space is not just iteration performance. It's memory occupancy, fitting in caches, and performance on other table operations as well, as with the tombstone stuff.

[c-blake]

Another issue with "just copying" Python stuff is that they have sentinel valus (NULL) for objs and Nim does not. So, while they can mark the deleted dense seq entry NULL, a Nim version would need a bit vector adding memory, indirection, and complexity (or some equivalent equally complex thing).

[Clyybber]

One simple thing we could do to alleviate the added memory usage is to make prev and next int32 and make them relative indices. scratch that, better to have a hard limit than fail randomly

[hashbackup]

@Clyybber I'm looking at that now. The typing is not always consistent in tables.nim: there needs to be a distinction (I think) between hash codes and hash indexes aka slot numbers, and they're kinda jumbled up right now. They are already indexes, not pointers. I don't quite get what you mean about relative. There is no spatial relationship between items in a hash table, and in fact, the point is to get items evenly distributed over the entire table. If we could have 1/2/4/8-byte prev/next pointers and do a table resize at 256/64K/4B entries, that might be interesting.

This is just my opinion, but if a data structure says it maintains insertion order, I think it should do that without caveats. So if deletes are going to get it out of order, and probably only some deletes will do that, then it shouldn't support deletes at all.

Likewise, if 100K items can be inserted into a data structure in 1/1000th of a second, I don't think it's reasonable for it to take 60s to delete them.

IMO, stdlib services should be well-rounded in all areas that they support. It looks good in documentation to say that a table supports deletes, and people generally expect that functionality for a table/dict and would find it odd for it to be missing, but it should also be documented that for 100K items, those deletes are 60000x slower than inserts, and it gets much much worse if the table is bigger.

Again, just my opinion, but to me, balanced performance in all areas of function, with reasonable CPU time and memory usage, are more important than having a race car for 2 features but the car blows up if you use the 3rd feature.

[c-blake]

My guess is that @hashbackup hasn't yet learned about seq.delete { and also won't like it. ;-) }

We could also provide a separate CompactTable.delete proc the same way seq has a "fast del" but "slow delete" with the latter being O(n) for both seq and CompactTable. Indeed, the proc for OrderedTable deletes should arguably always have been called delete instead of del to be consistent with the seq pattern.

I don't think available but slow deletes are bad as long as they're very clearly documented and there is compensating performance argument like inserts are faster, iteration is faster, memory is less, etc. There are not too many "perfectly balanced" data structures out there...

[hashbackup]

I do get that a sequential data structure like an array or list / seq is going to be slower deleting items (or copying over them) at the beginning rather than the end. The thing that is striking to me about OrderedTable now is that it isn't just slower, it's crazy slower.

Here's a Python program that builds a list and deletes all the items from last to first:

l = []
for i in xrange(100000):
    l.append(i)
for i in xrange(100000):
    l.pop()

ms:nx jim$ /usr/bin/time -l py list.py
        0.05 real         0.04 user         0.00 sys
   8765440  maximum resident set size

Same thing but deleting items from first to last:

l = []
for i in xrange(100000):
    l.append(i)
for i in xrange(100000):
    l.pop(0)

ms:nx jim$ /usr/bin/time -l py list.py
        1.91 real         1.90 user         0.00 sys
   9027584  maximum resident set size

Sure, there is a difference in performance of 38x. But if it were like OrderedTable, a difference of 60000x, then it would take 50 minutes to delete list items from front to back. People might actually, you know, complain about that.

I'm going to bow out on this one. I enjoy programming, but trying to get consensus on this seems like it will be difficult, and honestly, I don't care enough about the fate of OrderedTable to put in this much non-programming effort.

[hashbackup]

Updated "Notes about the change" to include large table test results for OLTab and Pros/Cons

[hashbackup]

@Clyybber I did some work on separating hash code and hash index types, to allow using smaller integers for either/both. Needed about 25 changes to tables, tableimpl, hashes, hashcommon. It compiles and runs, but incorrectly.

It might make sense to reduce all of these to int32, because that would allow 2B items to be hashed, which is quite a lot. A new 64-bit version like HugeTable could be added for >2B items.

The existing code uses hash code 0 as an "empty slot" marker, so when a key hashes to zero, it gets changed to a random constant. I changed the constant to 3 to handle tiny tables < 256 elements.

It also uses negative indexes for first/last/prev/next as "null" pointers. Negative indexes are a problem because they cut the available slots by half, ie, you'd have to use int16 instead of uint16, so can only store 32767 items instead of 65535. Like 0 is a reserved hash code, we also need a reserved hash index to act like null. I'd suggest high(HashIdx), because there is already a rule that LP tables always need at least one empty slot for unsuccesful searches to terminate. This could be handled along with zero as a reserved hash code and changed to 5/7/whatever. Then high(HashIdx) can be used as a "null" for the list.

The other problem with these changes is that other table implementations like sharedTable and even LemonBoy's Compactdict, import and use some of these table procedures. They will no longer compile without changes. I've proven that with Compactdict: it only works if types Hash and HashIdx (new) are int. I don't know if tableimpl was supposed to be public or not, but there ya go.

If these changes worked I'd commit them to this branch for posterity in case anyone wanted to take up the torch later and run them through the GH gauntlet. But there are problems and I'm not planning to continue with them.

rawGet & friends also return negative numbers to indicate the key isn't present and encode the target slot as a negative integer. These would have to be changed to return a tuple: the unsigned slot and a bool for exists vs missing.

I have a diff for these changes. If anyone wants it shoot me an email.

[hashbackup]

Apologies to @c-blake, the numbers for large table OLTab were wrong because of table changes in my directory. New numbers are posted and OLTab looks very competitive and creates tables smaller than everything except Compactdict. In a side-by-side test of these two, running all 4 tests one after another and including iterating over 1% of the table after 99% of items were deleted, OLTab takes 4.57s and uses 642M while Compactdict takes 4.59s and uses 455M of RAM.

However, Compactdict did not do very well in the small 1000-item table test whereas OLTab did, and Compactdict has some --gc:arc problem that OLTab doesn't.

[hashbackup]

There's a JSON benchmark at https://github.com/kostya/benchmarks#json, the first result in a Google search for nim json benchmark. Since it was mentioned as a concern, I was curious how much OrderedTable affected a JSON benchmark. In this benchmark, the maximum items added to a table is 4 and json.nim creates tables with this size.

The rankings on this page are sorted by CPU time and memory use is not given much, if any, importance. The C++ json-c test uses 1.6G, has an energy rating of 36, and is shown higher than the Haskell test, which uses only 9.58M and also has an energy rating of 36.

Profiling with the original OT shows this at the top:

total executions of each stack trace:
Entry: 1/148 Calls: 271/1642 = 16.50% [sum: 271; 271/1642 = 16.50%]
parsejson.nim: parseNumber 288/1642 = 17.54%
parsejson.nim: getTok 855/1642 = 52.07%
parsejson.nim: eat 454/1642 = 27.65%
json.nim: parseJson 1454/1642 = 88.55%

The first time the table module shows up is here:

Entry: 18/148 Calls: 18/1642 = 1.10% [sum: 1051; 1051/1642 = 64.01%]
refs_v2.nim: nimDecRefIsLast 144/1642 = 8.77%
tables.nim: rawInsert 25/1642 = 1.52%
tables.nim: []= 141/1642 = 8.59%
json.nim: []= 141/1642 = 8.59%
json.nim: parseJson 1454/1642 = 88.55%

Test results with different tables:

• existing OrderedTable

s:orig jim$ /usr/bin/time -l ./test
(x: 0.5004211581104818, y: 0.4996713815680409, z: 0.500270481798719)
3.05 real 2.53 user 0.50 sys
1341800448 maximum resident set size

• new OrderedTable

ms:orig jim$ /usr/bin/time -l ./test
(x: 0.5004211581104818, y: 0.4996713815680409, z: 0.500270481798719)
3.15 real 2.58 user 0.56 sys
1480757248 maximum resident set size

• new OrderedTable
• int32 hash code and index (max 2B entries)

ms:json jim$ /usr/bin/time -l ./test
(x: 0.5004211581104818, y: 0.4996713815680409, z: 0.500270481798719)
2.66 real 2.31 user 0.34 sys
875466752 maximum resident set size

• new OrderedTable
• int8 hash code and index (max 128 entries)

ms:json jim$ /usr/bin/time -l ./test
(x: 0.5004211581104818, y: 0.4996713815680409, z: 0.500270481798719)
2.60 real 2.28 user 0.31 sys
787873792 maximum resident set size

• OLTab from @c-blake

ms:oltab jim$ /usr/bin/time -l ./test
(x: 0.5004211581104818, y: 0.4996713815680409, z: 0.500270481798719)
2.88 real 2.47 user 0.40 sys
1007480832 maximum resident set size

Profiling with the int32 hash code and index, the first mention of tables shows:

Entry: 19/151 Calls: 19/1593 = 1.19% [sum: 1035; 1035/1593 = 64.97%]
hashes.nim: murmurHash 49/1593 = 3.08%
hashes.nim: hash 68/1593 = 4.27%
tables.nim: rawGet 120/1593 = 7.53%
tables.nim: []= 164/1593 = 10.30%
json.nim: []= 164/1593 = 10.30%
json.nim: parseJson 1414/1593 = 88.76%

No idea why the tests I ran use more memory than the tests they published, unless it's a platform thing.

At least for this benchmark, a better ranking for Nim is going to need faster parsing (89% of time), and the table code isn't much of a factor.

[hashbackup]

The reason the first 2 tests are using more memory than posted on the benchmark site is because of the recent rightSize change. json.nim is creating tables with 4, which is getting modified to 4*3/2 + 4 = 10 -> 16. This is sort of a special case because the tables are so tiny - 4 entries each, so there are 12 entries wasted, ie, 75%. I don't understand the +4 part, except if 1 is passed for a table size, 1*3/2 would be 1, which isn't big enough to insert 1 item (need a vacant slot). So I could understand + 1 there. That also would have created 8-slot tables vs 16.

Changing json.nim to pass 2 instead of 4 gives 2*3/2 + 4 = 7 -> 8, so only 50% wasted (and 1 vacant slot is always necessary for search misses). Somebody might want to change that in json.nim, or change the +4 to something lower in hashcommon.nim

[c-blake]

NOTE: if anyone changes that new slotsNeeded to be more tiny table friendly, be sure to check it is still the proper kind of inverse-mustRehash.

[hashbackup]

An easy way to get smaller (but slower) ordered tables is to use a higher load factor. There's no way to do that today, but an option loadFactor could be added to initOrderedTable and others. Here are some figures with the earlier 8M test and OrderedTable with the added back link:

Type	Items	Time	MB
OrderedTable LF=66%	8M	1.96	672
OrderedTable LF=96%		3.04	336
OrderedTable LF=96% int32 links		3.17	269
LruCache		3.42	793
Compact LF=66%		2.74	396
Compact LF=96%		7.14	367
OLtab LF=66%		2.7	517
OLtab LF=96%		6.14	416
OLtab LF=96% robinHood=true		4.64	416

Edit: add robinHood=true time and noting that there are no misses in this test. I'll try to look into the space issue. Not sure how to do it inside Nim.

[c-blake]

You probably want to pass robinHood=true to the initOLTab call if you are also passing a numer/denom targeted at 96%. (Separate times for hit & miss searches are also informative since they tend to be quite different.)

[c-blake]

Also, there are remaining optimizations that can be done in OLTab both in space, via my sequint.nim module, and time via saving 7 bits of hash code side-by-side with the index.

That said, I'm also not sure why it would be using as much space as it is in your results table compared to other tables. The space is in a pair of seqs rather than one seq with 8B bigger elements, but otherwise it should be less space by about 4B * len(the hash table part). Could be memory allocator padding. Might be useful to use Nim's internal accounting of space used here if there is one separate from total allocation from the OS...

[hashbackup]

I took a look at Compactdict today. Didn't realize how simple it is. But that also causes problems with entries not being reused. If the "populate table, delete all items" test is looped a few times, it looks like this for Compactdict vs OrderedTable (with --gc:arc):

ms:work1 jim$ /usr/bin/time -l ./comptable2
len = 8000000, maxmem=465354752, seconds to create table: 1.622628
Seconds to delete table: 1.112567
len = 8000000, maxmem=658292736, seconds to create table: 2.278949
Seconds to delete table: 1.014376
len = 8000000, maxmem=748470272, seconds to create table: 2.094339
Seconds to delete table: 1.009979999999999
len = 8000000, maxmem=748470272, seconds to create table: 2.053436000000001
Seconds to delete table: 1.013033
len = 8000000, maxmem=748474368, seconds to create table: 2.063143
Seconds to delete table: 1.013413
       15.37 real        15.08 user         0.28 sys
 746242048  maximum resident set size


ms:work1 jim$ /usr/bin/time -l ./ordtable2
len = 8000000, maxmem=688394240, seconds to create table: 0.57785
Seconds to delete table: 0.9042819999999999
len = 0
len = 8000000, maxmem=688394240, seconds to create table: 0.5671719999999998
Seconds to delete table: 0.9041450000000002
len = 0
len = 8000000, maxmem=688394240, seconds to create table: 0.5663990000000001
Seconds to delete table: 0.9038050000000002
len = 0
len = 8000000, maxmem=688394240, seconds to create table: 0.5664249999999997
Seconds to delete table: 0.9050409999999998
len = 0
len = 8000000, maxmem=688394240, seconds to create table: 0.5659830000000001
Seconds to delete table: 0.9118060000000003
len = 0
        7.71 real         7.49 user         0.21 sys
 671670272  maximum resident set size

It's even worse for Compactdict if only half of the items are deleted: 2x slower and uses 66% more memory. For OrderedTable, it's faster with no memory increase:

ms:work1 jim$ /usr/bin/time -l ./comptable3
len = 8000000, maxmem=465354752, seconds to create table: 1.470346
Seconds to delete table: 0.4403750000000002
len = 8000000, maxmem=824299520, seconds to create table: 2.663436
Seconds to delete table: 0.7054840000000002
len = 8000000, maxmem=954322944, seconds to create table: 1.673875
Seconds to delete table: 1.253536
len = 8000000, maxmem=954322944, seconds to create table: 1.870457
Seconds to delete table: 1.38885
len = 8000000, maxmem=1151455232, seconds to create table: 2.505291999999999
Seconds to delete table: 1.751953
       15.88 real        15.46 user         0.40 sys
1115672576  maximum resident set size


ms:work1 jim$ /usr/bin/time -l ./ordtable3
len = 8000000, maxmem=688394240, seconds to create table: 0.575
Seconds to delete table: 0.4436239999999999
len = 4000000
len = 8000000, maxmem=688394240, seconds to create table: 0.5042720000000001
Seconds to delete table: 0.4963829999999998
len = 4000000
len = 8000000, maxmem=688394240, seconds to create table: 0.545007
Seconds to delete table: 0.4962599999999999
len = 4000000
len = 8000000, maxmem=688394240, seconds to create table: 0.5445980000000001
Seconds to delete table: 0.5002570000000004
len = 4000000
len = 8000000, maxmem=688394240, seconds to create table: 0.547193
Seconds to delete table: 0.4711119999999998
len = 4000000
        5.47 real         5.24 user         0.22 sys
 671690752  maximum resident set size

OLTab scales well on these tests. First is populating table and deleting all items 5x, 2nd is only deleting half of the items, also 5x:

ms:nx jim$ /usr/bin/time -l ./oltable2
initsize = 8388608
resize!
resize!
len = 8000000, maxmem=524816384, seconds to create table: 1.402532
Seconds to delete table: 1.213329
len = 0 cap = 16777216
len = 8000000, maxmem=524816384, seconds to create table: 0.7864359999999997
Seconds to delete table: 1.208817
len = 0 cap = 16777216
len = 8000000, maxmem=524816384, seconds to create table: 0.7875629999999996
Seconds to delete table: 1.209598000000001
len = 0 cap = 16777216
len = 8000000, maxmem=524816384, seconds to create table: 0.7289789999999998
Seconds to delete table: 1.204859
len = 0 cap = 16777216
len = 8000000, maxmem=524816384, seconds to create table: 0.7859240000000014
Seconds to delete table: 1.158434
len = 0 cap = 16777216
       10.62 real        10.41 user         0.19 sys
 516493312  maximum resident set size


ms:nx jim$ /usr/bin/time -l ./oltable3
initsize = 8388608
resize!
resize!
len = 8000000, maxmem=524816384, seconds to create table: 1.370501
Seconds to delete table: 0.6712990000000003
len = 4000000 cap = 16777216
len = 8000000, maxmem=524816384, seconds to create table: 0.9627829999999999
Seconds to delete table: 0.682404
len = 4000000 cap = 16777216
len = 8000000, maxmem=524816384, seconds to create table: 0.9624570000000001
Seconds to delete table: 0.6837550000000006
len = 4000000 cap = 16777216
len = 8000000, maxmem=524816384, seconds to create table: 0.9629490000000001
Seconds to delete table: 0.7126869999999998
len = 4000000 cap = 16777216
len = 8000000, maxmem=524816384, seconds to create table: 1.074509
Seconds to delete table: 0.7521380000000004
len = 4000000 cap = 16777216
        8.96 real         8.77 user         0.17 sys
 516505600  maximum resident set size

We're still trying to figure out why OLTab is using more memory than it seems like it should.

[hashbackup]

It turns out that adding -d:useMalloc makes a lot of these tests use less memory, include OLTab, which goes down by about half. Compactdict uses less too for the 5-loop test deleting half the table, like 8xxMB instead of 1.1GB. And with -d:useMalloc, there are differences between platforms because they use different system mallocs.

I think some of these numbers are useful to understand relative behavior, but wouldn't count on any of them in an absolute sense without re-running tests with -d:useMalloc (and --gc:arc).

[hashbackup]

CountTable has the same delete problem as OrderedTable, where it deletes an item by rebuilding the table. Anytime a key's count is set to 0, it is considered a delete and causes a table rebuild.

Here's a test of 1M items with current code. It had to be killed after about a minute:

ms:nx jim$ /usr/bin/time -l ./bigtable
Seconds to create 1M item table: 0.09395299999999999
Seconds to iterate whole table: 0.002224000000000004 sum 499999500000
^CSIGINT: Interrupted by Ctrl-C.
       56.14 real        46.52 user         9.61 sys
  67657728  maximum resident set size

Here's the same test with remove() modified to zero a key's count if it is non-zero using the backshift algorithm in Tables and OrderedTables:

ms:work1 jim$ /usr/bin/time -l ./bigtable
Seconds to create 1M item table: 0.07650999999999999 len 999999
Seconds to iterate whole table: 0.002146000000000009 sum 499999500000
Seconds to delete 99% of table: 0.07521600000000001 len 9999
Seconds to iterate 1% of table: 0.002167999999999976 ideal is 2.146000000000009e-05 sum 4999500000

Seconds to create 2M item table: 0.155319 len 1999999
Seconds to iterate whole table: 0.004138000000000031 sum 1999999000000
Seconds to delete 99% of table: 0.158007 len 19999
Seconds to iterate 1% of table: 0.004251999999999978 ideal is 4.138000000000031e-05 sum 19999000000

Seconds to create 4M item table: 0.328422 len 3999999
Seconds to iterate whole table: 0.00802500000000006 sum 7999998000000
Seconds to delete 99% of table: 0.3280680000000001 len 39999
Seconds to iterate 1% of table: 0.007995999999999892 ideal is 8.02500000000006e-05 sum 79998000000

Seconds to create 8M item table: 0.665389 len 7999999
Seconds to iterate whole table: 0.01601000000000008 sum 31999996000000
Seconds to delete 99% of table: 0.6669400000000001 len 79999
Seconds to iterate 1% of table: 0.01594700000000016 ideal is 0.0001601000000000008 sum 319996000000

        2.57 real         2.36 user         0.19 sys
 369668096  maximum resident set size

The new remove() is (firstTry can be replaced by the old code):

proc remove[A](t: var CountTable[A], key: A) =
  var i = rawGet(t, key)
  if i < 0: return
  dec(t.counter)
  var j = i
  block outer:         # KnuthV3 Algo6.4R adapted for i=i+1 instead of i=i-1
    while true:        # The correctness of this depends on (h+1) in nextTry,
      while true:      # though may be adaptable to other simple sequences.
        i = nextTry(i, t)                 # increment mod table size
        if t.data[i].val == 0:            # end of collision cluster; So all done
          break outer
        let r = firstTry(hash(t.data[i].key), t)  # "home" location of key@i
        if not ((i >= r and r > j) or (r > j and j > i) or (j > i and i >= r)):
          break
      when defined(js):
        t.data[j] = t.data[i]
      else:
        t.data[j] = move(t.data[i]) # data[i] will be overwritten or marked empty
      itemMoved(t, j)
      j = i
  t.data[j].key = default(typeof(t.data[j].key))
  t.data[j].val = default(typeof(t.data[j].val))

[c-blake]

While one can nitpick about double link requirements in OrderedTable, I see no reason not to just use the regular backshift delete algo in CountTable. No idea how often it is needed.

Since maybe concrete examples may be helpful, one application area might be a histogram over a moving window of data. This is like a moving average, but where you retain the whole distribution as some datum leaves the window and another enters. Moving windows are helpful in the world when dealing with non-stationary statistical processes since they have well-defined finite "memory". Now, any operation on the whole distribution would also be O(n) on the CountTable. So, if such were done once per window, this delete improvement would be "only" a 2x speed-up on the entire "update-report" operation. 2x with no downside alone seems worthwhile, but it could be much more than a 2x speed-up. Some loop over the data could be just "reserving the right" to do a whole-distribution thing without actually doing it for every single data point, doing things in some "controlled thinning of data" way.

One might say, people with this application area can use Table.mgetOrPut and inc or dec to get what they need. A little proc taking a var count and a handle to the Table could even be used to optimize the decrement-and/or-delete operation to just 1 hash lookup unless decrement takes you to delete, in which case 2. So, users with a need like the above paragraph are not completely out of luck with the stdlib.

That said, to the extent we have a CountTable in the stdlib, I see no reason why this one operation should be unnecessarily very slow, forcing users into this more awkward regular Table solution. Just documenting CountTable.del as O(n) as we do now without mentioning the perhaps non-obvious-in-context Table solution might also make such users feel that they had no stdlib solution at all. Meanwhile, documenting it by saying "if you need del efficiency use Table" just sounds lame/lazy, and that has poor project optics. So, I think the best way out is just fixing CountTable.del along the lines @hashbackup mentions with a backshift delete algo. Coding-wise, not sure if here is a way to re-org/re-use the same actual code, but that seems more nice-to-have than necessary.

[Araq]

IMHO CountTable.del is a victim of the "silly consistency axiom". A CountTable is for counting items. Want to delete an item? Use something else instead, that's not covered by "counting". Want to set an item to -100 of its current value? That's not counting either.

CountTable.del exists for "consistency" with Table.del but at this point "consistency" became a shallow argument in favor of everything. But ymmv.

[c-blake]

In English NounTable is probably more common than VerbTable. So, I always read that as a "table of counts". Decrementing a count always seemed as appropriate to provide, though admittedly less common than incrementing.

However, even taken as a verb, "counting down" happens (in Nim, even!) as well as "counting up" and even has first-class mention in your iterator countup|countdown.

Your -100 also feels wrong. Sometimes you count up/down are somethingElse.lens, for example. The oldest proc inc(TCountTable) I can find took a val = 1, but using your strict ideas about "counting", "counting up by val" is also not "counting". So, it would seem even your own first conception of counting was more permissive than your present idea.

So, I don't know why you think one should only count up and only count by 1. It is certainly not directly implied by the name and more restrictive than other uses of "countFoo" in Nim. I don't know German. Maybe there is some kind of lost in translation thing happening.

Once you can count down, delete becomes a natural end-state thing, not weird. It's an open addressed, linear probed hash table. So, delete like usual for such seems the right thing to do, and it seems like @hashbackup is offering. Or deprecating both OrderedTable.del|pop and CountTable.del|pop. The latter might be pre-1.0 thinking, but that didn't save proc add. ;-) Retaining slow O(n) ops when there is no compensating "another op is smaller/faster" argument seems the worst of both worlds - both user-unfriendly and bad marketing, efficiency-wise, and in a high profile module like tables. I would say if you are not going to deprecate then we should encourage @hashbackup to do a PR for CountTable, as well as relaxing the Positive constraint on inc and maybe even providing dec. It's conceivable this should be an RFC since there seems to be some semantic disagreement.

[hashbackup]

I think having a very narrow definition of what counting is, is fine, and if del is not part of that, then it should be deprecated rather than have its current bizarre (to me at least) behavior.

An important related point is that CountTable allows setting a counter to a specific value. I don't know if that's inside your definition of counting or not. If it is not, then that also should be deprecated, because when a counter is set to 0, that forces either a) a table rebuild like today, or b) a backshift. If the counter value is simply set to 0, it breaks future searches for any collisions that follow it. I know, because I naively tried it. :-)

I don't think tables need to all have the same operations. It's nice, but not necessary. But whatever operations are provided, should be performant. Isn't that Nim's #1 priority? What impression do you think it leaves a new user about Nim if they see documented functions that, when executed, give crazy performance profiles that no other language (that I've ever seen) has?

I won't be doing a PR for any of this. I'm over the arguing / justifying / defending it. It's interesting for me to work on and I'm glad to contribute the code, but I won't go through the PR process with you guys. Too much stress / hassle. Sorry, it gets a -1 from me. :-)

[c-blake]

If it's just a matter of who does it, I can do a CountTable backshift delete PR. @hashbackup is right that if we don't do it, more than what I mentioned should probably be deprecated. It's a bit of work, though. So, if it will be outright rejected on semantics then it doesn't make sense to do it.

[Araq]

If it's just a matter of who does it, I can do a CountTable backshift delete PR

Oh yes please. :-)

[hashbackup]

Here's the test program I've been using, if it's helpful:

when defined(compact):
  import compactdict
when defined(lru):
  import lrucache
when defined(oltab):
  import adix/adix/oltab
else:
  import tables
import times


proc runtest(els: int) =

  when defined(table):
    var ot = initTable[int, int](els)
  when defined(ordered):
    var ot = initOrderedTable[int, int](els)
  when defined(count):
    var ot = initCountTable[int](els)
  when defined(lru):
    var ot = newLruCache[int, int](els)
  when defined(compact):
    var ot = initDict[int, int](els)
  when defined(oltab):
    var ot = initOLtab[int, int](els)

  var t0 = cpuTime()
  for i in 0 ..< els:
    ot[i] = i
  echo "Seconds to create ", els div 1_000_000, "M item table: ", cpuTime() - t0, " len ", len(ot)

  var twhole = cpuTime()
  var sum = 0
  for k, v in ot.pairs:
    sum += v
  twhole = cpuTime() - twhole
  echo "Seconds to iterate whole table: ", twhole, " sum ", sum

  t0 = cpuTime()
  for i in 0 ..< els:
    if i mod 100 != 0: ot.del(i)
  echo "Seconds to delete 99% of table: ", cpuTime() - t0, " len ", len(ot)

  sum = 0
  var t1 = cpuTime()
  for k, v in ot.pairs:
    sum += v
  t1 = cpuTime() - t1
  echo "Seconds to iterate 1% of table: ", t1, " ideal is ", twhole/100, " sum ", sum
  echo ""


for els in @[1_000_000, 2_000_000, 4_000_000, 8_000_000]:
  runtest(els)