Dict: decrement h.ndel when overwriting deleted entry

[oscardssmith]

fixes #47823. Previously we never lowered this leading us to rehash unnecessarily. I'm not sure if this counts as a bugfix.

[KristofferC]

Looks to me like the @inbounds should be used by the caller of this function (ref how the index access a few lines above does not have @inbounds on them.

[ancapdev]

My question was just if the inbounds propagation carries to the next level in the call chain, and without firm understanding of that it felt safer and harmless to add the inbounds here as any bounds violation would have thrown already at this point

[petvana]

Although the PR is a clear improvement, it cannot solve #47823 fully because the size of the dictionary remains only 16. Thus, after every ~20 remove operations, it needs to rehash and allocate memory. This is because the keys are different. A possible solution is to implement an in-place rehash! in case the size is not changed.

julia> using BenchmarkTools

julia> function foo(d, reps)
           for i in 1:reps
               d[i] = 1
               delete!(d, i)
           end
           nothing
       end
foo (generic function with 1 method)

julia> @btime foo($(Dict{Int, Int}()), 1000) # This PR
  23.679 μs (249 allocations: 37.61 KiB)

[oscardssmith]

Yeah, it's unfortunate that this doesn't fix it better. It would be a good idea to investigate if there are places where we can clean out more tombstones during setindexing. Also somewhat hilariously, 3 people reviewed this and none of them realized that I was doing the check after updating the slots making the check useless (now fixed).

[petvana]

Also somewhat hilariously, 3 people reviewed this and none of them realized that I was doing the check after updating the slots making the check useless (now fixed).

Now it provably helps ;-)

julia> @btime foo($(Dict{Int, Int}()), 1000)
  17.825 μs (126 allocations: 19.03 KiB)

[ancapdev]

Yeah, it's unfortunate that this doesn't fix it better. It would be a good idea to investigate if there are places where we can clean out more tombstones during setindexing.

Would it be possible to clean them out after hitting the rehash threshold and aborting rehash if cleanup takes you back under?

[oscardssmith]

@vtjnash pointed out that we also were unconditionally inserting tombstones on deletion, but we don't have to do so always (and can in fact delete them in some cases). When we do this properly, we get 20% faster with 0 rehashes for this benchmark. (there still will be possible unnecessary rehashes in more complicated scenarios)

[petvana]

@nanosoldier runbenchmarks("collection", vs = ":master")

[nanosoldier]

Your benchmark job has completed - no performance regressions were detected. A full report can be found here.

[oscardssmith]

I think this is ready for us to merge.