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

FSharp.Compiler.Service: update tagged collections with new implementation from FSharp.Core #10192

Merged
merged 1 commit into from Sep 28, 2020

Conversation

buybackoff
Copy link
Contributor

Copied new implementation from #10188 and #10190

Differences are:

  • TaggedCollections used fat leaves (Left/Right/Height present always). There are 2xN objects in the tree, of which N are leaves. Fat leaves give 20 (24 aligned) x N. More memory, worse memory locality, more GC are not good for perf.
  • TaggedCollections did not use optimized closures. Not sure if they are useful, but in many cases it's noop if f' was already in the right shape.

Copying the source opens the possibility to move MapTree and SetTree code to separate files and reuse a single implementation in both places.

…ation from FSharp.Core

Copied implementation from FSharp.Core

Differences are:

* TaggedCollections used fat leaves (Left/Right/Height present always). There are 2xN objects in the tree, of which N are leaves. Fat leaves give 20 (24 aligned) x N. More memory, worse memory locality, more GC are not good for perf.
* TaggedCollections did not use optimized closures. Not sure if they are useful, but in many cases it's noop if f' was already in the right shape.

Copying the source opens the possibility to move MapTree and SetTree code to separate files and reuse a single implementation in both places.
@cartermp cartermp requested a review from dsyme September 27, 2020 18:21
@cartermp
Copy link
Contributor

I think these look good overall, but this will require a @dsyme check as per here: #10188 (comment)

Though I do personally think we should consolidate on the FSharp.Core implementations

@buybackoff
Copy link
Contributor Author

I have profiled fsc while it builds FSharp.Core.UnitTests in PerfView/dotTrace/dotMemory and couldn't see anything related to Set/Map on the top lines (CPU/allocations). But there is 50% "native or optimized code" - may it hide there after ngen?

@cartermp
Copy link
Contributor

This comes into play more with tooling like in VS than anything else. Various parts of the compiler will hold data in memory for a long time and process it to varying degrees. So any improvements to that processing time and memory usage will be a win. Like most perf improvements, it'll be incremental but will work to make things a lot better over time.

@dsyme
Copy link
Contributor

dsyme commented Sep 28, 2020

I have profiled fsc while it builds FSharp.Core.UnitTests in PerfView/dotTrace/dotMemory and couldn't see anything related to Set/Map on the top lines (CPU/allocations). But there is 50% "native or optimized code" - may it hide there after ngen?

My understanding is that type inference is pretty heavily affected by Set performance, at least in some situations (e.g. possibly lots of unsolved type inference variables interacting with generalization)

Set and Map nodes do occupy a significant chunk of memory and allocations as well.

It would be good to get baselines. I'll try to resurrect the compiler performance script for use with .NET Core.

@dsyme
Copy link
Contributor

dsyme commented Sep 28, 2020

It would be good to get baselines. I'll try to resurrect the compiler performance script for use with .NET Core.

I don't see this as blocking merging the PR, which is fantastic work, thank you!

@cartermp cartermp merged commit af6ff33 into dotnet:main Sep 28, 2020
@forki
Copy link
Contributor

forki commented Sep 28, 2020

Wow.

@buybackoff
Copy link
Contributor Author

My understanding is that type inference is pretty heavily affected by Set performance, at least in some situations (e.g. possibly lots of unsolved type inference variables interacting with generalization)

I wonder if structural sharing is important, or thread safety? It looks like only one CPU is busy during the build time.

nosami pushed a commit to xamarin/visualfsharp that referenced this pull request Feb 23, 2021
…ation from FSharp.Core (dotnet#10192)

Copied implementation from FSharp.Core

Differences are:

* TaggedCollections used fat leaves (Left/Right/Height present always). There are 2xN objects in the tree, of which N are leaves. Fat leaves give 20 (24 aligned) x N. More memory, worse memory locality, more GC are not good for perf.
* TaggedCollections did not use optimized closures. Not sure if they are useful, but in many cases it's noop if f' was already in the right shape.

Copying the source opens the possibility to move MapTree and SetTree code to separate files and reuse a single implementation in both places.
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.

None yet

4 participants