Lazily populate FileIdentifier fields #69882

ToddGrun · 2023-09-11T18:39:49Z

Found while inspecting profiles for turning on/off razor precise semantic tokens. In the code analysis process, I see 1.6% of CPU and 0.4% of allocations spent in MS.CA.C#.FileIdentifier.Create. (which is over half of the CPU time spent in in CreateSemanticModel). I tested this locally by opening the Roslyn sln, waiting several minutes, and saw over 30K FileIdentifier objects created.

One thing I was curious about was which members were being heavily used in this object as there are differing costs associated with populating each of the members. What I found was that, at least for the scenarios I was testing, none of the objects members were being called for any of the 30K FileIdentifier objects, and thus we could lazily populate this object and not likely ever incur those costs.

Found while inspecting profiles for turning on/off razor precise semantic tokens. In the code analysis process, I see 1.6% of CPU and 0.4% of allocations spent in MS.CA.C#.FileIdentifier.Create. (which is over half of the CPU time spent in in CreateSemanticModel). I tested this locally by opening the Roslyn sln, waiting several minutes, and saw over 30K FileIdentifier objects created. One thing I was curious about was which members were being heavily used in this object as there are differing costs associated with populating each of the members. What I found was that, at least for the scenarios I was testing, none of the objects members were being called for any of the 30K FileIdentifier objects, and thus we could lazily populate this object and not likely ever incur those costs.

ToddGrun · 2023-09-12T18:45:50Z

@333fred, @AlekseyTs ptal

ToddGrun · 2023-09-12T18:50:39Z

src/Compilers/CSharp/Portable/Symbols/FileIdentifier.cs

-        string? encoderFallbackErrorMessage = null;
-        ImmutableArray<byte> hash = default;
-        try
+        if (_filePath is not null)


if (_filePath is not null)

Just realized I'm not sure if the properties are called on different threads. I could put _filePath in a local and use that in the method if that is a concern. #Closed

Went ahead and changed it, better safe than sorry

…arate threads.

src/Compilers/CSharp/Portable/Symbols/FileIdentifier.cs

…le enabled

AlekseyTs · 2023-09-12T22:18:56Z

src/Compilers/CSharp/Portable/Symbols/FileIdentifier.cs

    }
+
+    public static FileIdentifier Create(SyntaxTree tree)


Create

Is this helper used? #Closed

AlekseyTs · 2023-09-12T22:19:48Z

Done with review pass (commit 5)

RikkiGibson · 2023-09-12T22:55:49Z

src/Compilers/CSharp/Portable/Binder/Binder_Lookup.cs

@@ -1371,7 +1371,7 @@ FileIdentifier getFileIdentifierForFileTypes()
                    if (binder is BuckStopsHereBinder lastBinder)
                    {
                        // we never expect to bind a file type in a context where the BuckStopsHereBinder lacks an AssociatedFileIdentifier
-                        return lastBinder.AssociatedFileIdentifier.Value;
+                        return lastBinder.AssociatedFileIdentifier;


Let's insert ?? throw ExceptionUtilities.Unreachable() here so we preserve the null test being performed by the original .Value access.

RikkiGibson · 2023-09-12T22:56:49Z

src/Compilers/CSharp/Portable/Symbols/FileIdentifier.cs

@@ -9,33 +9,91 @@

 namespace Microsoft.CodeAnalysis.CSharp;

-internal readonly struct FileIdentifier
+internal class FileIdentifier


nit: sealed

RikkiGibson · 2023-09-12T23:05:04Z

src/Compilers/CSharp/Portable/Symbols/FileIdentifier.cs

+
+            var displayFilePath = GeneratedNames.GetDisplayFilePath(filePath);
+
+            _encoderFallbackErrorMessage = encoderFallbackErrorMessage;


I think we still don't have thread safety with this approach. I could be misunderstanding something, though. My suggestion was:

make all the class fields readonly, and initialize their values in the constructor, rather than EnsureInitialized.

for those types where we want to cache this, e.g. BuckStopsHereBinder, source type symbols, and PE type symbols, include a field of type FileIdentifier?, which initially has value null.

When the accessor e.g. AssociatedFileIdentifier is called, and the field is null, interlocked-initialize the FileIdentifier field with a fully initialized instance and return it.

I think this most conveniently solves the thread safety questions while still giving us the perf win, as most of the binders and symbols will never actually create the instance.

I'm definitely open to other approaches and would like to make sure @333fred and @AlekseyTs would also be comfortable with this approach.

I'm willing to go whatever approach you guys like best. I'm not clear on exactly where the concern lies with the current approach, and not clear on exactly what you are proposing, but if Fred and Aleksey like it better than what this PR has so far, I'm definitely willing to give that a go.

Discussed offline with Aleksey and we think that what I've described is overkill, and also just moves the problem from internal to this type to external to it, in some ways.

I think it will be adequate to instead use the solution you have, but modify the properties like DisplayFilePath, etc. to Volatile.Read the fields, rather than just returning them normally.

The scenario I want to prevent is that we do not get in an inconsistent state, where another thread observes that _filePath is null, but also observes the "derived" fields like _displayFilePath as still null, due to inadequate synchronization.

Instead of doing several Volatile.Reads, would it be sufficient to just do Volatile.Write(ref _filePath, null) when assigning _filePath instead?

Instead of doing several Volatile.Reads, would it be sufficient to just do Volatile.Write(ref _filePath, null) when assigning _filePath instead?

Yes, I think so. Doc says

A volatile write operation prevents earlier memory operations on the thread from being reordered to occur after the volatile write.

That's the way I read it too and it seemed easier. Just wanted to make sure I wasn't missing something.

A volatile write operation prevents earlier memory operations on the thread from being reordered to occur after the volatile write.

I believe Interlocked exchange (above) provides the same guarantees. Also, this doesn't affect other threads, but @RikkiGibson was concerned about reads happening on another thread.

AlekseyTs · 2023-09-13T13:40:02Z

src/Compilers/CSharp/Portable/Symbols/FileIdentifier.cs

+
+            ImmutableInterlocked.InterlockedInitialize(ref _filePathChecksumOpt, hash);
+
+            Volatile.Write(ref _filePath, null);


Volatile.Write(ref _filePath, null);

I believe Interlocked exchange (above) provides the same guarantees, but doing this doesn't hurt. Also, this doesn't affect other threads, but @RikkiGibson was concerned about reads happening on another thread. #Closed

@RikkiGibson -- do you still have concerns?

I just saw Cyrus use StrongBox in another PR for a similar concern. I think I would feel better about _filePathChecksumOpt being a StrongBox? instead of an ImmutableArray

for a similar concern

What concern? I am not aware of any issue with InterlockedInitialize.

Cyrus's fix wasn't around usage of InterlockedInitialize, but was around concurrent read/write of a struct. With the change from commit 7 to commit 8, I no longer need to worry about the _filePathChecksumOpt member getting partially written during another thread reading it. I have no problem changing it back if you are convinced that wouldn't have been a problem.

I do not believe the Volatile.Write alone is sufficient here. That does guarantee that the writes happen in a specific order, however there is nothing ensuring that the reads occur in a specific order. The reads on other threads of _filePath and _displayFilePath can be re-oredered with respect to each other. That effectively undoes the protections you're trying to get with Volatile.Write. Those need to be changed to use Volatile.Read, at least on _filePath to get the protections.

The problem with having these type of ordered writes / reads is they are very difficult to get correct and maintain. Very few people really understand this area. I wasn't even confident in posting my analysis until I'd gotten private buy off that I was remembering the rules correctly. Even if we get this right now it's very easy for the invariants to be broken by future developers who don't understand the significance of the ordering.

I would recommend instead putting all of the data that is calculated here into a single value like Tuple<string, string>. That way there is a single value that is either set or not set. No need for tricky read / write ordering.

I have no problem changing it back if you are convinced that wouldn't have been a problem.

It is a very common pattern that we use in symbols to rely on ImmutableInterlocked.InterlockedInitialize and simply read the array in another place. It feels like StrongBox is an overkill here, or all other places should be fixed up as well.

I like the idea of putting all the data into a single reference type. I'll go with something like the following unless there is a good reason someone has to use Tuple directly instead:

private record FileIdentifierData(string? EncoderFallbackErrorMessage, string DisplayFilePath, ImmutableArray FilePathChecksumOpt);

AlekseyTs · 2023-09-13T13:42:48Z

Done with review pass (commit 7)

AlekseyTs · 2023-09-13T18:11:33Z

src/Compilers/CSharp/Portable/Symbols/FileIdentifier.cs

+
+            _encoderFallbackErrorMessage = encoderFallbackErrorMessage;
+            _displayFilePath = displayFilePath;
+            _filePathChecksumOpt = new(hash);


new

Target typed new is forbidden in Compilers #Closed

src/Compilers/CSharp/Portable/Symbols/FileIdentifier.cs

…he data that is created on demand.

AlekseyTs · 2023-09-14T17:17:46Z

src/Compilers/CSharp/Portable/Symbols/FileIdentifier.cs

 {
+    private record FileIdentifierData(string? EncoderFallbackErrorMessage, string DisplayFilePath, ImmutableArray<byte> FilePathChecksumOpt);


record

Unless I am mistaken, we are not using any of the code generated for this type other than properties. Please switch to a regular class. Also, for private types like this, we often stick with public readonly fields (not even bothering with properties). #Closed

Are primary ctors ok to use?

Are primary ctors ok to use?

As far as I know.

src/Compilers/CSharp/Portable/Symbols/FileIdentifier.cs

AlekseyTs · 2023-09-14T17:32:06Z

Done with review pass (commit 9)

AlekseyTs

LGTM (commit 10)

ToddGrun requested review from 333fred and AlekseyTs September 11, 2023 18:39

ToddGrun requested a review from a team as a code owner September 11, 2023 18:39

dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Sep 11, 2023

ToddGrun mentioned this pull request Sep 12, 2023

Perf: Lazily populate FileIdentifier fields #69905

Closed

ToddGrun linked an issue Sep 12, 2023 that may be closed by this pull request

Perf: Lazily populate FileIdentifier fields #69905

Closed

ToddGrun commented Sep 12, 2023

View reviewed changes

Put member in local in case properties are called concurrently on sep…

a5fe476

…arate threads.

333fred reviewed Sep 12, 2023

View reviewed changes

src/Compilers/CSharp/Portable/Symbols/FileIdentifier.cs Show resolved Hide resolved

Switch FileIdentifier to class to allow safer lazy initialization

8c52c5b

AlekseyTs reviewed Sep 12, 2023

View reviewed changes

src/Compilers/CSharp/Portable/Symbols/FileIdentifier.cs Outdated Show resolved Hide resolved

ToddGrun added 2 commits September 12, 2023 14:45

Remove '?' from return types as the corresponding files aren't nullab…

ae8bd02

…le enabled

Take greater care in struct assignments as they aren't atomic

31860c2

AlekseyTs reviewed Sep 12, 2023

View reviewed changes

RikkiGibson self-assigned this Sep 12, 2023

RikkiGibson reviewed Sep 12, 2023

View reviewed changes

ToddGrun added 2 commits September 12, 2023 16:14

seal class, maintain throw semantics

6d8337e

add volatile read to ensure _filePath is written after the other members

137f7c7

build-analysis bot mentioned this pull request Sep 13, 2023

The active test run was aborted. Reason: Test host process crashed dotnet/dnceng#451

Open

3 tasks

AlekseyTs reviewed Sep 13, 2023

View reviewed changes

Use StrongBox to alleviate concerns about tearing during struct write

89594a4

AlekseyTs reviewed Sep 13, 2023

View reviewed changes

333fred reviewed Sep 13, 2023

View reviewed changes

src/Compilers/CSharp/Portable/Symbols/FileIdentifier.cs Outdated Show resolved Hide resolved

Simplify things considerably by just using a reference type to hold t…

ac96452

…he data that is created on demand.

333fred approved these changes Sep 14, 2023

View reviewed changes

AlekseyTs reviewed Sep 14, 2023

View reviewed changes

src/Compilers/CSharp/Portable/Symbols/FileIdentifier.cs Show resolved Hide resolved

Prefer simple class to record class

3cbfddc

AlekseyTs approved these changes Sep 14, 2023

View reviewed changes

RikkiGibson approved these changes Sep 14, 2023

View reviewed changes

ToddGrun merged commit 638ca0c into dotnet:main Sep 14, 2023
27 checks passed

ghost added this to the Next milestone Sep 14, 2023

allisonchou mentioned this pull request Sep 18, 2023

[Automated] PRs inserted in VS build main-34118.122 #69996

Closed

Cosifne modified the milestones: Next, 17.8 P3 Sep 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Lazily populate FileIdentifier fields #69882

Lazily populate FileIdentifier fields #69882

ToddGrun commented Sep 11, 2023

ToddGrun commented Sep 12, 2023

ToddGrun Sep 12, 2023 •

edited

Loading

ToddGrun Sep 12, 2023

AlekseyTs Sep 12, 2023 •

edited

Loading

ToddGrun Sep 12, 2023

AlekseyTs commented Sep 12, 2023

RikkiGibson Sep 12, 2023

RikkiGibson Sep 12, 2023

RikkiGibson Sep 12, 2023

ToddGrun Sep 12, 2023

RikkiGibson Sep 12, 2023 •

edited

Loading

ToddGrun Sep 12, 2023

RikkiGibson Sep 13, 2023

ToddGrun Sep 13, 2023

AlekseyTs Sep 13, 2023 •

edited

Loading

AlekseyTs Sep 13, 2023 •

edited

Loading

ToddGrun Sep 13, 2023

ToddGrun Sep 13, 2023

AlekseyTs Sep 13, 2023

ToddGrun Sep 13, 2023

jaredpar Sep 13, 2023

jaredpar Sep 13, 2023 •

edited

Loading

AlekseyTs Sep 13, 2023

ToddGrun Sep 13, 2023

AlekseyTs commented Sep 13, 2023

AlekseyTs Sep 13, 2023 •

edited

Loading

AlekseyTs Sep 14, 2023 •

edited

Loading

ToddGrun Sep 14, 2023

AlekseyTs Sep 14, 2023

AlekseyTs commented Sep 14, 2023

AlekseyTs left a comment


		var displayFilePath = GeneratedNames.GetDisplayFilePath(filePath);

		_encoderFallbackErrorMessage = encoderFallbackErrorMessage;


		ImmutableInterlocked.InterlockedInitialize(ref _filePathChecksumOpt, hash);

		Volatile.Write(ref _filePath, null);

		{
		private record FileIdentifierData(string? EncoderFallbackErrorMessage, string DisplayFilePath, ImmutableArray<byte> FilePathChecksumOpt);

Lazily populate FileIdentifier fields #69882

Lazily populate FileIdentifier fields #69882

Conversation

ToddGrun commented Sep 11, 2023

ToddGrun commented Sep 12, 2023

ToddGrun Sep 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs Sep 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs commented Sep 12, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RikkiGibson Sep 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs Sep 13, 2023 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Sep 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaredpar Sep 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs commented Sep 13, 2023

AlekseyTs Sep 13, 2023 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Sep 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs commented Sep 14, 2023

AlekseyTs left a comment

Choose a reason for hiding this comment

ToddGrun Sep 12, 2023 •

edited

Loading

AlekseyTs Sep 12, 2023 •

edited

Loading

RikkiGibson Sep 12, 2023 •

edited

Loading

AlekseyTs Sep 13, 2023 •

edited

Loading

AlekseyTs Sep 13, 2023 •

edited

Loading

jaredpar Sep 13, 2023 •

edited

Loading

AlekseyTs Sep 13, 2023 •

edited

Loading

AlekseyTs Sep 14, 2023 •

edited

Loading