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.

Sign up for GitHub

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

Jump to bottom

Refactor CE checking #17464

Merged
merged 22 commits into from
Aug 6, 2024
Merged

Refactor CE checking #17464

merged 22 commits into from
Aug 6, 2024

Conversation

vzarytovskii
Copy link
Member

  1. Move CE checking functions out of TcComputationExpression.
  2. Pass all arguments explicitly (before they were captured inside) - not the prettiest, but makes it possible to argue about perf and optimizations.
  3. Perf is not a goal now, benchmarks show no degradation, slight improvement if anything in hyperfine:

Before:
Time (mean ± σ): 2.215 s ± 0.071 s [User: 2.266 s, System: 0.269 s]
Range (min … max): 2.119 s … 2.332 s 100 runs

This PR:
Time (mean ± σ): 2.151 s ± 0.048 s [User: 2.222 s, System: 0.251 s]
Range (min … max): 2.081 s … 2.224 s 100 runs

This PR is only a refactoring and is a prerequisite before I will start tinkering with CE checking.

Test file:
testcode.fs.zip

@vzarytovskii vzarytovskii requested a review from a team as a code owner July 30, 2024 12:40
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jul 30, 2024 •
edited
Loading

⚠️ Release notes required, but author opted out

Warning

Author opted out of release notes, check is disabled for this pull request.
cc @dotnet/fsharp-team-msft

@vzarytovskii vzarytovskii added the NO_RELEASE_NOTES Label for pull requests which signals, that user opted-out of providing release notes label Jul 30, 2024
@vzarytovskii vzarytovskii marked this pull request as draft July 30, 2024 19:50
@dotnet dotnet deleted a comment from azure-pipelines bot Jul 31, 2024
@dotnet dotnet deleted a comment from azure-pipelines bot Jul 31, 2024
@dotnet dotnet deleted a comment from azure-pipelines bot Jul 31, 2024
@vzarytovskii
Copy link
Member Author

So, good news is that it's not this pr causes proto build crash. Bad news is that I don't know what causes it or what are the consequences. I am going to build it in loop on devbox until postmortem debugger kicks in

@dotnet dotnet deleted a comment from azure-pipelines bot Jul 31, 2024
@dotnet dotnet deleted a comment from azure-pipelines bot Aug 1, 2024
@dotnet dotnet deleted a comment from azure-pipelines bot Aug 1, 2024
@dotnet dotnet deleted a comment from azure-pipelines bot Aug 1, 2024
@dotnet dotnet deleted a comment from azure-pipelines bot Aug 1, 2024
@dotnet dotnet deleted a comment from azure-pipelines bot Aug 1, 2024
@vzarytovskii
Copy link
Member Author

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@vzarytovskii
Copy link
Member Author

It seems that the reason for this crash is the issue in p6 runtime has (there are a bunch of crashes in Rosyln have been reported). It seems it doesn't crash on p4 or p7. I will proceed with this PR (I have some change to introduce a wrapper type to pass into all functions, and do it one-by-one).

@edgarfgp
Copy link
Contributor

edgarfgp commented Aug 5, 2024

One thing to consider is that this PR might put back some removed code in previous PRs due to refactor nature

@vzarytovskii
Copy link
Member Author

vzarytovskii commented Aug 5, 2024 •
edited
Loading

One thing to consider is that this PR might put back some removed code in previous PRs due to refactor nature

Hardly anything has changed in the CE checking in a long while, pretty sure, should be ok

@vzarytovskii
Copy link
Member Author

That's ready as soon as tests are passing.

@vzarytovskii vzarytovskii marked this pull request as ready for review August 5, 2024 19:26
@vzarytovskii
Copy link
Member Author

Oh, Azure DevOps seems to be not feeling very well now. Might take a bit before CI passes :|

@vzarytovskii
Copy link
Member Author

/run fantomas

@github-actions
Automated command ran: fantomas
a7b1845 
  Co-authored-by: vzarytovskii <[email protected]>
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Aug 5, 2024

Ran fantomas: https://github.com/dotnet/fsharp/actions/runs/10256051400

@vzarytovskii vzarytovskii merged commit 6f66b01 into dotnet:main Aug 6, 2024
28 checks passed
@vzarytovskii vzarytovskii deleted the refactor-ce-checking branch August 6, 2024 08:17
psfinaki pushed a commit that referenced this pull request Aug 6, 2024
@dotnet-bot @vzarytovskii @github-actions
Refactor CE checking (#17464) (#17493)
6b4c4d5 
Co-authored-by: Vlad Zarytovskii <[email protected]>
Co-authored-by: vzarytovskii <[email protected]>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
This was referenced Aug 13, 2024
Closed
Closed
@dotnet-bot dotnet-bot mentioned this pull request Aug 28, 2024
Closed
vzarytovskii added a commit that referenced this pull request Oct 7, 2024
@dotnet-bot @vzarytovskii
@psfinaki @T-Gro @KevinRansom @github-actions @mmitche
Merge release/dev17.12 to main (#17842)
c4850d6 
* Update azure-pipelines.yml

* Refactor CE checking (#17464) (#17493)

Co-authored-by: Vlad Zarytovskii <[email protected]>
Co-authored-by: vzarytovskii <[email protected]>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

* Update azure-pipelines.yml

* Localized file check-in by OneLocBuild Task: Build definition ID 499: Build ID 2524346 (#17610)

* Fixes #17447 -MethodAccessException on equality comparison of a record with private fields (#17467)

* Fix17447

* tests + readme

* fixes #17541 - Equals visibility for DU's (#17548)

* update version number

* Switch to new VMR control set (#17703) (port from main) (#17788)

* Now that fsharp is on 9.0, we can switch to the new control set. Generally:
- DotNetBuildFromSource -> DotNetBuildSourceOnly - Building a source-only build.
- DotnetBuildFromSourceFlavor == Product -> DotNetBuildOrchestrator == true - Building in the VMR, could be source-only or MS's build.
- ArcadeBuildFromSource -> DotNetBuildRepo == true -> Indicates an outer repo build.
- ExcludeFromSourceBuild -> ExcludeFromSourceOnlyBuild

* Split out source build args

* Split out source build args

* Remove unnecessary source build env var set

* Add properties to the bootstrap compiler build

* BuildRepo -> BuildInnerRepo

* Only build proto repo in inner build

* Additional VMR properties for completeness

* Rename sourcebuild.props -> dotnetbuild.props

---------

Co-authored-by: Petr <[email protected]>

* respect generic arity in method uniqueness

* [17.12] Turn off realsig when building product and proto (#17808)

* Bugfix : make sure nullness does not break XmlDoc info import for methods and types (#17741)

* Remove nullness signal in string-based type encoding of a symbol (since it is used for xmldoc lookup)

* release notes

* Discard unused values

* Rendering AllowsRefStruct for type parameters (#17706)

* Update azure-pipelines.yml

---------

Co-authored-by: Vlad Zarytovskii <[email protected]>
Co-authored-by: Petr <[email protected]>
Co-authored-by: Tomas Grosup <[email protected]>
Co-authored-by: Kevin Ransom (msft) <[email protected]>
Co-authored-by: vzarytovskii <[email protected]>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Kevin Ransom <[email protected]>
Co-authored-by: Matt Mitchell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@T-Gro T-Gro T-Gro approved these changes

@edgarfgp edgarfgp edgarfgp approved these changes

@psfinaki psfinaki psfinaki approved these changes

Assignees
No one assigned
Labels
NO_RELEASE_NOTES Label for pull requests which signals, that user opted-out of providing release notes
Projects
F# Compiler and Tooling
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@vzarytovskii @edgarfgp @psfinaki @T-Gro