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

More ValueOption in compiler: part 3 (and the last) #16822

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

psfinaki
Copy link
Member

@psfinaki psfinaki commented Mar 6, 2024

Closes #16286

If my RegEx is correct, this should be it.


Some benchmarks (for what was easy to bench):

Before:

Method Mean Error StdDev Gen0 Allocated
TaskCancelled_TargetPattern 9.627 ns 0.4100 ns 1.2024 ns 0.0004 24 B
TaskCancelled_OtherPattern 4.766 ns 0.4129 ns 1.2173 ns - -
StopProcessing_TargetPattern 11.123 ns 0.6545 ns 1.9297 ns 0.0004 24 B
StopProcessing_OtherPattern 4.433 ns 0.2699 ns 0.7957 ns - -
ConstToILFieldInit_TargetPattern 15.940 ns 0.9367 ns 2.7472 ns 0.0008 48 B
ConstToILFieldInit_OtherPattern 2.418 ns 0.0852 ns 0.1621 ns - -

After:

Method Mean Error StdDev Gen0 Allocated
TaskCancelled_TargetPattern 0.0010 ns 0.0037 ns 0.0066 ns - -
TaskCancelled_OtherPattern 3.5823 ns 0.2947 ns 0.8688 ns - -
StopProcessing_TargetPattern 2.2672 ns 0.1675 ns 0.4938 ns - -
StopProcessing_OtherPattern 2.9171 ns 0.2328 ns 0.6863 ns - -
ConstToILFieldInit_TargetPattern 9.8902 ns 0.4454 ns 1.2707 ns 0.0004 24 B
ConstToILFieldInit_OtherPattern 3.0223 ns 0.1553 ns 0.4432 ns - -

@psfinaki psfinaki requested a review from a team as a code owner March 6, 2024 16:47
@psfinaki psfinaki marked this pull request as draft March 6, 2024 16:47
Copy link
Contributor

github-actions bot commented Mar 6, 2024

❗ Release notes required


✅ Found changes and release notes in following paths:

Change path Release notes path Description
src/Compiler docs/release-notes/.FSharp.Compiler.Service/8.0.400.md

@psfinaki psfinaki marked this pull request as ready for review March 6, 2024 17:54
@psfinaki
Copy link
Member Author

psfinaki commented Mar 8, 2024

/azp run

Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@psfinaki psfinaki enabled auto-merge (squash) March 11, 2024 11:06
Copy link
Member

@vzarytovskii vzarytovskii left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll have to wait with this one for now, since now we're having some live issues with SO

@psfinaki psfinaki disabled auto-merge March 18, 2024 14:39
@vzarytovskii vzarytovskii mentioned this pull request Mar 23, 2024
3 tasks
@vzarytovskii
Copy link
Member

I've merged couple of fixes and workarounds for the SOs in CEs and in big nested expressions. We can probably merge this one (extra would be to test AnyCpu variant on those).

@abelbraaksma
Copy link
Contributor

These performance improvements are quite impressive and more than I'd expect from such change. Nice!

@psfinaki
Copy link
Member Author

psfinaki commented May 7, 2024

I ran the current version of fscAnyCpu.exe against the code from this and this PRs, don't notice any regressions there.

@psfinaki psfinaki requested a review from vzarytovskii May 7, 2024 12:43
@psfinaki psfinaki enabled auto-merge (squash) May 7, 2024 15:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

Replace active pattern with structs in the compiler code
4 participants