Skip to content

Faster and less-memory-consuming niceName - #434

Merged
gdziadkiewicz merged 4 commits into
fsprojects:masterfrom
Thorium:niceName-opt
Jul 9, 2024
Merged

gdziadkiewicz merged 4 commits into
fsprojects:masterfrom
Thorium:niceName-opt

Conversation

@Thorium

@Thorium Thorium commented Jun 21, 2024

Copy link
Copy Markdown
Member

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

@Thorium

Thorium commented Jun 21, 2024

Copy link
Copy Markdown
Member Author

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

Copy link
Copy Markdown
Collaborator

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

@gdziadkiewicz gdziadkiewicz self-assigned this Jun 24, 2024
@gdziadkiewicz

Copy link
Copy Markdown
Collaborator

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

@gdziadkiewicz

Copy link
Copy Markdown
Collaborator

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

Thorium commented Jun 26, 2024

Copy link
Copy Markdown
Member Author

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

@gdziadkiewicz

Copy link
Copy Markdown
Collaborator

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

Thorium commented Jul 4, 2024

Copy link
Copy Markdown
Member Author

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

Thorium commented Jul 4, 2024

Copy link
Copy Markdown
Member Author

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

Copy link
Copy Markdown
Collaborator

@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

Thorium commented Jul 9, 2024

Copy link
Copy Markdown
Member Author

This should be good now.

@gdziadkiewicz
gdziadkiewicz merged commit 0aacf26 into fsprojects:master Jul 9, 2024
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