Fix type instabilities in AVL, RB and splay tree

[goerch]

Improves benchmark results for
#762

v0.18.10

100000 elements
DataStructures.AVLTree 6.061 s (7102520 allocations: 114.48 MiB)
1000 elements
DataStructures.AVLTree 36.800 ms (28637 allocations: 509.97 KiB)
10 elements
DataStructures.AVLTree 134.361 μs (102 allocations: 2.23 KiB)

v0.19.0-DEV

100000 elements
DataStructures.AVLTree 6.113 s (7102520 allocations: 114.48 MiB)
1000 elements
DataStructures.AVLTree 36.298 ms (28637 allocations: 509.97 KiB)
10 elements
DataStructures.AVLTree 130.629 μs (102 allocations: 2.23 KiB)

Now:

100000 elements
DataStructures.AVLTree 213.450 ms (200002 allocations: 9.16 MiB)
1000 elements
DataStructures.AVLTree 1.408 ms (2002 allocations: 93.80 KiB)
10 elements
DataStructures.AVLTree 7.814 μs (22 allocations: 1008 bytes)

[oxinabox]

Nice work.
Those are quite the performance improvements.

need to address the type-piracy though

We should also delete/change

https://github.com/JuliaCollections/DataStructures.jl/pull/766/files?diff=split&w=1#diff-bed9f1a3faabfaae84a4ed46f9cd059405077f120c7737ab8e782d84a54c7d32R15
and
https://github.com/JuliaCollections/DataStructures.jl/pull/766/files?diff=split&w=1#diff-11e9fe382dc297594ea9e1ef03a4076696e21896a49b4be935cd710bfa23fa37R18

which define:
RBTreeNode(d) = RBTreeNode{Any}(d) should really be: RBTreeNode(d::K) where K = RBTreeNode{K}(d)
and similar

[goerch]

After some more experimenting with type specific convert I don't see a way around specializing setproperty!. Without it I currently get (on a different system)

100000 elements
  DataStructures.AVLTree                   431.418 ms (200002 allocations: 9.16 MiB)
  DataStructures.RBTree                    545.514 ms (200003 allocations: 12.21 MiB)
  DataStructures.SplayTree                 366.566 ms (200002 allocations: 9.16 MiB)
1000 elements
  DataStructures.AVLTree                   5.499 ms (2002 allocations: 93.80 KiB)
  DataStructures.RBTree                    4.756 ms (2003 allocations: 125.11 KiB)
  DataStructures.SplayTree                 3.164 ms (2002 allocations: 93.80 KiB)
10 elements
  DataStructures.AVLTree                   48.985 μs (22 allocations: 1008 bytes)
  DataStructures.RBTree                    43.387 μs (23 allocations: 1.36 KiB)
  DataStructures.SplayTree                 28.924 μs (22 allocations: 1008 bytes)

Specializing like

Base.setproperty!(x::AVLTreeNode{T}, f::Symbol, v) where {T} =
    setfield!(x, f, v)

I see

100000 elements
  DataStructures.AVLTree                   194.724 ms (200002 allocations: 9.16 MiB)
  DataStructures.RBTree                    192.539 ms (200003 allocations: 12.21 MiB)
  DataStructures.SplayTree                 123.407 ms (200002 allocations: 9.16 MiB)
1000 elements
  DataStructures.AVLTree                   1.250 ms (2002 allocations: 93.80 KiB)
  DataStructures.RBTree                    1.253 ms (2003 allocations: 125.11 KiB)
  DataStructures.SplayTree                 982.515 μs (2002 allocations: 93.80 KiB)
10 elements
  DataStructures.AVLTree                   6.998 μs (22 allocations: 1008 bytes)
  DataStructures.RBTree                    5.598 μs (23 allocations: 1.36 KiB)
  DataStructures.SplayTree                 7.348 μs (22 allocations: 1008 bytes)

Do you still suspect type piracy with the above change?

Edit: Tried to discuss the problem on Discourse recently: https://discourse.julialang.org/t/dynamic-dispatch-with-union-nothing/70174/8

[goerch]

We should also delete/change

https://github.com/JuliaCollections/DataStructures.jl/pull/766/files?diff=split&w=1#diff-bed9f1a3faabfaae84a4ed46f9cd059405077f120c7737ab8e782d84a54c7d32R15 and https://github.com/JuliaCollections/DataStructures.jl/pull/766/files?diff=split&w=1#diff-11e9fe382dc297594ea9e1ef03a4076696e21896a49b4be935cd710bfa23fa37R18

which define: RBTreeNode(d) = RBTreeNode{Any}(d) should really be: RBTreeNode(d::K) where K = RBTreeNode{K}(d) and similar

Sorry I can't estimate the impact of these changes. You should be able to edit this patch at will?

[oxinabox]

Sorry I can't estimate the impact of these changes. You should be able to edit this patch at will?

A follow up can be made

[oxinabox]

Do you still suspect type piracy with the above change?

There is no longer any type-piracy in the code, and it is much cleaner.
Good job

[oxinabox]

I am surprised this is needed.
What happens if we delete this?

[goerch]

See #766 (comment)

[goerch]

Specifically: Juno profiler shows dynamic dispatch in

setproperty!(x, f::Symbol, v) = setfield!(x, f, convert(fieldtype(typeof(x), f), v))

called by

    z.rightChild = α

in left_rotate and

    z.leftChild = α

in right_rotate.

[goerch]

An alternative fix seems to be

    if α === nothing
        z.rightChild = nothing
    else
        z.rightChild = α
    end

but I do not feel something like this should be necessary?

[oxinabox]

Similar I am surprised this is needed, what happens if we delete this?

[goerch]

See #766 (comment)

[goerch]

Here the analysis with Juno profiler is not that clear cut. It surprisingly indicates alloctions in

setproperty!(x, f::Symbol, v) = setfield!(x, f, convert(fieldtype(typeof(x), f), v))

called from

    node_x.rightChild = node_y.leftChild

in left_rotate! and

    node_x.leftChild = node_y.rightChild

in right_rotate!.

misclicked

[goerch]

@oxinabox, @eulerkochy: any ideas how to proceed?

[oxinabox]

Lets merge this now, it can always be improved later.