Skip to content

Minor perf opt #417

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

Merged
merged 1 commit into from
May 13, 2025
Merged

Conversation

Thorium
Copy link
Member

@Thorium Thorium commented Apr 15, 2025

  • isNull is inline also in FSharp.Core. It's called in many places.
  • Lazy load of ILAssemblyRefs: There is no point in spending so much time initially to construct ilAssemblyRefs which is only used by GetReferencedAssemblies() call
  • Avoid looping 120 libraries through on most cases: Often convTypeRef is called to get some very basic base-type like a "System.Object" or "System.Int32" thus going through the dependency assemblies one-by-one can be a fall-back method rather than default implementation.

I would love to have some benchmark tests for ProvidedTypes.fs but the issue is that it's mostly used by FSharp.Compiler via a heavy IDE, and current unit-tests are not covering the majority of the process, so I don't have any idea how that could be done. Right now the best way seems to be to attach a VS to VS and open some type-provider and see what is going on.

- isNull is inline also in FSharp.Core
- Faster eqTypes
- Lazy load of ILAssemblyRefs
- Avoid looping 120 libraries through on most cases
@Thorium Thorium force-pushed the designtime-perf-opt branch from 44dec43 to 1f9af4a Compare May 10, 2025 14:32
@Thorium
Copy link
Member Author

Thorium commented May 10, 2025

Same content, just made the commit a tiny bit smaller.

Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR applies minor performance optimizations to the type provider code by reducing unnecessary computations and delaying resource-intensive operations until needed.

  • Changed utility functions (isNull, isNil, isEmpty) to be inline.
  • Introduced lazy initialization for ILAssemblyRefs to defer their construction.
  • Modified ProvidedTypes lookup logic to short-circuit basic type translations while reusing the ProvidedTypeSymbol check.
Comments suppressed due to low confidence (1)

src/ProvidedTypes.fs:6256

  • Ensure that the lazy initialization is thread-safe in contexts where ILAssemblyRefs may be accessed concurrently. If concurrent access is possible, consider specifying an explicit thread-safety mode or verifying that the default Lazy behavior meets the requirements.
let ilAssemblyRefs = lazy [ for i in 1 .. getNumRows ILTableNames.AssemblyRef do yield seekReadAssemblyRef i ]

@Thorium
Copy link
Member Author

Thorium commented May 13, 2025

  • Ensure that the lazy initialization is thread-safe in contexts where ILAssemblyRefs may be accessed concurrently. If concurrent access is possible, consider specifying an explicit thread-safety mode or verifying that the default Lazy behavior meets the requirements.

The type ILModuleReader in ProvidedTypes.fs uses a lot of lazy keyword without lazy (lock (System.Object ()) (fun () ->-pattern. For example, see the seekReadTopExportedTypes and seekReadNestedExportedTypes just above this call. Thus, I expect the type is not shared with multiple threads but rather initialised multiple times if VisualStudio decides to use multiple threads to call this.

@sergey-tihon sergey-tihon merged commit 3b22420 into fsprojects:master May 13, 2025
2 checks passed
@sergey-tihon
Copy link
Member

Changes look reasonable. Thanks @Thorium for the PR and explanation.

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