Detect multiple concurent writes to Dict + remove dead code

[petvana]

Thanks to #38180, the removed code seems to be dead because no finalizers should be used anymore to modify dictionaries (it was dangerous). Furthermore, it may help users to detect illegal concurrent writes, since it doesn't recurse and have new error message. There should be no, or even a positive, performance effect.

Btw, this seems to be the last missing step to allow manual shrinkage at sizehint!

julia/base/dict.jl

Lines 251 to 256 in 89a613b

	if newsz <= oldsz
	# todo: shrink
	# be careful: rehash!() assumes everything fits. it was only designed
	# for growing.
	return d
	end

Master

$ julia -t 2

julia> d = Dict()
Dict{Any, Any}()

julia> Threads.@threads for i = 1:1000; d[i] = i; end
#... Hangs for ... so long ... and I terminated the loop

PR

$ julia -t 2

julia> d = Dict()
Dict{Any, Any}()

julia> Threads.@threads for i = 1:1000; d[i] = i; end
ERROR: TaskFailedException
Stacktrace:
 [1] wait
   @ ./task.jl:345 [inlined]
 [2] threading_run(fun::var"#23#threadsfor_fun#5"{var"#23#threadsfor_fun#4#6"{UnitRange{Int64}}}, static::Bool)
   @ Base.Threads ./threadingconstructs.jl:38
 [3] top-level scope
   @ ./threadingconstructs.jl:89

    nested task error: AssertionError: Muliple concurent writes to Dict detected!
    Stacktrace:
     [1] rehash!(h::Dict{Any, Any}, newsz::Int64)
       @ Base ./dict.jl:235
     [2] _setindex!
       @ ./dict.jl:382 [inlined]
     [3] setindex!(h::Dict{Any, Any}, v::Any, key::Int64)
       @ Base ./dict.jl:419
     [4] macro expansion
       @ ./REPL[2]:1 [inlined]
     [5] (::var"#23#threadsfor_fun#5"{var"#23#threadsfor_fun#4#6"{UnitRange{Int64}}})(tid::Int64; onethread::Bool)
       @ Main ./threadingconstructs.jl:84
     [6] #23#threadsfor_fun
       @ ./threadingconstructs.jl:52 [inlined]
     [7] (::Base.Threads.var"#1#2"{var"#23#threadsfor_fun#5"{var"#23#threadsfor_fun#4#6"{UnitRange{Int64}}}, Int64})()
       @ Base.Threads ./threadingconstructs.jl:30

[vtjnash]

move this above the assignments, so we only do the mutation if things are not broken?

[petvana]

It's only a sanity check for now and thus can be moved anywhere. I've also added one aging for more safety.

[vtjnash]

This seems good. Does it improve TTFP too? This call used to cause a lot of inference latency.

[vtjnash]

(I am fairly sure that comment is also very old, and refers to a time when we assumed that doubling the size meant eliminating hash collisions–a false assumption we deleted long ago)

[petvana]

This seems good. Does it improve TTFP too? This call used to cause a lot of inference latency.

Unfortunately not, TTFP seems unchanged.

[aviatesk]

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

[nanosoldier]

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

[petvana]

Not sure, if multiple threads are enabled in CI. Just to make sure, it will be deleted.

[petvana]

Any idea on how to enable CI with Threads.nthreads() != 1? Going through other tests and not finding the right way.

[petvana]

Does it mean that the following test is disabled now in CI?

julia/test/threads_exec.jl

Lines 30 to 32 in 897c3cf

	# basic lock check
	if nthreads() > 1
	let lk = Base.Threads.SpinLock()

[simeonschaub]

The test seems fundamentally flawed since there is no guarantee you actually will run into a data race, so you might get nondeterministic test failures.

[petvana]

The test seems fundamentally flawed since there is no guarantee you actually will run into a data race, so you might get nondeterministic test failures.

Fair point, thanks. Although the probability would be very low for the test to fail for larger dictionaries (based on how long rehashing takes), it may still happen. So, I removed the test, and PR is ready to be merged.