Performance improvements for IdDict

[Zentrik]

The main improvement is from get! which no longer calculates the hash twice when it has to setindex and so is twice as fast. This is my first time working with the Julia C code so there might be some mistakes there but it seems to work fine for me.
This is useful for a global value numbering pass I've written.

[JeffBezanson]

Thanks, any speedup numbers to whet the appetite?

I'm not really comfortable duplicating all the lookup code, and the try-catch around the conversion is not a good pattern to use. This needs a different design. Maybe we can copy how get! works for Dict.

[Zentrik]

Here's some performance numbers,

function f!(d)
    for x in 1:10^4
        d[x] = 1
    end
end

function g!(d)
   for x in 1:10^4
       get!(d, x, 1)
   end
end

On master

julia> versioninfo()
Julia Version 1.11.0-DEV.356
Commit 4768d7ab8fe (2023-08-28 12:54 UTC)
Build Info:
  Official https://julialang.org/ release
Platform Info:
  OS: Linux (x86_64-linux-gnu)
  CPU: 16 × AMD Ryzen 7 1700 Eight-Core Processor
  WORD_SIZE: 64
  LLVM: libLLVM-15.0.7 (ORCJIT, znver1)
  Threads: 1 on 16 virtual cores

julia> @btime f!(d) setup=(d=IdDict{Int, Int}(); sizehint!(d, 10^4))
  409.015 μs (9489 allocations: 148.27 KiB)

julia> @btime g!(d) setup=(d=IdDict{Int, Int}(); sizehint!(d, 10^4))
  639.763 μs (18978 allocations: 296.53 KiB)

On this pr

@btime f!(d) setup=(d=IdDict{Int, Int}(); sizehint!(d, 10^4))
  324.515 μs (9489 allocations: 148.27 KiB)

@btime g!(d) setup=(d=IdDict{Int, Int}(); sizehint!(d, 10^4))
  330.156 μs (9489 allocations: 148.27 KiB)

[oscardssmith]

why does this need to be implemented in C?

[Zentrik]

I'm not sure how to call jl_table_assign_bp directly from Julia as it can change d.ht.

[vtjnash]

There used to be ways, but not anymore with the generational GC. The usual way now to implement this is to switch the return object around as such:

Suggested change

	keyindex = ccall(:jl_eqtable_keyindex, Cssize_t, (Any, Any), d, key)
	keyindex = Ref(UInt(0))
	d.ht = ccall(:jl_table_assign_bp, Any, (Any, Any, Ref{UInt}), d.ht, key, keyindex)
	keyindex = keyindex[]

[vtjnash]

Can you change this around still also? It will avoid us from needing to reflect the iddict struct into C

[Zentrik]

Just to note, I believe this would remove the ~20% speedup (see comments above) we get from not having to heap allocate for setindex!. Happy to make this change if that's fine.

[Zentrik]

CI looks good, ASAN just timed out I think.

[vtjnash]

There seems to be a discrepancy here in the state of the IdDict during this convert call or if this convert failed. The ht_keyindex! call has fully inserted the key, but not the value. For Dict, neither has occurred yet, and all insertion and accounting happens at once in _setindex!. But in IdDict here, it looks like half of the insertion occurs in ht_keyindex! (effectively inserting it in the deleted state) but then the final accounting update for it doesn't happen until after the _setindex! call later in this method. Do we update ndel over this call so the value is consistent?

There also may need to be an age counter added to the IdDict, so it can detect concurrent modifications that happen in this convert call (or move this convert call back to the top of the function)

[vtjnash]

Suggested change

@inbounds d[-keyindex] = nothing

[Zentrik]

Does the deleted state not require they key to be nothing? Though this prevents a segfault so I'm probably missing something.

[Zentrik]

Nevermind, the segfault was because I meant to write d.ht[-keyindex] = nothing. My concern is if the convert fails we have a valid key inserted and the value is NULL. This will prevent us from ever updating the value as we will hit this assert

julia/src/iddict.c

Lines 63 to 69 in e22db88

	if (jl_atomic_load_relaxed(&tab[index + 1]) != NULL) {
	jl_atomic_store_release(&tab[index + 1], val);
	jl_gc_wb(a, val);
	return 0;
	}
	// `nothing` is our sentinel value for deletion, so need to keep searching if it's also our search key
	assert(key == jl_nothing);

I can move the key updating code out of ht_keyindex2! if that helps.