From 4d988584eea4d96a71be1ff4c6f74c39b22e5b52 Mon Sep 17 00:00:00 2001 From: XperiAndri Date: Tue, 20 Oct 2026 22:41:56 +0200 Subject: [PATCH 1/3] fix: `IndexedList.ofList` and `ofSeq` of an F# list copy the list before they build the tree MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An F# list implements `IReadOnlyList`, and its indexer walks the list from its head. `ImmutableList.CreateRange` builds the tree by indexing a list that implements `IReadOnlyList`, so the build took time that grows with the square of the length: 0.43 ms for 1000 elements, 41 ms for 10 000 and 5.9 s for 100 000. The copy in an array takes 8 µs, 90 µs and 1.4 ms, and the tree is built from the array. Co-Authored-By: Claude Sonnet 5.5 --- .../IndexedList/IndexedList.fs | 12 +++++++++-- .../IndexedList/IndexedList.fsi | 6 +++++- .../IndexedList/IndexedListTests.fs | 21 +++++++++++++++++++ 3 files changed, 36 insertions(+), 3 deletions(-) diff --git a/src/FSharp.Collections.Immutable/IndexedList/IndexedList.fs b/src/FSharp.Collections.Immutable/IndexedList/IndexedList.fs index dd8044f5..c5efb11d 100644 --- a/src/FSharp.Collections.Immutable/IndexedList/IndexedList.fs +++ b/src/FSharp.Collections.Immutable/IndexedList/IndexedList.fs @@ -76,7 +76,12 @@ let ofArray (array : 'T array) = ImmutableList.CreateRange array [] -let ofList (list : 'T list) = ImmutableList.CreateRange list +let ofList (list : 'T list) = + // An F# list is an IReadOnlyList whose indexer walks the list from its head, and the BCL builds a tree by indexing a + // list that implements IReadOnlyList: ImmutableList.CreateRange of a list took 0.43 ms for 1000 elements, 41 ms for + // 10 000 and 5.9 s for 100 000, a time that grows with the square of the length. The copy in an array takes 8 µs, 90 µs + // and 1.4 ms for the same lists, and the tree is built from the array. + ofFreshArray (List.toArray list) [] let ofReadOnlySpan (span : ReadOnlySpan<'T>) = ImmutableList.Create<'T> span @@ -85,7 +90,10 @@ let ofReadOnlySpan (span : ReadOnlySpan<'T>) = ImmutableList.Create<'T> span let ofSeq (source : seq<'T>) = // A default flat list given as a sequence is a null input; the BCL would fail while it enumerates it (ADR 0001, D6) checkNotNullOrDefaultSeq (nameof source) source - ImmutableList.CreateRange source + + match source with + | :? ('T list) as list -> ofList list + | _ -> ImmutableList.CreateRange source [] let ofSpan (span : Span<'T>) = ImmutableList.Create<'T>(Span<'T>.op_Implicit span) diff --git a/src/FSharp.Collections.Immutable/IndexedList/IndexedList.fsi b/src/FSharp.Collections.Immutable/IndexedList/IndexedList.fsi index 6d2fbbce..132c1c92 100644 --- a/src/FSharp.Collections.Immutable/IndexedList/IndexedList.fsi +++ b/src/FSharp.Collections.Immutable/IndexedList/IndexedList.fsi @@ -65,7 +65,10 @@ val ofArray : array : 'T array -> IndexedList<'T> /// Creates an indexed list from a list. /// The input list. /// The indexed list of the elements of the list. -/// O(n). +/// +/// O(n). The elements are copied into an array first, because the BCL builds the tree by indexing and the indexer of an +/// F# list walks the list from its head, so that building the tree from the list itself takes O(n²). +/// [] val ofList : list : 'T list -> IndexedList<'T> @@ -89,6 +92,7 @@ val ofReadOnlySpan : span : ReadOnlySpan<'T> -> IndexedList<'T> /// /// O(n). The function wraps , /// which returns an indexed list itself, without a copy. +/// An F# list is copied into an array first, as does. /// [] val ofSeq : source : seq<'T> -> IndexedList<'T> diff --git a/tests/FSharp.Collections.Immutable.Tests/IndexedList/IndexedListTests.fs b/tests/FSharp.Collections.Immutable.Tests/IndexedList/IndexedListTests.fs index 8d3645bf..fde638a0 100644 --- a/tests/FSharp.Collections.Immutable.Tests/IndexedList/IndexedListTests.fs +++ b/tests/FSharp.Collections.Immutable.Tests/IndexedList/IndexedListTests.fs @@ -909,3 +909,24 @@ type IndexedListTests () = $"IndexedList.%s{name} must throw the type of exception of List" ) Assert.AreEqual(expected.Message, ex.Message, $"IndexedList.%s{name} must throw the message of List") + + /// An F# list is an `IReadOnlyList` whose indexer walks the list from its head, and the BCL builds a tree by indexing, + /// so `ImmutableList.CreateRange` of a list takes time that grows with the square of the length: about 6 s for 100 000 + /// elements and four times as much for 200 000, where the copy that `ofList` makes takes a few milliseconds. The timeout is + /// far above the time of the copy and a fraction of the time of the quadratic build. + [] + member _.``ofList and ofSeq copy a long list before they build the tree`` () = + let expected = Array.init 200_000 id + let list = List.ofArray expected + + CollectionAssert.AreEqual ( + expected, + IndexedList.toArray (IndexedList.ofList list), + "IndexedList.ofList must keep the elements of a long list in order" + ) + + CollectionAssert.AreEqual ( + expected, + IndexedList.toArray (IndexedList.ofSeq list), + "IndexedList.ofSeq must keep the elements of a long list in order" + ) From 849d6b6c2330e839ffcfbd3a9fe870416ffca98f Mon Sep 17 00:00:00 2001 From: XperiAndri Date: Wed, 21 Oct 2026 02:47:07 +0200 Subject: [PATCH 2/3] perf: `IndexedList` walks a list of reference types through the interface `ImmutableList.GetEnumerator` returns a struct, which is the fastest way to walk a list of a value type. The code of a generic function is shared by all reference types, and each call of a member of a generic struct in it needs a lookup of the exact type at run time: walking the list through `IEnumerable` took 0.56 to 0.70 times as long for strings. The internal module `Walk` has the loops `iter`, `iteri`, `fold` and `tryPick`; `typeof<'T>.IsValueType` picks the enumerator, and the body of a loop is a lambda that the compiler inlines into both loops. `iter`, `iteri`, `fold`, `mapi`, `scan`, `collect`, `choose` and `pick`/`tryPick` use them. For strings of 100 to 100 000 elements `iter` and `fold` took 0.6 to 0.7 times as long, `tryPick` 0.6, `mapi` and `scan` 0.7 to 0.9; nothing changed for integers. `mapFold` keeps its loop: compiled into the library, the loop of `Walk` took 1.1 to 1.4 times as long there for strings, with machine code that differs from the faster loop only in the order of a copy of a struct. Co-Authored-By: Claude Sonnet 5.5 --- .../FSharp.Collections.Immutable.fsproj | 1 + .../Helper/Walk.fs | 100 ++++++++++++++++++ .../IndexedList/IndexedList.fs | 62 ++++------- .../IndexedList/IndexedListTests.fs | 76 +++++++++++++ 4 files changed, 195 insertions(+), 44 deletions(-) create mode 100644 src/FSharp.Collections.Immutable/Helper/Walk.fs diff --git a/src/FSharp.Collections.Immutable/FSharp.Collections.Immutable.fsproj b/src/FSharp.Collections.Immutable/FSharp.Collections.Immutable.fsproj index f66316ea..7600d6ac 100644 --- a/src/FSharp.Collections.Immutable/FSharp.Collections.Immutable.fsproj +++ b/src/FSharp.Collections.Immutable/FSharp.Collections.Immutable.fsproj @@ -37,6 +37,7 @@ + diff --git a/src/FSharp.Collections.Immutable/Helper/Walk.fs b/src/FSharp.Collections.Immutable/Helper/Walk.fs new file mode 100644 index 00000000..8ea26c3b --- /dev/null +++ b/src/FSharp.Collections.Immutable/Helper/Walk.fs @@ -0,0 +1,100 @@ +/// +/// The loops over the elements of an , which walk it with the +/// enumerator that is the faster one for the type of the elements (ADR 0001, D5). +/// +/// +/// +/// ImmutableList<T>.GetEnumerator returns a struct. For a value type that is the fastest way to walk the list: the +/// compiler knows the struct, and its calls are direct and inlined. For a reference type the code of a generic function is +/// shared by all reference types, and each call of a member of a generic struct in it needs a lookup of the exact type +/// at run time. Walking the list through instead, with a boxed +/// enumerator and calls through the interface, took 0.56 to 0.70 times as long for strings (6 against 10 ns for an +/// element) and 1.1 to 1.5 times as long for integers. As functions of the library, iter and fold took 0.6 to 0.7 times +/// as long for strings of 100 to 100 000 elements, tryPick 0.6, mapi and scan 0.7 to 0.9, and none of them changed for +/// integers. The loop of mapFold stays as it was, because the same loop took 1.1 to 1.4 times as long there. +/// +/// +/// The test typeof<'T>.IsValueType is a constant for the compiler of the run time, so each instantiation of a +/// function keeps one of the two loops. The functions are inline, and the body of a loop is passed as a lambda that the +/// compiler inlines into both loops, so that a body is written once. A body cannot capture a mutable local, which is why +/// and +/// count and carry the state themselves. +/// +/// +module internal FSharp.Collections.Immutable.Walk + +open System.Collections.Immutable + +/// +/// Calls an action for each element of a list, in order. +/// +/// The action. +/// The list, which is not . +let inline iter ([] action : 'T -> unit) (list : ImmutableList<'T>) = + if typeof<'T>.IsValueType then + for item in list do + action item + else + for item in (list :> seq<'T>) do + action item + +/// +/// Calls an action for each element of a list, in order, with the index of the element. +/// +/// The action. +/// The list, which is not . +let inline iteri ([] action : int -> 'T -> unit) (list : ImmutableList<'T>) = + let mutable index = 0 + + if typeof<'T>.IsValueType then + for item in list do + action index item + index <- index + 1 + else + for item in (list :> seq<'T>) do + action index item + index <- index + 1 + +/// +/// Threads a state through the elements of a list, in order. +/// +/// The function that returns the next state for the state and the element. +/// The first state. +/// The list, which is not . +/// The last state. +let inline fold ([] folder : 'State -> 'T -> 'State) (state : 'State) (list : ImmutableList<'T>) = + let mutable state = state + + if typeof<'T>.IsValueType then + for item in list do + state <- folder state item + else + for item in (list :> seq<'T>) do + state <- folder state item + + state + +/// +/// The first value that a chooser returns for the elements of a list, in order, without a call for the elements after it. +/// +/// The chooser. +/// The list, which is not . +/// The first value, or . +let inline tryPick ([] chooser : 'T -> 'U voption) (list : ImmutableList<'T>) = + let mutable result = ValueNone + + if typeof<'T>.IsValueType then + let mutable enumerator = list.GetEnumerator () + + try + while result.IsNone && enumerator.MoveNext () do + result <- chooser enumerator.Current + finally + enumerator.Dispose () + else + use enumerator = (list :> seq<'T>).GetEnumerator() + + while result.IsNone && enumerator.MoveNext () do + result <- chooser enumerator.Current + + result diff --git a/src/FSharp.Collections.Immutable/IndexedList/IndexedList.fs b/src/FSharp.Collections.Immutable/IndexedList/IndexedList.fs index c5efb11d..b36bf3db 100644 --- a/src/FSharp.Collections.Immutable/IndexedList/IndexedList.fs +++ b/src/FSharp.Collections.Immutable/IndexedList/IndexedList.fs @@ -28,18 +28,7 @@ let private range (list : IndexedList<'T>) (start : int) (count : int) = ofFreshArray items /// The first value that the chooser returns for the elements, in their order. -let private firstChoice (chooser : 'T -> 'U voption) (list : IndexedList<'T>) = - let mutable enumerator = list.GetEnumerator () - - try - let mutable result = ValueNone - - while result.IsNone && enumerator.MoveNext () do - result <- chooser enumerator.Current - - result - finally - enumerator.Dispose () +let private firstChoice (chooser : 'T -> 'U voption) (list : IndexedList<'T>) = Walk.tryPick chooser list /// A range of an indexed list sorted stably: the range is sorted in a copy, with LINQ, which sorts stably, and put back /// in place of the range. ImmutableList.Sort(Int32, Int32, IComparer) is not stable (ADR 0001, D5). @@ -513,10 +502,13 @@ let choose (chooser : 'T -> 'U voption) (list : IndexedList<'T>) = // elements; there is no tree of an input to join to its tree here, as collect and concat have (see Collectors). let chosen = ResizeArray<'U>() - for item in list do - match chooser item with - | ValueSome value -> chosen.Add value - | ValueNone -> () + Walk.iter + (fun item -> + match chooser item with + | ValueSome value -> chosen.Add value + | ValueNone -> () + ) + list ImmutableList.CreateRange chosen @@ -545,9 +537,7 @@ let collect (mapping : 'T -> IndexedList<'U>) (list : IndexedList<'T>) = // The collector copies the short results and joins the long ones to its tree as they are (see Collectors) let collector = Collectors.ListCollector<'U>() - for item in list do - collector.Add (mapping item) - + Walk.iter (fun item -> collector.Add (mapping item)) list collector.ToImmutable () [] @@ -623,12 +613,7 @@ let map3 (mapping : 'T1 -> 'T2 -> 'T3 -> 'U) (list1 : IndexedList<'T1>) (list2 : let mapi (mapping : int -> 'T -> 'U) (list : IndexedList<'T>) = checkNotNull (nameof list) list let results = Array.zeroCreate list.Count - let mutable index = 0 - - for item in list do - results[index] <- mapping index item - index <- index + 1 - + Walk.iteri (fun index item -> results[index] <- mapping index item) list ofFreshArray results [] @@ -854,12 +839,7 @@ let compareWith (comparer : 'T -> 'T -> int) (list1 : IndexedList<'T>) (list2 : [] let fold (folder : 'State -> 'T -> 'State) (state : 'State) (list : IndexedList<'T>) = checkNotNull (nameof list) list - let mutable state = state - - for item in list do - state <- folder state item - - state + Walk.fold folder state list [] let fold2 (folder : 'State -> 'T1 -> 'T2 -> 'State) (state : 'State) (list1 : IndexedList<'T1>) (list2 : IndexedList<'T2>) = @@ -888,8 +868,7 @@ let iter (action : 'T -> unit) (list : IndexedList<'T>) = checkNotNull (nameof list) list // ForEach calls a delegate for every element; the loop calls the function directly - for item in list do - action item + Walk.iter action list [] let iter2 (action : 'T1 -> 'T2 -> unit) (list1 : IndexedList<'T1>) (list2 : IndexedList<'T2>) = @@ -903,11 +882,7 @@ let iter2 (action : 'T1 -> 'T2 -> unit) (list1 : IndexedList<'T1>) (list2 : Inde [] let iteri (action : int -> 'T -> unit) (list : IndexedList<'T>) = checkNotNull (nameof list) list - let mutable index = 0 - - for item in list do - action index item - index <- index + 1 + Walk.iteri action list [] let iteri2 (action : int -> 'T1 -> 'T2 -> unit) (list1 : IndexedList<'T1>) (list2 : IndexedList<'T2>) = @@ -922,6 +897,10 @@ let iteri2 (action : int -> 'T1 -> 'T2 -> unit) (list1 : IndexedList<'T1>) (list let mapFold (mapping : 'State -> 'T -> struct ('Result * 'State)) (state : 'State) (list : IndexedList<'T>) = checkNotNull (nameof list) list let results = Array.zeroCreate list.Count + + // Not a Walk loop: for strings, the loop of Walk took 1.1 to 1.4 times as long as this one once it was compiled into the + // library, although its machine code differs only in the order of a copy of the struct that the mapping returns, and + // the same source in a script ran 0.8 times as long. The loops of iter, fold, mapi and scan took 0.6 to 0.9 times as long. let mutable state = state let mutable index = 0 @@ -1060,12 +1039,7 @@ let scan (folder : 'State -> 'T -> 'State) (state : 'State) (list : IndexedList< checkNotNull (nameof list) list let states = Array.zeroCreate (list.Count + 1) states[0] <- state - let mutable index = 0 - - for item in list do - states[index + 1] <- folder states[index] item - index <- index + 1 - + Walk.iteri (fun index item -> states[index + 1] <- folder states[index] item) list ofFreshArray states [] diff --git a/tests/FSharp.Collections.Immutable.Tests/IndexedList/IndexedListTests.fs b/tests/FSharp.Collections.Immutable.Tests/IndexedList/IndexedListTests.fs index fde638a0..43ed65b6 100644 --- a/tests/FSharp.Collections.Immutable.Tests/IndexedList/IndexedListTests.fs +++ b/tests/FSharp.Collections.Immutable.Tests/IndexedList/IndexedListTests.fs @@ -930,3 +930,79 @@ type IndexedListTests () = IndexedList.toArray (IndexedList.ofSeq list), "IndexedList.ofSeq must keep the elements of a long list in order" ) + + /// The functions that walk the list use the enumerator of the interface for reference types (see Walk), a path that + /// the contracts, over integers, do not reach. + [] + member _.``the functions that walk a list give the results of List for strings`` (length : int) = + let items = Array.init length (fun index -> $"item %d{index}") + let list = IndexedList.ofArray items + let expected = List.ofArray items + let visited = ResizeArray() + IndexedList.iter visited.Add list + CollectionAssert.AreEqual (items, visited.ToArray (), "IndexedList.iter must visit the strings in order") + + let visitedAt = ResizeArray() + IndexedList.iteri (fun index item -> visitedAt.Add $"%d{index}:%s{item}") list + + CollectionAssert.AreEqual ( + expected + |> List.mapi (fun index item -> $"%d{index}:%s{item}") + |> List.toArray, + visitedAt.ToArray (), + "IndexedList.iteri must pass the index and the string in order" + ) + + let fold (total : int) (item : string) = total * 31 + item.Length + + Assert.AreEqual( + List.fold fold 7 expected, + IndexedList.fold fold 7 list, + "IndexedList.fold must thread the state through the strings in order" + ) + + CollectionAssert.AreEqual ( + expected + |> List.mapi (fun index item -> $"%d{index}:%s{item}") + |> List.toArray, + IndexedList.mapi (fun index item -> $"%d{index}:%s{item}") list + |> IndexedList.toArray, + "IndexedList.mapi must map the strings in order" + ) + + CollectionAssert.AreEqual ( + expected |> List.scan fold 7 |> List.toArray, + IndexedList.scan fold 7 list |> IndexedList.toArray, + "IndexedList.scan must give the states for the strings" + ) + + let pick (item : string) = + if item.EndsWith ('7') then + ValueSome item.Length + else + ValueNone + + Assert.AreEqual( + expected + |> List.tryPick (fun item -> pick item |> ValueOption.toOption) + |> ValueOption.ofOption, + IndexedList.tryPick pick list, + "IndexedList.tryPick must return the first value for the strings" + ) + + CollectionAssert.AreEqual ( + expected + |> List.choose (fun item -> pick item |> ValueOption.toOption) + |> List.toArray, + IndexedList.choose pick list |> IndexedList.toArray, + "IndexedList.choose must keep the chosen values for the strings" + ) + + CollectionAssert.AreEqual ( + expected + |> List.collect (fun item -> [ item; item ]) + |> List.toArray, + IndexedList.collect (fun item -> IndexedList.ofArray [| item; item |]) list + |> IndexedList.toArray, + "IndexedList.collect must concatenate the results for the strings" + ) From d381e3fd1d597adea7c8bd958c2d544e3625d38f Mon Sep 17 00:00:00 2001 From: XperiAndri Date: Tue, 20 Oct 2026 23:57:54 +0200 Subject: [PATCH 3/3] perf: `choose` collects the elements in an array that is rented from the pool for a long list The elements that `IndexedList.choose` and `IIndexedList.choose` choose waited in a `ResizeArray` that doubles its array as it fills. For a million elements it allocated 18 % (integers) to 35 % (strings) more than the tree needs, and the arrays of its growth in the large object heap made the collections of the second generation more frequent. The result is at most as long as the list, so the elements now wait in an array of that length: a new one below 1024 elements, where the array is cheap, and one rented from `ArrayPool.Shared` above, which allocates nothing in the steady state. The rented array is at most 1 048 576 elements long, so that a sparse selection from a huge list does not rent an array of the length of the list; a longer result moves into a list. A rented array is cleared as far as it was written when it holds references, and is not returned when the chooser throws. A million integers allocate 46 875 KB, the size of the tree, where 55 067 KB were allocated before, and a million strings 46 875 KB where 63 259 KB were. Measured with one variant in a process, in a loop that calls the function again and again, the list caused up to 11 collections of the second generation per 1000 calls for 100 000 elements chosen out of 100 000, and the array none. The time did not change for integers of up to 10 000 elements and for a million, and was up to 10 % less for 100 000; with the enumerator of the previous commit strings took 0.65 to 0.9 times as long. A buffer of chunks joined to the result as subtrees was not taken: it took no less time than the array, and the indexer of its tree was 6 to 10 % slower (the average of two runs, 100 to 100 000 elements). Co-Authored-By: Claude Sonnet 5.5 --- .../FSharp.Collections.Immutable.fsproj | 1 + .../Helper/Buffers.fs | 125 ++++++++++++++++++ .../IndexedList/IIndexedList.fs | 8 +- .../IndexedList/IndexedList.fs | 14 +- .../FSharp.Collections.Immutable.Tests.fsproj | 1 + .../IndexedList/ChosenBufferTests.fs | 71 ++++++++++ .../IndexedList/IIndexedListTests.fs | 15 +++ .../IndexedList/IndexedListTests.fs | 15 +++ 8 files changed, 241 insertions(+), 9 deletions(-) create mode 100644 src/FSharp.Collections.Immutable/Helper/Buffers.fs create mode 100644 tests/FSharp.Collections.Immutable.Tests/IndexedList/ChosenBufferTests.fs diff --git a/src/FSharp.Collections.Immutable/FSharp.Collections.Immutable.fsproj b/src/FSharp.Collections.Immutable/FSharp.Collections.Immutable.fsproj index 7600d6ac..5fe67a27 100644 --- a/src/FSharp.Collections.Immutable/FSharp.Collections.Immutable.fsproj +++ b/src/FSharp.Collections.Immutable/FSharp.Collections.Immutable.fsproj @@ -38,6 +38,7 @@ + diff --git a/src/FSharp.Collections.Immutable/Helper/Buffers.fs b/src/FSharp.Collections.Immutable/Helper/Buffers.fs new file mode 100644 index 00000000..bea7f019 --- /dev/null +++ b/src/FSharp.Collections.Immutable/Helper/Buffers.fs @@ -0,0 +1,125 @@ +/// +/// The buffer in which a function collects the elements that it chooses one by one, until it builds the tree of the result +/// (ADR 0001, D5). +/// +/// +/// +/// A result of unknown length waits in a that doubles its array as it +/// fills, and the tree is built once, from its elements. The growth costs the copies, and for a result of some 20 000 +/// integers or 10 000 references or more the arrays of the growth are allocated in the large object heap, which a collection +/// of the second generation reclaims; for a million elements the list allocated 18 % (integers) to 35 % (strings) more than +/// the tree needs. +/// +/// +/// The number of elements to choose from is known, and it bounds the result. The buffer takes an array of that length: a +/// new one for a short result, where the array is cheap and the pool would only add its own work, and one rented from +/// for a long one, which allocates nothing in the steady state. An array +/// of the length of the source is more than the result needs when few of the elements are chosen, and it would be the +/// array of a hundred million elements for a list of that length. The rented array is therefore at most +/// elements long; a longer result moves into a list. +/// +/// +/// A buffer that is not turned into a list, because the chooser threw, is not returned to the pool. The pool allocates +/// another array, and nothing leaks. +/// +/// +module internal FSharp.Collections.Immutable.Buffers + +open System +open System.Buffers +open System.Collections.Generic +open System.Collections.Immutable +open System.Runtime.CompilerServices +open System.Runtime.InteropServices + +/// +/// The least length of the source for which the buffer is rented from the pool. +/// +/// +/// For 4 to 2048 elements, an array of its own and a rented one took the same time, 0 to 20 % less than the list when all +/// of the elements were chosen; the first call that rents an array of a length allocates it, and the first call took 6 to +/// 81 µs for 4 to 2048 elements. +/// An array below the limit is a small allocation that the next collection of the youngest generation reclaims. +/// +[] +let PooledFrom = 1024 + +/// +/// The most elements in the array that the buffer rents; a result that is longer moves into a list. +/// +[] +let PooledUpTo = 1048576 + +/// +/// Collects the elements that a function chooses from a source of a known length, and builds the indexed list of them. +/// +/// The type of the elements. +/// +/// A buffer is used once: ends it. +/// +[] +type ChosenBuffer<'T> (bound : int) = + + let pooled = bound >= PooledFrom + + let mutable buffer : 'T array = + if bound = 0 then + Array.Empty<'T>() + elif pooled then + ArrayPool<'T>.Shared.Rent(min bound PooledUpTo) + else + Array.zeroCreate bound + + let mutable count = 0 + let mutable overflow : ResizeArray<'T> | null = null + + /// Returns the rented array to the pool, cleared as far as it was written when it holds references. The buffer is + /// left with an empty array, which is full at once, so that an element goes to the slow path of Add. + member private _.Release () = + if pooled && buffer.Length > 0 then + if RuntimeHelpers.IsReferenceOrContainsReferences<'T>() then + Array.Clear (buffer, 0, count) + + ArrayPool<'T>.Shared.Return(buffer, false) + + buffer <- Array.Empty<'T>() + count <- 0 + + /// Moves the elements of the full array into a list that grows, and gives the array back. + member private this.Spill () : ResizeArray<'T> = + let spilled = ResizeArray<'T>(buffer.Length * 2) + CollectionsMarshal.SetCount (spilled, count) + ReadOnlySpan<'T>(buffer, 0, count).CopyTo(CollectionsMarshal.AsSpan spilled) + this.Release () + overflow <- spilled + spilled + + [] + member private this.AddSlow (value : 'T) = + match overflow with + | null -> this.Spill().Add value + | spilled -> spilled.Add value + + /// + /// Appends an element. + /// + /// The element. + [] + member this.Add (value : 'T) = + if count = buffer.Length then + this.AddSlow value + else + buffer[count] <- value + count <- count + 1 + + /// + /// The indexed list of the elements that were added, in their order. + /// + /// The indexed list, which is empty when nothing was added. + member this.ToImmutable () : ImmutableList<'T> = + match overflow with + | null -> + let result = ImmutableList.Create<'T>(ReadOnlySpan<'T>(buffer, 0, count)) + this.Release () + result + | spilled -> ImmutableList.Create<'T>(Span<'T>.op_Implicit(CollectionsMarshal.AsSpan spilled)) diff --git a/src/FSharp.Collections.Immutable/IndexedList/IIndexedList.fs b/src/FSharp.Collections.Immutable/IndexedList/IIndexedList.fs index 5e3356ff..ec7aecff 100644 --- a/src/FSharp.Collections.Immutable/IndexedList/IIndexedList.fs +++ b/src/FSharp.Collections.Immutable/IndexedList/IIndexedList.fs @@ -370,17 +370,15 @@ let append (list1 : IIndexedList<'T>) (list2 : IIndexedList<'T>) = [] let choose (chooser : 'T -> 'U voption) (list : IIndexedList<'T>) = checkNotNull (nameof list) list - // The number of chosen elements is not known in advance, so they wait in a ResizeArray and CreateRange builds the - // balanced tree once. A builder rebalances its tree after every Add and took 2.3 times as long for 100 000 - // elements; there is no tree of an input to join to its tree here, as collect and concat have (see Collectors). - let chosen = ResizeArray<'U>() + // As IndexedList.choose: the chosen elements wait in an array as long as the list (see Buffers) + let chosen = Buffers.ChosenBuffer<'U> list.Count for item in list do match chooser item with | ValueSome value -> chosen.Add value | ValueNone -> () - ImmutableList.CreateRange chosen + chosen.ToImmutable () [] let chunkBySize chunkSize (list : IIndexedList<'T>) = diff --git a/src/FSharp.Collections.Immutable/IndexedList/IndexedList.fs b/src/FSharp.Collections.Immutable/IndexedList/IndexedList.fs index b36bf3db..5c09ce2a 100644 --- a/src/FSharp.Collections.Immutable/IndexedList/IndexedList.fs +++ b/src/FSharp.Collections.Immutable/IndexedList/IndexedList.fs @@ -497,10 +497,16 @@ let append (list1 : IndexedList<'T>) (list2 : IndexedList<'T>) = [] let choose (chooser : 'T -> 'U voption) (list : IndexedList<'T>) = checkNotNull (nameof list) list - // The number of chosen elements is not known in advance, so they wait in a ResizeArray and CreateRange builds the - // balanced tree once. A builder rebalances its tree after every Add and took 2.3 times as long for 100 000 + // The chosen elements wait in an array as long as the list, the most that the result can have: a new one below 1024 + // elements, and one rented from the pool above (see Buffers). The ResizeArray that was here doubles its array as it + // fills, and allocated 18 % more than the tree needs for a million integers and 35 % more for a million strings; the + // arrays of its growth in the large object heap made the collections of the second generation more frequent. The + // array took the same time for integers of 10 to 10 000 elements and up to 10 % less above; a buffer of chunks that + // are joined to the result as subtrees left a tree whose indexer was 6 to 10 % slower (the average of two runs, 100 to + // 100 000 elements), and it took no less time than the array. A builder rebalances its tree + // after every Add and took 3.3 to 3.6 times as long for integers and 2.5 times for strings of 100 000 to a million // elements; there is no tree of an input to join to its tree here, as collect and concat have (see Collectors). - let chosen = ResizeArray<'U>() + let chosen = Buffers.ChosenBuffer<'U> list.Count Walk.iter (fun item -> @@ -510,7 +516,7 @@ let choose (chooser : 'T -> 'U voption) (list : IndexedList<'T>) = ) list - ImmutableList.CreateRange chosen + chosen.ToImmutable () [] let chunkBySize chunkSize (list : IndexedList<'T>) = diff --git a/tests/FSharp.Collections.Immutable.Tests/FSharp.Collections.Immutable.Tests.fsproj b/tests/FSharp.Collections.Immutable.Tests/FSharp.Collections.Immutable.Tests.fsproj index 12eeec14..b1950bfc 100644 --- a/tests/FSharp.Collections.Immutable.Tests/FSharp.Collections.Immutable.Tests.fsproj +++ b/tests/FSharp.Collections.Immutable.Tests/FSharp.Collections.Immutable.Tests.fsproj @@ -73,6 +73,7 @@ + diff --git a/tests/FSharp.Collections.Immutable.Tests/IndexedList/ChosenBufferTests.fs b/tests/FSharp.Collections.Immutable.Tests/IndexedList/ChosenBufferTests.fs new file mode 100644 index 00000000..fcdebf08 --- /dev/null +++ b/tests/FSharp.Collections.Immutable.Tests/IndexedList/ChosenBufferTests.fs @@ -0,0 +1,71 @@ +namespace FSharp.Collections.Immutable.Tests + +open System.Buffers +open FSharp.Collections.Immutable +open Microsoft.VisualStudio.TestTools.UnitTesting + +/// The buffer in which `choose` collects the elements: an array of its own below `Buffers.PooledFrom` elements, one rented +/// from the pool above, and a list for a result that does not fit the rented array. +[] +type ChosenBufferTests () = + + [] + [] + [] + [] + [] + [] + [] + [] + [] + member _.``the buffer builds the list of the elements that were added`` (bound : int, count : int) = + let buffer = Buffers.ChosenBuffer bound + + for value in 0 .. count - 1 do + buffer.Add value + + CollectionAssert.AreEqual ( + Array.init count id, + buffer.ToImmutable () |> IndexedList.toArray, + "The buffer must give the elements that were added, in order" + ) + + [] + member _.``a result longer than the rented array moves into a list`` () = + let count = Buffers.PooledUpTo + 10 + let buffer = Buffers.ChosenBuffer count + + for value in 0 .. count - 1 do + buffer.Add value + + let list = buffer.ToImmutable () + Assert.HasCount (count, list, "The buffer must keep all elements of a result longer than the rented array") + Assert.AreEqual( + Buffers.PooledUpTo - 1, + list[Buffers.PooledUpTo - 1], + "The last element of the rented array must stay" + ) + Assert.AreEqual( + Buffers.PooledUpTo, + list[Buffers.PooledUpTo], + "The first element after the rented array must follow it" + ) + Assert.AreEqual(count - 1, list[count - 1], "The last element must stay") + + [] + member _.``the array that a buffer gives back holds no references`` (count : int) = + let buffer = Buffers.ChosenBuffer count + + for _ in 1..count do + buffer.Add "x" + + buffer.ToImmutable () |> ignore + let rented = ArrayPool.Shared.Rent count + + try + let leftovers = + rented + |> Array.filter (fun item -> not (obj.ReferenceEquals (item, null))) + Assert.IsEmpty (leftovers, "The pool must not keep the elements that a buffer held") + finally + ArrayPool.Shared.Return rented diff --git a/tests/FSharp.Collections.Immutable.Tests/IndexedList/IIndexedListTests.fs b/tests/FSharp.Collections.Immutable.Tests/IndexedList/IIndexedListTests.fs index 464efd97..2a714632 100644 --- a/tests/FSharp.Collections.Immutable.Tests/IndexedList/IIndexedListTests.fs +++ b/tests/FSharp.Collections.Immutable.Tests/IndexedList/IIndexedListTests.fs @@ -674,6 +674,21 @@ type IIndexedListTests () = Assert.AreEqual(-1, IIndexedList.indexOf [| 1; 2 |] arrays, "IIndexedList.indexOf must compare arrays by reference") + /// The contracts choose from short lists, which a new array holds; a long list goes through the rented array, and the + /// selectivities give the results of all lengths, down to a few elements. + [] + member _.``choose of a long list keeps the chosen elements in order`` (percent : int) = + let items = Array.init 100_000 id + let list : IIndexedList = IndexedList.ofArray items + + CollectionAssert.AreEqual ( + items + |> Array.choose (fun x -> if x % 100 < percent then Some (x * 2) else None), + IIndexedList.choose (fun x -> if x % 100 < percent then ValueSome (x * 2) else ValueNone) list + |> IIndexedList.toArray, + $"IIndexedList.choose must keep the chosen elements of a long list in order (%d{percent} %% chosen)" + ) + /// The sequence-like contract for `IIndexedList` over an implementation that is not an indexed list. The functions that /// take a shortcut for indexed lists go the general way here, and must give the same results. [] diff --git a/tests/FSharp.Collections.Immutable.Tests/IndexedList/IndexedListTests.fs b/tests/FSharp.Collections.Immutable.Tests/IndexedList/IndexedListTests.fs index 43ed65b6..dd070ade 100644 --- a/tests/FSharp.Collections.Immutable.Tests/IndexedList/IndexedListTests.fs +++ b/tests/FSharp.Collections.Immutable.Tests/IndexedList/IndexedListTests.fs @@ -1006,3 +1006,18 @@ type IndexedListTests () = |> IndexedList.toArray, "IndexedList.collect must concatenate the results for the strings" ) + + /// The contracts choose from short lists, which a new array holds; a long list goes through the rented array, and the + /// selectivities give the results of all lengths, down to a few elements. + [] + member _.``choose of a long list keeps the chosen elements in order`` (percent : int) = + let items = Array.init 100_000 id + + CollectionAssert.AreEqual ( + items + |> Array.choose (fun x -> if x % 100 < percent then Some (x * 2) else None), + IndexedList.ofArray items + |> IndexedList.choose (fun x -> if x % 100 < percent then ValueSome (x * 2) else ValueNone) + |> IndexedList.toArray, + $"IndexedList.choose must keep the chosen elements of a long list in order (%d{percent} %% chosen)" + )