fsprojects / FSharpx.Extras

Functional programming and other utilities from the original "fsharpx" project
https://fsprojects.github.io/FSharpx.Extras/
The Unlicense
682 stars 146 forks source link

Faster and less-memory-consuming niceName #434

Closed Thorium closed 2 months ago

Thorium commented 2 months ago

This makes niceName function a bit faster and consume less memory.

Thorium commented 2 months ago

Mac CI build-failure is not related to this commit.

This function has even more tweaked version in here, but that uses Spans which are not available in this project.

Edit: (AsSpan etc are coming from System.Memory NuGet package, or as part of .Net Standard 2.1/NET6/NET8)

gdziadkiewicz commented 2 months ago

Hi, thanks for contributing! I will check it today 😄

gdziadkiewicz commented 2 months ago

Can you refresh with master? I fixed the macos failing build

gdziadkiewicz commented 2 months ago

Hi, I read through the code and am happy with it, but I still need to do one last check if the changes in @?, LetterDigit, Upper, and Lower are breaking for the current users.

Thorium commented 2 months ago

If they appear to be, there could easily be both internal and external versions of those functions.

gdziadkiewicz commented 2 months ago

Hi, I got a repro for the problematic situation: https://github.com/gdziadkiewicz/fsharpx-extras-binary-compatibility. I don't mind incrementing the major version, but please give me a moment to think about other breaking changes we could include during the increment (do you have any ideas?)

Thorium commented 2 months ago

There is no reason why nice-name would need to use the special @? operator so if you want to keep that as option, that's totally doable and a small change. Should I modify this PR?

Thorium commented 2 months ago

The reason I needed the optimization: In SQLProvider we use this function on intellisense on database, on every table, on every column. Databases can be big. And with intellisense, you expect that when you type a dot, the system reacts now and not a second later.

Just a tiny test on my laptop (after 4 warm-up runs on both):

open System
let someNames = [|"testThing"; "test3X"; "BIG"; "this_is_test"|] |> Array.replicate 500000 |> Array.concat
#time;;

// FSharpX.Extras Old:
someNames |> Array.map niceName;;
// Real: 00:00:01.240, CPU: 00:00:01.234, GC gen0: 16, gen1: 5, gen2: 1

// FSharpX.Extras New:
someNames |> Array.map niceName;;
// Real: 00:00:00.655, CPU: 00:00:00.296, GC gen0: 0, gen1: 0, gen2: 0
gdziadkiewicz commented 2 months ago

@Thorium If you add the ValueOption-based satisfies and active patterns as private code, then we can merge and release a new, quicker niceName even today (sorry for the additional work!) with a patch.

As for the general move to ValueOption for API, I will create a separate issue and do it all at once with a major version bump

Thorium commented 2 months ago

This should be good now.