Ah, CI doesn't seem to run on a draft PR.

@auduchinok what figures around memory traffic are you seeing and under what scenario? Is there a before/after you could share?

@cartermp I measured type checking after editing a rather big file in ReSharper.FSharp solution some time ago and remember seeing this method to be responsible for about half of allocations in reading custom attributes during that type check.
I'll try to measure it in a similar way again for comparison and will post it back.

I've opened the PR to have CI run the tests and for getting some feedback but wanted it to be a draft at first.

This looks great, the code is much nicer like this in any case

Just to see what the impact of this is on something that isn't the repro case, I took a look at a recent trace - 72 secs of editing a few files. GetParamDatas accounts for 143MB of allocations:

Where the bulk comes from GetCustomAttrs:

So if this can reduce redundant work here, 👍 even for doing things that the repro case doesn't show as the problem.

As a side note, GetCustomAttrs allocated 335MB in my 72 second sample, so perhaps we could dig into why that is separately

As a side note, GetCustomAttrs allocated 335MB in my 72 second sample, so perhaps we could dig into why that is separately

Just to say we've long had problems with GetCustomAttrs as a cause of either

1. Long term memory usage (if we cache the attributes we read naively - e.g. caching all IL attributes all the time in all cases)
2. Short term repeated CPU+allocation recompute (if we don't cache either the attributes or the information extracted from them at all)

This has applied particularly in some cases where there are zillions of attributes on the target types/methods. The number of attributes in .NET metadata has also been steadily growing over time.

The right solution is likely to be either

1. lazily compute and cache extracted information (i.e. the few bits we care about such as "is this obsolete" etc.) in a higher level object such as the ParamAttributes
2. lazily compute and cache the attributes themselves in that object.

At a quick glance of the code I'm not sure of the effect on this PR - I think it might be making things worse in this regard, as property implementations like this will repeatedly compute the attribute where before it would have been computed once - I suspect each of these boolean flag computations should be cached into the ParamAttributes object?

Thanks @dsyme that's good to know. I think we'll need to test this more thoroughly and look for GetCustomAttrs to see if we end up allocating more with this approach.

I'm going to work on a benchmark dotnet test for type checking this week. Once that is in place, we can test this PR to see the results.

I'm going to work on a benchmark dotnet test for type checking this week. Once that is in place, we can test this PR to see the results.

TBH nothing beats the scenario-based testing like @cartermp has been doing, where we profile using the IDE tools.

Many of the false assumptions in the compiler came from over-testing on compilation scenarios (where long-running memory is not considered expensive, since the process is not long-running), so we originally cached like crazy. IDE scenarios exercise the code very differently with different budgets

So I don't really know what a benchmark would be that corresponds to IDE usage. Capturing a trace of calls into FCS might indicate what it would look like.

TBH nothing beats the scenario-based testing like @cartermp has been doing, where we profile using the IDE tools.

No doubt we should absolutely test the scenarios by profiling like what @cartermp is doing. I just want to have a benchmark test where I can see allocation results and timing quickly in this case; I have had success with this before. It doesn't give insight into the long running memory problem, which is where memory dumps can help.

I hope that makes sense.

No doubt we should absolutely test the scenarios by profiling like what @cartermp is doing. I just want to have a benchmark test where I can see allocation results and timing quickly in this case; I have had success with this before. It doesn't give insight into the long running memory problem, which is where memory dumps can help. I hope that makes sense.

Benchmarks totally make sense. @vasily-kirichenko took a crack at something like this, it might make sense to revive (or replace) that, see benchmarks\CompilerServiceBenchmarks

Conflicts need attention, and I guess we'll wait to see the impact with additional testing.