dotnet / dotnet/fsharp

Type 'Ast.Ident' allocations high when parsing TypeChecker.fs

Open
#8,250 0 comments 2 reactions 0 assignees View on GitHub
Feature Improvement
Dominant language
F#
Stars
4.3k
Forks
876
Avg merge
4d 11h
Merged PRs (30d)
131

Description

![Untitled](https://user-images.githubusercontent.com/1278959/72575497-7c907500-3881-11ea-874f-812e099211f6.png)

This is just when parsing `TypeChecker.fs` and nothing else.

Now, I think perfview might actually be including `SynExpr.Ident` as part of this because it's probably not splitting them out by enclosing type, but after drilling down to what the actual calls are, the majority are calls involving `Ast.Ident`.

`Ast.Ident` is actually a struct and is getting boxed by our parser. If we want to keep it as a struct, the only way to stop the boxing from happening is to modify the generator in some way.

If we make `Ident` a class, we still allocate, but just a little bit less:
![Untitled2](https://user-images.githubusercontent.com/1278959/72576461-ae570b00-3884-11ea-9304-0203c7c4cdd5.png)

The goal is to reduce allocations, which I don't know if we can without modifying the generator. If `Ast.Ident` is a class, we can't use an object pool because that would require us to know the lifetime of `Ast.Ident`, which would require us to know the lifetime of some Ast nodes which is really really not feasible.

`Ast.Ident` is a bit of large object:
```fsharp
[]
[]
type Ident (text: string, range: range) =
member x.idText = text
member x.idRange = range
override x.ToString() = text
```
On 64-bit, that's a 24 byte object, 32-bit 20 byte object. 16 bytes is generally the cap you want to be at for a struct, but it depends on how you use it. If you are not passing `Ast.Ident` around a lot, it's probably ok, though whoever is holding onto `Ast.Ident` is also going to be that big, which our syntax nodes do.

This is quite tricky unfortunately. No easy solution. I think we are going to allocate either way in some capacity considering syntax nodes hold an `Ast.Ident`, but if we keep `Ast.Ident` a struct and fixing the boxing issues, we could have a significant reduction. Ideally since `Ast.Ident` is so large, it could just be a `string` and an `int`, where the `int` represents the starting position in the source. Later, you could just compute the final range when you need to. Most of the time you never need the final range unless you want to report an error or want tooling information.

I'd say this is actually low-medium priority considering I looked at the overall allocations in a project, not every file is the size of `TypeChecker.fs`, and `Ast.Ident` isn't at the top.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.