Delay Creation of ElementLocation

[yuehuang010]

Issue

• Drafted as in memory project have different characteristics as those from disk.

Many ElementLocation objects are created for Verify() but is often not used outside of an error. So instead of creating an new object, add an interface IInternalLocation to the XML objects and pass the interface instead. When needed, Verify() would call get_Location() to get the IElementLocation.

Note

ProjectTaskElement.EnsureParametersInitialized() still creates locations and caches the location. It would be great to delay it too.

Tests

Each line is a run of a "helloworld" vcxproj project doing load/evaluation/unload 1000 times averaged. Forced GC afterwards. First run discarded. Project created with Microsoft.Build.Evaluation.Project() then Zombify() to dispose.

After:
Evaluation average 497046tick + GC 66284tick = 563331tick
Evaluation average 496872tick + GC 65720tick = 562592tick
Evaluation average 495546tick + GC 65837tick = 561384tick
Evaluation average 501929tick + GC 66190tick = 568119tick
Evaluation average 498541tick + GC 66604tick = 565145tick
Evaluation average 497091tick + GC 65755tick = 562846tick
Evaluation average 494466tick + GC 65964tick = 560431tick
Evaluation average 498267tick + GC 65885tick = 564153tick

Before:
Evaluation average 487367tick + GC 64650tick = 552017tick
Evaluation average 478148tick + GC 64545tick = 542694tick
Evaluation average 478841tick + GC 64590tick = 543431tick
Evaluation average 479940tick + GC 64787tick = 544728tick
Evaluation average 494942tick + GC 66813tick = 561755tick
Evaluation average 506728tick + GC 65485tick = 572214tick
Evaluation average 498566tick + GC 65650tick = 564217tick
Evaluation average 500238tick + GC 65723tick = 565961tick
Evaluation average 497829tick + GC 65577tick = 563406tick

Evaluation average 551038tick + GC 67453tick = 618491tick
Evaluation average 549966tick + GC 67914tick = 617881tick
Evaluation average 546807tick + GC 67633tick = 614440tick
Evaluation average 554436tick + GC 68152tick = 622589tick
Evaluation average 547378tick + GC 66956tick = 614335tick

[yuehuang010]

Using the interface named "IInternalLocation" to expose the internal IElementLocation. "ILocation" is a better name public name. Thoughts?

[rokonec]

@yuehuang010 I have reviewed this PR and I do not understand how are those changes improving perf or GC.
In current implementation Location object is always created when XML doc is parsed because it is only time when we know location of element in its file. Various Location properties getters almost never creates the Location object but returns reference to already created Location object instance. Passing reference to facade interface is not, IMHO, faster than passing reference to already existing (no Lazy creation here) Location object.

On the other hand, the presented performance measurement show improvements. Wonder what I am missing...

What tooling did you use to collect this perf samples?

[yuehuang010]

@yuehuang010 I have reviewed this PR and I do not understand how are those changes improving perf or GC. In current implementation Location object is always created when XML doc is parsed because it is only time when we know location of element in its file. Various Location properties getters almost never creates the Location object but returns reference to already created Location object instance. Passing reference to facade interface is not, IMHO, faster than passing reference to already existing (no Lazy creation here) Location object.

Before, XML object -> get_Location -> Verify. After, XML object -> Verify -> get_Location. Verify only needs location when it prints a message. In most use case, it doesn't need it.

On the other hand, the presented performance measurement show improvements. Wonder what I am missing...

What tooling did you use to collect this perf samples?

Updated description on perf collection methods.

[rokonec]

I have looked at the code again and to me situation looks like this:
before: XML Object -> Creates Location object in ctor -> get_Location; gets already created and cached object -> Verfiy
after: XML Object -> Creates Location object in ctor -> Verfiy -> conditionally invokes IInternalLocation.get_Location which returns the same object already constructed in XML Object ctor

So the amount of Location object allocations is same in both cases - if I understand it correctly, there is no lazy Location object creation.

[yuehuang010]

Good call. I found an issue. In my case, the XmlElementWithLocation.OwnerDocument.FullPath is null, so it never match with current location, thus it will always create a new copy of ElementLocation. Let me re-run baseline.

[yuehuang010]

Drafted this PR and created #6998 for address the null check. @rokonec

[Forgind]

Team triage:
It seems like we spend virtually no time allocating memory for locations (which are pretty small), and with the other bug you fixed, do we still need this?

[yuehuang010]

@Forgind, I have moved this pr back to draft. Don't like draft, feel free to close it.