Index RStrpool with a hash table instead of scanning it ##util

[phix33]

• Mark this if you consider it ready to merge
• I've added tests (optional)
• I wrote some lines in the book (optional)

Description

Half of the time spent loading a binary with a large DWARF line table goes into r_strpool_add: the pool's 1024-bit bloom saturates after a few hundred strings, and r_strpool_get then compares every pooled string on every call (the function carries an XXX this is O(n) comment). The addrline store interns the file name twice per row, so a libc with 1553 source files and 600k rows does about 10^8 strcmp calls.

$ /usr/bin/time -f '%e s %U user' r2 -N -qc q bins/elf/libc-2.26-debug.so
2.36 s 2.34 user

With this PR the same load takes 1.84 s wall and 1.82 user on the same box; callgrind attributed 48% of the instructions to the pool scan before.

• RStrpool keeps an HtPP from string to position instead of the bloom. It is built on the first r_strpool_get/r_strpool_add, so an append-only user (corelog) never pays for it, and r_strpool_append keeps it in sync once it exists. slice, slice_range and empty drop it, since positions move.
• r_strpool_add is get or append; a repeated append of the same string still returns a new position while get keeps answering the first one, as the scan did.

test/unit/test_strpool.c covers dedup, append-then-get, 5000 strings across pool growth, and lookups after slice_range and empty; the pool's visible behaviour is unchanged, so no golden moves and the numbers above are the evidence. A failed realloc in the pool used to free the old buffer and leave the positions pointing at it; it now keeps the buffer and reports the failure.

[trufae]

Several concerns here:

• bloom works better for small string sets
• Hashtable takes a lot of memory and it can actually be HtPU for string-position instead.. if stringpool is reallocated the position is kept but the string poiting at is not
• We only want a ht to add, when the string pool is complete we no longer need a hashtable or bloom. So maybe we can add lock and unlock functions to reindex or free that hashtable . And make bloom/ht optional.
• Also im unsure about the reason why its reescanimg, the purpose of bloom should be to permit duplicated strings at the cost of not having to scan them because of the dupes will be catched that way. Dunno if its a design mistske or jist a bad usage, so we may benchmark on small datasets to confirm and choose if we want to keep optional bloom for small sets or if ht is needed etc

[phix33]

I measured it before changing anything, and you were right on the main point: the cost is mostly bad usage.

Why it rescans. The DWARF line store calls r_strpool_add once per row with the file name of that row. On libc-2.26-debug.so that is 152626 adds for 1684 distinct strings, so 99% of adds are duplicates. A bloom can only answer "maybe present" for a duplicate, never where it is, so every duplicate scans. The 1024-bit bloom also saturates after a few hundred strings, but resizing it to the pool changes nothing measurable (3488 vs 3520 ns per add on the libc sequence).

Small sets. Over the 108 DWARF files in testbins, 94 pools get adds; the median has 3 distinct strings and 90 have 256 or fewer. With no duplicate locality, the bloom beats a hash table from 64 to 256 strings, and a plain scan beats both below about 32. So a table should not be the default for small pools.

Memory. The HtPP in this PR copies every key, so the index holds a second copy of every string: 350 KB on libc against 105 KB for the whole pool today. Keying by string hash to position avoids the copy (139 KB), but none of this shows in peak RSS (495 MB for that load).

What fixes it. The same string arrives on consecutive rows 61-98% of the time, so remembering the last hit is enough:

libc-2.26-debug.so, -qc q, arm64 -O2	instructions	wall
master	31.0 G	2.0 s
this PR (HtPP)	16.4 G	1.32 s
last-hit check in r_strpool_add, bloom kept	17.0 G	1.36 s
last-hit check + hash index, scan below 32 strings	16.2 G	1.32 s

CL output is byte-identical to master for every variant on libc, libstdc++, dwarf_go_tree and dwarf_rust_bubble.

I would replace this PR with the last-hit check alone: about ten lines in strpool.c, the bloom stays, no new memory. The index with lock/unlock and the small-set scan is worth adding only if a caller without duplicate locality shows up; nothing in testbins has one today. Does that work for you, or do you want the index and lock/unlock in now?

[trufae]

Yeah an extra concern for me is that the kashrable keys are strdups and considering we hold them all in a string pool we can just store the offset inside that pool instead and that will require a different htpp monster (change the callbacks that implement the keydup and such).

[trufae]

The last hit thing for dupped checks is probably the best approach for this usecase but not for all thats why i think we can have a better strpool policy to handle different usecases in a better way