Profile and improve map performance

[domenicquirl]

We just merged #43, which gives us benchmarks! Currently we have benches against dashmap and hashbrown.

First runs of the benchmarks seem to indicate that retrieving values from the map is quite performant, but inserting into the map is rather slow compared to both other maps. Preliminary results:

• against hashbrown, insert and insert_erase run in the order of one ms, compared to 10-20 us in hashbrown. get holds up reasonably well though, still ~20 us to ~5-6, but given the added concurrency this seems much better than insert. Both of these use one guard across all operations of one benchmark run and relatively consistent across input distributions.
• against dashmap, dashmap inserts in something between 1 ms with 1 thread and .8 ms for multiple threads, however what's probably more important is throughput. This for some reason is not in the report produced by criterion, but was on the order of 100 MElements/s. We compare with about 25-12 ms depending on thread count, which is so slow it takes several minutes to run any benchmark and puts throughput at something like 5, maybe 10 MElements/s. This is using 1 guard per map operation, which admittedly is not what you would do in a scenario such as the benchmark.
• against dashmap, but with only one guard per thread (the different setup), we insert in 14-11 ms, which is better, but still considerably slower than dashmap. Note that this does not compare directly due to the different setup of the threads.
• get against dashmap seems really good! dashmap gets 700-500 us, while we get 1.5 ms on one thread, 800us on 2 threads and go down to below 300 us on 8 threads, even with pin() for every call to get. With only one guard, this improves to 800 to a bit more than 200 us, depending on thread count.

It would be interesting to find out if these results are reproducible and, if so, what causes inserts to be so slow. @jonhoo suggests graphing the results of perf record as a profiling method in the discussion in #43, maybe this can be used as a starting point for the analysis.

[jonhoo]

To start people off, here is a flamegraph of insert_flurry_hashbrown/low_guard_once:

Looking at the profile, the stack on the right (ignoring the gnuplot stack) is probably what we should look at (direct link). From what I can tell, table initialization is really the big thing here, which isn't really what we want the benchmark to measure. We should look into how to avoid measuring that when measuring the cost of inserts.

[jonhoo]

When looking at the cost of inserts, the biggest cost I see is allocation (Owned::new), which isn't terribly surprising. This is a cost we have to pay, but many other implementations do not. I wonder if we could save a fair bit by keeping a pool of Owned that can be re-used rather than defer_destroying them immediately...

[domenicquirl]

Interesting. The hashbrown benchmarks create the map once and then clear() it at the start of every iteration. This should definitely be added to our benchmarks once #42 is done. I contemplated using retain(|_,_| false) instead, on my machine this performs somewhat faster (significant difference), but still only on the order of a few hundred us. We should have an eye on improvements from clear() though.

Overall, I'm more concerned with the comparison to dashmap since it is more directly comparable in usage and the difference is so much larger. It is also worth noting that dashmap does create a new map in each iteration in their benchmarks for insert (see task_insert_dashmap_u64_u64()).

[domenicquirl]

Re: Owned what would support your thoughts about re-using them is that Owned::drop is also a sizeable portion of transfer, using slightly more time than malloc according to the graph.

Edit: transfer is also responsible for most of the time used by with_capacity and with_capacity_and_hasher, as it is used to help with initialisation, so these methods would transitively profit from this.

[jonhoo]

Here is the flamegraph for dashmap::insert_u64_guard_once:

[jhinch]

If creating and dropping Owned within the transfer method is a major source of performance issues, one option might be to share the Moved nodes allocated to point to the new table, instead of allocating one for each entry. Each BinEntry::Moved is exactly the same in terms of the data it holds and Atomic which stores the bin entry can take both a Owned or a Shared as the pointer for its input.

[jonhoo]

@jhinch that could work, though the tricky part is going to be to find the right way to free them. If we can figure that out though, that would probably help a lot.

[domenicquirl]

I'd say the Moved would be associated with the table where it is used and could be marked as garbage together with the containing table? So either with Table::drop or explicitly when a resize finishes. Terminating resizes is the point where the old table is no longer accessible, so no new threads can get a reference to the Moved and it could be defer_destroyed and collected after the references to the old table are gone? This would be similar to the argument about the safety of defer_destroying the old table here. I'll think about this some more when I'm less tired, but also I don't have that much free time at the moment, so we'll see.

[jonhoo]

That's a good observation, and yes, I think that could work! That should give a very nice speedup for resizes indeed. Anyone want to write up a PR?

[jonhoo]

Thinking some more about this, I think the trick will be to allocate a Moved for a table when its replacement is allocated, and then to store a pointer to that in the Table struct of the table that is being moved from. And then in Table::drop we mark it for deletion. We have to be a little careful though that we don't have any outstanding references to it, even inside drop, when we destroy it.

[domenicquirl]

As an update on hashbrown, I changed the benchmark to use clear now that we have it (see #55), which gets insert down to about 260us on my machine. The other benchmarks are untouched since they do not use clear, though interestingly get is now down to ~7-8us for me as well, matching hashbrown more closely. For now, this seems fine, except that I don't know where the rather big difference for insert_erase originates.

[domenicquirl]

@jonhoo That's what I meant, yeah. Sorry if that was unclear from my previous, rather tired comment 😅

Could you expand on the problem you see with existing references at the point of droping the old table? The safety guarantee we need to provide for defer_destroy is only that no new references to the Moved can be obtained by other threads, which should be the case if drop for the old table is called due to a resize finishing as then the map will refer new threads to the new table. Existing references would be protected by the guard that obtained them, since as long as they are alive the old table cannot be dropped, no?

[jonhoo]

@domenicquirl Ah, but notice that the drop code uses epoch::unprotected, which destroys immediately.