Update targets to reduce syntax tree recreations due to ParseOption changes #78093

ToddGrun · 2025-04-10T02:51:08Z

Additionally, reduce false positives in telemetry reporting inefficiencies in project system interactions wrt Preprocessor directives.

…hanges Additionally, reduce false positives in telemetry reporting inefficiencies in project system interactions wrt Preprocessor directives.

jasonmalinowski · 2025-04-10T05:17:15Z

src/Compilers/Core/MSBuildTask/Microsoft.CSharp.Core.targets

    <CommandLineArgsForDesignTimeEvaluation Condition="'$(ChecksumAlgorithm)' != ''">$(CommandLineArgsForDesignTimeEvaluation) -checksumalgorithm:$(ChecksumAlgorithm)</CommandLineArgsForDesignTimeEvaluation>
    <CommandLineArgsForDesignTimeEvaluation Condition="'$(DefineConstants)' != ''">$(CommandLineArgsForDesignTimeEvaluation) -define:$(DefineConstants)</CommandLineArgsForDesignTimeEvaluation>
+    <CommandLineArgsForDesignTimeEvaluation Condition="'$(Features)' != ''">$(CommandLineArgsForDesignTimeEvaluation) -features:$(Features)</CommandLineArgsForDesignTimeEvaluation>
+    <CommandLineArgsForDesignTimeEvaluation Condition="'$(DocumentationFile)' != ''">$(CommandLineArgsForDesignTimeEvaluation) -doc:$(DocumentationFile)</CommandLineArgsForDesignTimeEvaluation>


This might need some quoting. Now, ironically, if all the IDE cares about is if it's set or not set, then it's kinda fine if it doesn't get quoting and only the first part of the file name is treated as part of the switch. But we should probably either quote, or leave a comment here that we don't actually care.

Quotes FTW!

jasonmalinowski

Overall looks fine, some suggestions but that's just some improvements.

jasonmalinowski · 2025-04-10T05:32:16Z

src/Workspaces/Core/Portable/Workspace/ProjectSystem/ProjectSystemProject.cs

+                            TryReportCompilationThrownAway(currentSolution, Id);
                    }
                }
            }


If you could also make a paranoia change here: we should perhaps move all this sending of telemetry after the _projectPropertyModificationsInBatch.Add() line, so if this were to throw an exception, we're not corrupting state.

Sounds good

jasonmalinowski · 2025-04-10T05:36:47Z

src/Workspaces/Core/Portable/Workspace/ProjectSystem/ProjectSystemProject.cs

+                        {
+                            if (oldValue is ParseOptions oldParseOptions)
+                            {
+                                var projectState = currentSolution.SolutionState.GetRequiredProjectState(Id);


Rather than fetching the project to get language services, you should be able to shortcut and just do _projectSystemProjectFactory.SolutionServices.GetRequiredLanguageService(Language). That use of CurrentSolution should be fine, but this is probably shorter and doesn't require me to think about it.

Yeah, that's nicer.

jasonmalinowski · 2025-04-10T05:37:42Z

src/Workspaces/Core/Portable/Workspace/ProjectSystem/ProjectSystemProject.cs

+                        if (newValue is ParseOptions newParseOption)
+                        {
+                            if (oldValue is ParseOptions oldParseOptions)
+                            {


Can we just do this logic in the ParseOptions setter and pass in logThrowAwayTelemetry appropriately? That way we're not doing this checks in other property tests, and we also skip all the stuff above which is probably not cheap either.

I'm a bit hesitant to move it to the caller as the code is currently running inside the _gate lock. I'd like to understand the concern a bit better. The code is only executed when logThrowAwayTelemetry is true, so it's just two of the callers, and the added code is only executed before the solution finishes loading.

Moving it out would also mean adding exception handling to ensure the field was still updated

It's just a bit strange that this is applying to a specific case, but it's in the general helper.

Moving it out would also mean adding exception handling to ensure the field was still updated

What do you mean?

The issues I see if the logThrowAwayTelemetry block were called outside and prior to ChangeProjectProperty:

Exception handling would need to be added around the call to that block to ensure ChangeProjectProperty would continue to be called

The block would probably need to be executed under the gate's protection (at least it is now, and I haven't tried to suss out if it need be)

I've added some comments that make things a bit clearer, I'm definitely willing to change things if you think there is a better way.

…onsInBatch 2) Simplify obtaining ISyntaxTreeFactoryService 3) Change version reference 4) Quote potential string containing value in targets files

CyrusNajmabadi · 2025-04-10T19:57:16Z

src/Compilers/Core/MSBuildTask/Microsoft.CSharp.Core.targets

    <CommandLineArgsForDesignTimeEvaluation Condition="'$(ChecksumAlgorithm)' != ''">$(CommandLineArgsForDesignTimeEvaluation) -checksumalgorithm:$(ChecksumAlgorithm)</CommandLineArgsForDesignTimeEvaluation>
    <CommandLineArgsForDesignTimeEvaluation Condition="'$(DefineConstants)' != ''">$(CommandLineArgsForDesignTimeEvaluation) -define:$(DefineConstants)</CommandLineArgsForDesignTimeEvaluation>
+    <CommandLineArgsForDesignTimeEvaluation Condition="'$(Features)' != ''">$(CommandLineArgsForDesignTimeEvaluation) -features:$(Features)</CommandLineArgsForDesignTimeEvaluation>
+    <CommandLineArgsForDesignTimeEvaluation Condition="'$(DocumentationFile)' != ''">$(CommandLineArgsForDesignTimeEvaluation) -doc:"$(DocumentationFile)"</CommandLineArgsForDesignTimeEvaluation>


what do each of these correspond to?

perhaps doc why we want these.

CyrusNajmabadi · 2025-04-10T20:06:02Z

src/Workspaces/Core/Portable/Workspace/ProjectSystem/ProjectSystemProject.cs

                    if (!isFullyLoaded)
                    {
-                        TryReportCompilationThrownAway(_projectSystemProjectFactory.Workspace.CurrentSolution, Id);
+                        var reportCompilationThrownAway = true;


extract local function plz.

CyrusNajmabadi · 2025-04-10T20:08:02Z

src/Workspaces/Core/Portable/Workspace/ProjectSystem/ProjectSystemProject.cs

+                        }
+
+                        if (reportCompilationThrownAway)
+                            TryReportCompilationThrownAway(_projectSystemProjectFactory.Workspace.CurrentSolution, Id);


mentioned offline. but there just isn't that many reasons why a prse option could change. we should actually detect what actually changed and report that as part of telem. For example, i'd love to know thigns like "90% of throwaways were caused by LangVersion changing", or something like that.

CyrusNajmabadi · 2025-04-10T20:50:02Z

src/Workspaces/Core/Portable/Workspace/ProjectSystem/ProjectSystemProject.cs

+        {
+            // Preprocessor directive only ParseOptions changes have special handling in DocumentState.UpdateParseOptionsAndSourceCodeKind.
+            // We don't want to report telemetry for those changes or for the initial evaluation result.
+            if (newValue is not ParseOptions newParseOption)


consider doc'ing the two flows that get here. one that is a ParsOptions one that is not. this was not clear to me :D

src/Workspaces/Core/Portable/Workspace/ProjectSystem/ProjectSystemProject.cs

…stemProject.cs Co-authored-by: Cyrus Najmabadi <cyrus.najmabadi@gmail.com>

ToddGrun · 2025-04-11T22:28:33Z

/azp run

azure-pipelines · 2025-04-11T22:28:50Z

Azure Pipelines successfully started running 3 pipeline(s).

ToddGrun · 2025-04-11T22:29:41Z

@dotnet/roslyn-compiler -- ptal

jjonescz · 2025-04-14T09:09:28Z

src/Compilers/Core/MSBuildTask/Microsoft.CSharp.Core.targets

    <CommandLineArgsForDesignTimeEvaluation Condition="'$(ChecksumAlgorithm)' != ''">$(CommandLineArgsForDesignTimeEvaluation) -checksumalgorithm:$(ChecksumAlgorithm)</CommandLineArgsForDesignTimeEvaluation>
    <CommandLineArgsForDesignTimeEvaluation Condition="'$(DefineConstants)' != ''">$(CommandLineArgsForDesignTimeEvaluation) -define:$(DefineConstants)</CommandLineArgsForDesignTimeEvaluation>
+    <CommandLineArgsForDesignTimeEvaluation Condition="'$(Features)' != ''">$(CommandLineArgsForDesignTimeEvaluation) -features:$(Features)</CommandLineArgsForDesignTimeEvaluation>
+    <CommandLineArgsForDesignTimeEvaluation Condition="'$(DocumentationFile)' != ''">$(CommandLineArgsForDesignTimeEvaluation) -doc:"$(DocumentationFile)"</CommandLineArgsForDesignTimeEvaluation>


I'm not sure how to review this change from the compiler's perspective. Ideally, I would like to see tests with different values passed here (e.g., to verify escaping works). But it seems this is really an IDE only property, not used by the compiler at all, right? In that case LGTM

Yeah, I don't really know how this fits into the compiler side of things, AFAIK this is just used as part of what the IDE feeds into the project build. @jasonmalinowski -- do you think this needs compiler side reviews too?

But it seems this is really an IDE only property, not used by the compiler at all, right? In that case LGTM

Correct; this only impacts the IDE side of things. I guess @jaredpar would be the arbiter here of whether he wants to see more reviews from his team or not.

Want to keep things simple and say that it's in the compiler code, we need two compiler sign offs. Even if the sign offs are as simple as us validating that "this is not compiler code"

jaredpar

Verified compiler changes do not impact compiler.

Update targets to reduce syntax tree recreations due to ParseOption c…

c96f3b2

…hanges Additionally, reduce false positives in telemetry reporting inefficiencies in project system interactions wrt Preprocessor directives.

ToddGrun requested review from a team as code owners April 10, 2025 02:51

ghost added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Apr 10, 2025

dotnet-policy-service bot added the VSCode label Apr 10, 2025

jasonmalinowski reviewed Apr 10, 2025

View reviewed changes

jasonmalinowski approved these changes Apr 10, 2025

View reviewed changes

1) Reorder telemetry logging with updating _projectPropertyModificati…

15551b8

…onsInBatch 2) Simplify obtaining ISyntaxTreeFactoryService 3) Change version reference 4) Quote potential string containing value in targets files

CyrusNajmabadi reviewed Apr 10, 2025

View reviewed changes

Extract to local function and simplify

3e5705d

CyrusNajmabadi reviewed Apr 10, 2025

View reviewed changes

CyrusNajmabadi approved these changes Apr 10, 2025

View reviewed changes

ToddGrun added 3 commits April 10, 2025 13:52

comment update

00fd718

update comments again

2f9723b

remove unused var and mis-spelling

776352c

CyrusNajmabadi reviewed Apr 10, 2025

View reviewed changes

src/Workspaces/Core/Portable/Workspace/ProjectSystem/ProjectSystemProject.cs Outdated Show resolved Hide resolved

CyrusNajmabadi reviewed Apr 10, 2025

View reviewed changes

src/Workspaces/Core/Portable/Workspace/ProjectSystem/ProjectSystemProject.cs Outdated Show resolved Hide resolved

ToddGrun and others added 3 commits April 10, 2025 16:32

Update src/Workspaces/Core/Portable/Workspace/ProjectSystem/ProjectSy…

af386a5

…stemProject.cs Co-authored-by: Cyrus Najmabadi <cyrus.najmabadi@gmail.com>

Update src/Workspaces/Core/Portable/Workspace/ProjectSystem/ProjectSy…

aa80af4

…stemProject.cs Co-authored-by: Cyrus Najmabadi <cyrus.najmabadi@gmail.com>

Cyrus did me wrong

603d832

jjonescz reviewed Apr 14, 2025

View reviewed changes

jaredpar approved these changes Apr 16, 2025

View reviewed changes

jjonescz approved these changes Apr 16, 2025

View reviewed changes

arunchndr merged commit e7069f2 into dotnet:main Apr 18, 2025
31 checks passed

dotnet-policy-service bot added this to the Next milestone Apr 18, 2025

This was referenced Apr 21, 2025

[Automated] PRs inserted in VS build main-10521.04 #78229

Closed

[Automated] PRs inserted in VS build feature.debugger.main-10521.160 #78252

Closed

dotnet-bot mentioned this pull request May 31, 2025

[Automated] PRs inserted in VS build feature.telemetry.d18OpenTelemetry-10630.96 #78782

Closed

RikkiGibson modified the milestones: Next, 18.0 P1 Aug 19, 2025

Update targets to reduce syntax tree recreations due to ParseOption changes #78093

Update targets to reduce syntax tree recreations due to ParseOption changes #78093

Uh oh!

Conversation

ToddGrun commented Apr 10, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasonmalinowski left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ToddGrun commented Apr 11, 2025

Uh oh!

azure-pipelines bot commented Apr 11, 2025

Uh oh!

ToddGrun commented Apr 11, 2025

Uh oh!

jjonescz Apr 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaredpar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

jjonescz Apr 14, 2025 •

edited

Loading