Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat!: change 'event lists/seq to consistently use arrays #411

Merged
merged 6 commits into from
Jul 26, 2023

Conversation

bartelink
Copy link
Collaborator

@bartelink bartelink commented Jul 25, 2023

Changes the standard signatures to remove variation that was not earning its keep:

  • Changed the standard fold signature from 'state -> 'event seq -> 'state to 'state -> 'event[] -> 'state
    • Very few stores did not already internally have them as arrays; it's not a perf concern
    • Means let fold = Array.fold evolve never needs type annotations
    • More clearly conveys that states tend to be evolved in batches (and e.g. that if you are able to derive a state from the last event, you can do an Array.last)
    • In tests, using [| comprehensions vs seq { tends to produce less noise
  • Changed the Decider's interpret and decide function signatures to use [] instead of list
    • The current implementation wants and needs it to be an array in the end, in order to be able to determine whether a Sync is required, so any perf or allocations argument is not relevant
    • While there are some cases where cons'ing a list together provides a pleasing solution, the advent of array comprehensions with implicit yield in F# 6 is simply a better more general pattern
  • Changed DeciderCore's interpret and decide function signatures to use [] instead of seq
    • While C# provides the generator pattern for IEnumerable via yield break...
      • they should also implement array comprehensions; I'm betting it'll happen this side of 2030
      • the core impl materialises it as an Array anyway, so no perf gain
      • Conversion overload shims can be added pretty easily
    • The Decider impl can more directly use the Array impls, which makes the whole thing more legible

This leaves the key differences between the Decider and DeciderCore interfaces clearer:

  • Func vs FSharpFunc (mapped internally to Func)
  • passing CancellationTokens everywhere vs everything just working
  • struct ValueTuples vs System.Tuple (open to nudging on this)
    • the perf overhead of sprinkling in ValueTuple.Create is not the point, async is already way more allocation heavy anyway
    • switching between async and task atm involves introducing struct noise in code
    • Propulsion's F# interfaces atm don't use struct tuples either

The base of this work is identical to #409; Props to @nordfjord for thinking it could be done, nudging me for ages, and then proving it!

@bartelink bartelink marked this pull request as ready for review July 25, 2023 21:14
Base automatically changed from cats-have-names to master July 26, 2023 08:54
@bartelink bartelink force-pushed the event-arrays-clone branch from 8f22c3b to 105ab67 Compare July 26, 2023 10:06
@bartelink bartelink changed the title Use Arrays for Events and more feat!: Use Arrays for Events and more Jul 26, 2023
@bartelink bartelink changed the title feat!: Use Arrays for Events and more feat!: Event lists/seqs are arrays everywhere Jul 26, 2023
@bartelink bartelink force-pushed the event-arrays-clone branch 4 times, most recently from 3c7106e to 3c59e2c Compare July 26, 2023 12:08
@bartelink bartelink force-pushed the event-arrays-clone branch from 9d42688 to c7fe71b Compare July 26, 2023 12:50
@bartelink bartelink changed the title feat!: Event lists/seqs are arrays everywhere feat!: change 'event lists/seq to consistently use arrays Jul 26, 2023
@bartelink bartelink merged commit 6df7c58 into master Jul 26, 2023
@bartelink bartelink deleted the event-arrays-clone branch July 26, 2023 13:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants