-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Dev/erarndt/project root element #12318
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR optimizes memory usage by trimming the ProjectRootElement
class to reduce object size from 206B per instance, addressing garbage collection performance issues. The optimization targets ~66k objects consuming 13.7MB during OrchardCore builds.
- Removes rarely-used cached fields and debug-only properties
- Replaces boolean
IsEphemeral
property with type-based approach usingEphemeralProjectRootElement
subclass - Eliminates unused
ContainsTargetsWithReturnsAttribute
field
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
File | Description |
---|---|
src/Build/Construction/ProjectRootElement.cs | Main optimization removing cached fields, debug properties, and introducing EphemeralProjectRootElement subclass |
src/Build/Construction/ProjectTargetElement.cs | Removes code that set the now-deleted ContainsTargetsWithReturnsAttribute field |
src/Build/Definition/Project.cs | Updates to use type checking instead of IsEphemeral property and simplifies debug trace messages |
Let's run exp insertion for this PR. |
|
||
if (reason != null) | ||
{ | ||
Trace.WriteLine(String.Format(CultureInfo.InvariantCulture, "MSBUILD: Is dirty because {0} [{1} - {2}] [PC Hash {3}]", reason, FullPath, import.ImportedProject.FullPath == FullPath ? String.Empty : import.ImportedProject.FullPath, ProjectCollection.GetHashCode())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello, according to my search in code, there are several possible dirty reasons.
Are we reasonably sure that they are not in use please?
MarkDirty("Set project FullPath to '{0}'", FullPath); D:\bld\src\Build\Construction\ProjectRootElement.cs 456 17 Microsoft.Build FullPath ProjectRootElement Read
MarkDirty("Project reloaded", null); D:\bld\src\Build\Construction\ProjectRootElement.cs 1732 13 Microsoft.Build ReloadFrom ProjectRootElement Read
MarkDirty("Set express as attribute: {0}", value.ToString()); D:\bld\src\Build\Construction\ProjectElement.cs 92 21 Microsoft.Build ExpressedAsAttribute ProjectElement Read
MarkDirty("CopyFrom", null); D:\bld\src\Build\Construction\ProjectElement.cs 415 13 Microsoft.Build CopyFrom ProjectElement Read
MarkDirty("Replace element {0}", newElement.Name); D:\bld\src\Build\Construction\ProjectElement.cs 467 13 Microsoft.Build ReplaceElement ProjectElement Read
Or maybe (to be on a safer side) since there is a limited pool of the reasons, we could shift to an enum to keep the information while getting rid of the string.
Fixes #
Context
To combat expensive individual garbage collections, it's important to minimize the amount of memory that gets promoted. One way to help is to shrink the size of the objects. This PR trims
ProjectRootElement
. This is a heap snapshot when building OrchardCore.There are roughly 13.7MB of these items on the heap (~66k at 206B per object).
The
_escapedFullPath
is intended to cache the result for theEscapedFullPath
property. However, this is called very infrequently (I saw once per build), so the field provides little value.There are two fields,
_dirtyReason
and_dirtyParameter
that are only used when theMSBUILDDEBUGEVALUATION
environment variable is set. I believe we can remove these fields and still provide useful information by updating the trace messages.The
IsEphemeral
property is rarely used, and it can be covered by having another class that includes this field (EphemeralProjectRootElement
) and used in places where an ephemeral version is needed.Finally,
ContainsTargetsWithReturnsAttribute
was only ever written to, so it can be removed.Changes Made
Testing
Notes