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

Cache Platform Negotiation in graph build #9343

Merged
merged 2 commits into from
Oct 27, 2023

Conversation

rokonec
Copy link
Contributor

@rokonec rokonec commented Oct 18, 2023

Fixes https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1834963

Context

PR #7699 caused perf regression when running large-graphs-with-platform-negotiation kind of build.

Changes:

var projectInstance = projectInstanceFactory(
projectReferenceFullPath,
null, // Platform negotiation requires an evaluation with no global properties first

have caused that dependency projects were evaluated for Platform Negotiation purpose (with null global properties) for each incoming dependency.

Changes Made

Caching "evaluation for Platform Negotiation purpose".

Testing

Unit tests.
Local.

Notes

Without changes it was running 6K evaluations (for VS/VC build) and it took 132 s.
image

After changes only 1.5K evaluations with duration of 33 s.
image

The fact that every dependency has to be specially evaluated for Platform Negotiation purposes has big enough, IMO, negative impact on performance, to reevaluate this approach.

@rokonec
Copy link
Contributor Author

rokonec commented Oct 18, 2023

@yuehuang010 It might be related to slowness you have been experiencing.

@yuehuang010
Copy link
Contributor

yuehuang010 commented Oct 18, 2023

Nice improvement! I think the issue is still present, because the evaluation per second remains the same. 6395/132 = 48 vs 1525/33 = 46. This change reduces the total number of evaluations during graph build and thus, reduces the total time. It is still a win.

The issue showed two behaviors, 1) the time spent in GC was really high of over 50%, 2) enabling gcServer speed it up at the cost of more memory. A quick way of testing this is to modify the msbuild.exe.config with

<configuration>
   <runtime>
      <gcServer enabled="true"/>
   </runtime>
</configuration>

@danmoseley
Copy link
Member

Aside, do we know where most of these allocations are? Perhaps in something like XmlReader below MSBuild? 50% GC is collossal.

@yuehuang010
Copy link
Contributor

Aside, do we know where most of these allocations are? Perhaps in something like XmlReader below MSBuild? 50% GC is collossal.

I wasn't able to track it down. Here are my findings. ProjectEvaluationEvent uses a lot of memory, but when I removed it, it made no impact on total time. Enabling filelogger or binarylogger increases graph processing time by 25%. I prototyped a change to discard the Evaluations during graph but that didn't improve the total time. The prototype kept the total memory below the 300mb, so GC won't overreact to high memory usage.

Ruling those out Total memory, my next theories are a bug in GC or a data structure that GC doesn't like. Sadly my timebox ran out, so I wish the next person best of luck.

@rokonec rokonec merged commit efd0158 into dotnet:main Oct 27, 2023
8 checks passed
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.

5 participants