Use PersistentDict for efficient implementation of ScopedValue's

[vchuravy]

In #50958 I used an inlined immutable dictonary to implement ScopedValue.
That implementation performs very well for a small number of scoped values,
or a shallow nesting of dynamical scopes.

Crucially the lookup time for a scoped value grows O(n) with the number
dynamic scopes. Here I propose the usage of a PersistentDict based on
a hash array mapped trie (HAMT). HAMT has lookup O(log(32, n)).

A few numbers for the old implementation gather from ScopedValues.jl

ID	time
["DEPTH", "depth=1"]	2.980 ns (5%)
["DEPTH", "depth=2"]	3.720 ns (5%)
["DEPTH", "depth=4"]	3.720 ns (5%)
["DEPTH", "depth=8"]	5.700 ns (5%)
["DEPTH", "depth=16"]	10.631 ns (5%)
["DEPTH", "depth=32"]	25.954 ns (5%)
["DEPTH", "depth=64"]	57.548 ns (5%)
["DEPTH", "depth=256"]	265.488 ns (5%)
["DEPTH", "depth=512"]	518.125 ns (5%)
["DEPTH", "depth=1024"]	1.034 μs (5%)
["DEPTH", "depth=2048"]	2.055 μs (5%)
["DEPTH", "depth=4096"]	4.101 μs (5%)

With a PersistentDict:

ID	time
["DEPTH", "depth=1"]	9.660 ns (5%)
["DEPTH", "depth=2"]	9.660 ns (5%)
["DEPTH", "depth=4"]	9.660 ns (5%)
["DEPTH", "depth=8"]	9.660 ns (5%)
["DEPTH", "depth=16"]	9.620 ns (5%)
["DEPTH", "depth=32"]	9.659 ns (5%)
["DEPTH", "depth=64"]	9.624 ns (5%)
["DEPTH", "depth=128"]	9.663 ns (5%)
["DEPTH", "depth=256"]	9.721 ns (5%)
["DEPTH", "depth=512"]	9.792 ns (5%)
["DEPTH", "depth=1024"]	12.900 ns (5%)
["DEPTH", "depth=2048"]	13.333 ns (5%)
["DEPTH", "depth=4096"]	14.286 ns (5%)

As we can see access time is now predictable (similar in cost to task local storage),
and no longer running the risk that abundant usage of scoped values in libraries,
having negative effects through spooky action at a distance.

Now the tradeoff is that dynamic scope entry becomes more expensive,
from ~80ns to ~192ns and memory usage going from 112 bytes to 368 bytes.

So why the complication of using a HAMT and not just copying a dictonary at entry.
HAMT have the benefit of being able to structurally share memory.
Using the persitent operation insert in https://github.com/vchuravy/HashArrayMappedTries.jl we can see
the overhead cost.

ID	time	GC time	memory	allocations
["Base.Dict", "insert, size=0"]	96.143 ns (5%)		544 bytes (1%)	4
["Base.Dict", "insert, size=1"]	102.561 ns (5%)		544 bytes (1%)	4
["Base.Dict", "insert, size=2"]	102.608 ns (5%)		544 bytes (1%)	4
["Base.Dict", "insert, size=4"]	103.010 ns (5%)		544 bytes (1%)	4
["Base.Dict", "insert, size=8"]	103.189 ns (5%)		544 bytes (1%)	4
["Base.Dict", "insert, size=16"]	118.819 ns (5%)		1.33 KiB (1%)	4
["Base.Dict", "insert, size=32"]	117.118 ns (5%)		1.33 KiB (1%)	4
["Base.Dict", "insert, size=64"]	653.087 ns (5%)		4.66 KiB (1%)	4
["Base.Dict", "insert, size=128"]	655.882 ns (5%)		4.66 KiB (1%)	4
["Base.Dict", "insert, size=256"]	1.475 μs (5%)		17.39 KiB (1%)	4
["Base.Dict", "insert, size=512"]	1.515 μs (5%)		17.39 KiB (1%)	4
["Base.Dict", "insert, size=1024"]	4.547 μs (5%)		68.36 KiB (1%)	6
["Base.Dict", "insert, size=2048"]	4.816 μs (5%)		68.36 KiB (1%)	6
["Base.Dict", "insert, size=4096"]	16.320 μs (5%)		272.28 KiB (1%)	7
["Base.Dict", "insert, size=8192"]	18.520 μs (5%)		272.28 KiB (1%)	7
["Base.Dict", "insert, size=16384"]	73.600 μs (5%)		1.06 MiB (1%)	7
["HAMT", "insert, size=0"]	65.594 ns (5%)		192 bytes (1%)	4
["HAMT", "insert, size=1"]	65.015 ns (5%)		208 bytes (1%)	4
["HAMT", "insert, size=2"]	63.245 ns (5%)		208 bytes (1%)	4
["HAMT", "insert, size=4"]	63.908 ns (5%)		224 bytes (1%)	4
["HAMT", "insert, size=8"]	69.670 ns (5%)		512 bytes (1%)	4
["HAMT", "insert, size=16"]	69.619 ns (5%)		624 bytes (1%)	4
["HAMT", "insert, size=32"]	101.133 ns (5%)		464 bytes (1%)	6
["HAMT", "insert, size=64"]	102.006 ns (5%)		528 bytes (1%)	6
["HAMT", "insert, size=128"]	108.866 ns (5%)		576 bytes (1%)	6
["HAMT", "insert, size=256"]	105.794 ns (5%)		592 bytes (1%)	6
["HAMT", "insert, size=512"]	153.153 ns (5%)		672 bytes (1%)	8
["HAMT", "insert, size=1024"]	108.324 ns (5%)		1.31 KiB (1%)	6
["HAMT", "insert, size=2048"]	197.957 ns (5%)		1.45 KiB (1%)	6
["HAMT", "insert, size=4096"]	145.598 ns (5%)		912 bytes (1%)	8
["HAMT", "insert, size=8192"]	166.904 ns (5%)		1.20 KiB (1%)	8
["HAMT", "insert, size=16384"]	158.961 ns (5%)		1.22 KiB (1%)	8

For Dict we must copy the dictionary to have the persitency requirement we need for scoped values.

function insert(dict::Base.Dict{K, V}, key::K, v::V) where {K,V}
    dict = copy(dict)
    dict[key] = v
    return dict
end

This would also be an in-tree user of #46453.

Kudos to @gbaraldi for bouncing a lot of ideas over the last two weeks.

[vchuravy]

@Tokazama could you review this? I took parts #46453 to make sure that I did not provide a divergent API.
I kinda dislike setindex(dict, v, k) and much rather would have insert(dict, k, v) as the mirror image to delete(dict, k).

Also happy to split the persitent dict out into a separate PR, but I do need it for ScopedValues :)

[vchuravy]

CC other folks who might be interested in the API design @mcabbott, @jw3126

[gbaraldi]

Is this thread safe?

[Tokazama]

@Tokazama could you review this? I took parts #46453 to make sure that I did not provide a divergent API. I kinda dislike setindex(dict, v, k) and much rather would have insert(dict, k, v) as the mirror image to delete(dict, k).

Also happy to split the persitent dict out into a separate PR, but I do need it for ScopedValues :)

I agree that the order of the arguments is a bit odd but I'm not sure if the parallel to setindex! makes changing that a no go

[vchuravy]

Is this thread safe?

The underlying HAMT is not. The would require a CTrie (ConcurrentTrie), the PersistentDict by how it is implemented is thread safe.

I agree that the order of the arguments is a bit odd but I'm not sure if the parallel to setindex! makes changing that a no go

Yeah the problem is that the API exists for Names tuple, but I think we could probably add insert(dict, key, value) for ImmutableDict that's just the constructor.

[Tokazama]

Implementation looks good and love the additional stuff this is building up to. I think there's a need for more general API docs on some of this but I don't know if it's necessarily important to bog down this PR since some of the details will depend on what happens moving forward with other stuff in #46453.

[Tokazama]

In the original docs it had setindex(collection, value, key...). Should this be setindex(collection, value, inds...) for application to other collection types (tuples, arrays, etc.)? Also, is this public API now and belongs in the docs?

[vchuravy]

Yeah this makes it public API IMO (I still vote for insert instead).

If we don't want to make Base.setindex API just jet I could also do the same as ImmutableDict and just use the constructor.

[vchuravy]

Implementation looks good and love the additional stuff this is building up to. I think there's a need for more general API docs on some of this but I don't know if it's necessarily important to bog down this PR since some of the details will depend on what happens moving forward with other stuff in #46453.

Yeah I kinda would want #46453 to happen first so that I don't have to make the API calls here xD.
There is the alternative world where I just add PersistentDict as an internal implementation detail and delay exposing it to #46453.

[Tokazama]

Yeah I kinda would want #46453 to happen first so that I don't have to make the API calls here xD. There is the alternative world where I just add PersistentDict as an internal implementation detail and delay exposing it to #46453.

The problem with that PR is that it implements a ton of stuff that isn't currently used in Base, so there's little incentive to merge it and take on the maintenance burden. On the other hand, there are places that it could/should be used in base but isn't because the methods don't yet exist. I think doing some of it here without committing to a public API yet may be a good step forward (shows necessity of methods without prematurely committing to a public API).

[vchuravy]

Yeah I agree with that reasoning and that's why I think going with Base.setindex is the right call for now.

[LilithHafner]

IMO nothing this PR adds should be part of the public API. It's good to add the feature and use it internally in #50958 to work out the quirks before committing to anything.

By all means, pay careful attention to the internal API of functions that are defined here/used by #50958, but I wouldn't put any of it in the manual except explicitly marked as internal. And, because we don't have #50105, it might even be good to note that methods and functions this adds are internal in their docstrings.

[vchuravy]

Note for triage: Two decisions need to be made.

1. Are we okay with this additional complexity for a feature where we do not know the adoption rate yet?
   IMO that I rather have a predictable access cost for a scoped value irrespective of depth or amount of scoped values in the system.
2. This adds a second persistent dictionary type to Base (the first being ImmtuableDict) there has been some work on a proper API definition for persistent data-types (Functional non-mutating methods. #46453) and I am adding Base.delete here and making Base.setindex into a full-API. If we don't want this yet, I can turn setindex into a constructor mirroring ImmutableDict and only add delete (technically I don't need delete for ScopedValue, but for other consumers it's an important function, ImmutableDict doesn't have it and only allows shadowing so we could do that as well.)

[vchuravy]

Triage: delete is fine. Let's not use Base.setindex

[LilithHafner]

IIRC triage was against adding any new public API with this PR. (Because this PR already has a lot going on, not because we were opposed to this functionality eventually finding its way into the API)

Edit to add public

[Tokazama]

IIRC triage was against adding any new API with this PR. (Because this PR already has a lot going on, not because we were opposed to this functionality eventually finding its way into the API)

That's not what I got. I understood it to mean we do our best here with an internal and well documented API that might change and be finalized into a public API at some unknown date.

[LilithHafner]

I was imprecise in my previous comment. I agree with @Tokazama's recounting and believe we are in agreement. I have edited my previous comment to try to clarify.