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

Miscellaneous logging improvements #6326

Merged
merged 14 commits into from Apr 20, 2021
Merged

Miscellaneous logging improvements #6326

merged 14 commits into from Apr 20, 2021

Conversation

KirillOsenkov
Copy link
Member

@KirillOsenkov KirillOsenkov commented Apr 5, 2021

We're almost at the finish line. This PR contains most remaining low-hanging fruit fixes to logging efficiency. There are more fruit of course, but it's no longer clear what else to do to meaningfully reduce binlog size and overhead even further.

The primary insights are:

  • don't log BuildEventArgs.Message where it can be recovered (less strings)
  • log Message separately from Arguments, instead of concatenating into formatted message and logging that (smaller strings, more deduplication)
  • cache string resources instead of retrieving them every time, log resource string and arguments separately
  • add more test coverage for Serial and Parallel console loggers (which is currently sorely lacking, since we mostly use MockLogger in tests)

I tried to separate independent changes into separate commits, so reviewing commit-by-commit is better. For some changes, turn on Ignore Whitespace will help.

Final binlog size and perf numbers are very favorable.

Binlog size:

16.9.2 main This PR
20 MB 3.5 MB 2.7 MB
120 MB 15 MB 10 MB

Incremental build duration on a smaller project:

  16.9.2 This PR
/m /bl 18.6 s 14.1 s
/bl 32 s 18 s
/m 9 s 9 s
no /m no /bl 15 s 15 s

Incremental build for Roslyn.sln:

  16.9.2 This PR
/m /bl 1:11 s 37 s
/bl 2:23 s 1:03 s

For most BuildEventArgs we can completely reconstruct the message text from the other fields. Do not allocate and log the Message string in this case, and override Message implementation to recover the message lazily on demand. This way the message never needs to be allocated to travel across nodes or written/read into binlog. It is only materialized when a console/file logger requests it.

For cases where LazyFormattedBuildEventArgs has Arguments, don't flatten and materialize the Message during node packet translation or binlog write/read.

The two improvements above help significantly reduce string allocations, the number of strings logged and the string size. Smaller strings means that deduplication can do a better job reusing strings, and as such most large strings simply dissolve.

Strings before: Total size: 7,770,723, Count: 33,648, Largest: 55,401
Strings after: Total size: 1,382,154, Count: 18,701, Largest: 10,197

Increment the binlog file format version to 13. Write LazyFormattedBuildEventArgs.Arguments array into the binlog if present.

Do not persist Importance for BuildEventArgs that ignore it or where it defaults to low.
Do not write ThreadId into the binlog as it's never used.
Introduce the internal field BuildEventArgs.RawMessage to expose the underlying message field and avoid the side effects of calling Message (which formats using the Arguments).

Add Condition, EvaluatedCondition and OriginallySucceeded on TargetSkippedEventArgs so we can recover the message.

Do not sort item metadata when writing a binlog. We want binlogs to be fully roundtrippable and not lose any information. This is useful for unit-tests that compare against text logs and verify that played back logs are fully identical.

Add a new ILoggingService.LogCommentFromText overload that accepts a string and arguments. This allows us to cache resource strings instead of retrieving them every time.

Do not allocate or set Message when creating ProjectStarted, ProjectFinished, TargetStarted, TargetFinished, TaskStarted or TaskFinished.

Following the pattern set for TaskParameterEventArgs, inject the logic to format resource strings into BuildEventArgs using a static delegate. Later we can refactor and find a better pattern for this, but this works for now.

For TargetSkipped, log either OriginallySucceeded or Condition/EvaluatedCondition, and don't log the Message.

When logging input files and output files for skipped targets, do not concatenate them into huge semicolon-separated strings, but log a TaskParameterEventArgs instead. Introduce two new kinds of TaskParameter: TargetSkippedInputs and TargetSkippedOutputs. However as this is a flat list of files with no itemType, log itemType as null. Adjust the GetParameterText() logic to not write an extra indent or itemType if it is null.

Implement manual node packet translation for ProjectImported, TargetSkipped and Telemetry event args to avoid TranslateDotNet and binary formatter.
When logging input files and output files for skipped targets, do not concatenate them into huge semicolon-separated strings, but log a TaskParameterEventArgs instead. Introduce two new kinds of TaskParameter: TargetSkippedInputs and TargetSkippedOutputs. This way the smaller strings are deduplicated better.
For scalar task parameters, log a string with arguments instead of concatenating it into a single string such as "TaskParameter:a=b".
These concatenated strings are huge and are almost never useful.

Cache resource strings during evaluation instead of retrieving them every time.
Instead of retrieving resource strings every time, cache them, reduce allocations and concatenations. No need to use a resource string to just add 4/8/10/12 spaces.

Primarily this avoids concatenating the final string into a single string, but instead keeps the message separate from the arguments until the final string is requested.

Don't log the image runtime version if it's v4.0.30319 as it's the overwhelming default and carries no useful information.

Use better terminology for inclusion and exclusion lists.

This change looks extensive, but is pretty mechanical. Hard to review, but a significant improvement to allocations and RAR logging overhead.
Allows to turn off perf summary even in diagnostic mode.

Useful for diffing logs (as the perf numbers will be different when playing back a binlog).
Turns out we didn't have good coverage for Serial and Parallel console loggers. Include them in the binary logger roundtrip so we verify that logs from playing back a binlog are exactly identical to the logs from the real build.
Record a binlog every time we build using TestEnvironment, as well as parallel and serial console logs. After the build, playback the binlog into a new serial and console logs and diff the results.

This ensures that all events that happen during our test can be correctly played back from a binlog and result in the exact same output.

I leave a switch to turn it off for individual tests, but if these prove noisy we can turn this validation off entirely by default (and only turn it back on locally when working on logging related changes).

I did have to turn it off for TreatWarningsAsErrorsWhenBuildingSameProjectMultipleTimes because the binlog contained an extra property MSBuildLastTaskResult=true. I'm not exactly sure what is happening there, but one theory is the console log sees the state where that property is not set, then it's being mutated, then the binary log observes the state. Since two logs capture live data from ProjectEvaluationFinished, and access at different times, it may be that this is a case where the evaluation mutates in between. We need to be on the lookout for more cases like this and maybe turn this validation off by default. But it does give us a ton of free coverage for Serial and Parallel loggers. Most of the tests actually only exercise the MockLogger, so it's nice to test the real text loggers too.

I measured the overall duration of "build.cmd -test" and got inconclusive numbers. I think there's no obvious evidence that this change slows down the test run by any significant amount. The durations I got are:

07:34.6 07:38.8
06:45.4 06:52.5
08:09.7 07:28.4
06:49.7 07:32.5

and I'm intentionally not telling which one is which to illustrate that there is no obvious slowdown.
@KirillOsenkov
Copy link
Member Author

Fixes #6199

@danmoseley
Copy link
Member

cache string resources instead of retrieving them every time

This is not directly related to /bl right? Is it worth it? You are only saving a hashtable lookup.

@danmoseley
Copy link
Member

Some nice savings in bytes across the pipe, I assume this gives measurable improvements for /m /flp:d or similar.

@KirillOsenkov
Copy link
Member Author

@danmoseley I have to confess I didn't know how GetResourceString is implemented, so I just assumed there'll be some non-zero amount of overhead there. Now that you're mentioning it, you're right of course, it will likely be negligible even if called thousands of times. I just benchmarked it and even though it's about 5x overhead compared to a simple hashtable lookup, it's still nanoseconds:

https://github.com/KirillOsenkov/Benchmarks/blob/c77e8587477e38a9c7347efa6bb3d08b79f33a02/Benchmarks/Tests/ResourceManager.cs#L13-L15

I was also driven by the fact that messages logged by RAR are among the most numerous and expensive:
image

So it's likely not worth going after GetResourceString() just for the sake of it, but here it also helps with reducing duplication and preparing indented strings once instead of allocating every time. Allocations are most certainly a problem.

@KirillOsenkov
Copy link
Member Author

Oh, and file loggers are not even worth measuring right now because we know there are too many low hanging fruit there, primarily because of allocations. To give you a rough example:

Build Duration
Single-core no logging: 15 s
/bl with no TaskParameterEventArgs 15 s
/bl 18 s
/flp1:v=diag 59 s

There's just so much we can fix there. I hope I'll have time this year to do a pass like I did with /bl.

@KirillOsenkov
Copy link
Member Author

I just checked the ratio of binlog to diag text log and this is pretty neat:
image

@danmoseley
Copy link
Member

It would be great to improve /flp1:v=diag but nobody does that normally of course. v=d and v=n would likely be worth more attention..

@KirillOsenkov
Copy link
Member Author

Absolutely, when we look at text logging we'll look at those verbosities first of course.

Copy link
Member

@benvillalobos benvillalobos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Initial review, stopped on 95021b1. So far looks good!

@@ -1315,6 +1315,13 @@ private void LogPropertyReassignment(P predecessor, P property, string location)
string newValue = property.EvaluatedValue;
string oldValue = predecessor?.EvaluatedValue;

if (string.Equals(property.Name, "MSBuildAllProjects", StringComparison.OrdinalIgnoreCase))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like it removes the MSBuildALlProjects property altogether, is that the case?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't log any property reassignment messages for MSBuildAllProjects, yes.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just for clarity, MSBuildAllProjects isn't gone, and its final state is still logged; this only suppresses property reassignment.

return;
}

if (string.Equals(property.Name, "MSBuildAllProjects", StringComparison.OrdinalIgnoreCase))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While we're here, I took a look at a binlog to see what other properties stand out. DefineConstants gets about as long as MSBuildAllProjects and is reassigned quite a bit. Any way to log these once at the end of evaluation?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DefaultItemExcludes doesn't get nearly as long or reassigned as many times but it seems worth mentioning.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I've noticed DefaultItemExcludes too, but decided to not go too crazy. DefineConstants is certainly useful, so let's still log that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was already familiar with MSBuildAllProjects. How is DefineConstants used?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, is it that valuable to remove the initial value logging? That should only happen once, and it should be pretty small at that point, right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DefineConstants is very useful, this is that thing that is passed to Csc that tells the compiler what #ifdef values are considered true.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, not removing the initial logging, just the reassignment. There's a separate bug that the logging of initial values, environment variable reads is not working: #5015

RequiredBy = GetResourceFourSpaces("ResolveAssemblyReference.RequiredBy");
Resolved = GetResourceFourSpaces("ResolveAssemblyReference.Resolved");
ResolvedFrom = GetResourceFourSpaces("ResolveAssemblyReference.ResolvedFrom");
SearchedAssemblyFoldersEx = EightSpaces + GetResource("ResolveAssemblyReference.SearchedAssemblyFoldersEx");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: For consistency, GetResourceEightSpaces and so on. Or GetResourceXSpaces("Resource", SomeFancyEnumEnum.Four) (the former seems like the better option).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this better than the second option, but I like the first option more.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK by popular demand I've introduced GetResourceEightSpaces. It does make sense.
Also added a separate bool initialized field.

Strings.Initialize(Log);
}

private static class Strings
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In an earlier commit you had a string for Indent that was four or eight spaces. Does it make sense to create a more global strings class that has things like fourspaces, eightspaces, resource formatters, etc?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably not worth it honestly. But let's think about it some more.


internal static void Initialize(TaskLoggingHelper log)
{
if (Resolved != null)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Checking Resolved here seems a bit arbitrary. Some sort of HasBeenInitialized bool sounds more intuitive.

I was going to suggest checking if (this != null), but this is essentially a static singleton, so I see how this came about.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No big deal honestly. I agree WasInitialized is marginally better.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK I added a static bool initialized field.


Log.LogMessageFromResources(MessageImportance.Low, "ResolveAssemblyReference.LogTaskPropertyFormat", "TargetFrameworkMonikerDisplayName");
Log.LogMessageFromResources(MessageImportance.Low, "ResolveAssemblyReference.FourSpaceIndent", TargetFrameworkMonikerDisplayName);
Log.LogMessage(importance, property, "TargetFrameworkMoniker");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

General question: I tried chasing when the string eventually gets formatted and stopped my search at LoggingService.RouteBuildEvent, the args get passed into a buildeventsink.Consume. Where do they get picked up and eventually formatted?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When either SerialConsoleLogger or ParallelConsoleLogger actually wants to print it. If there are no text loggers, the string is never concatenated together. Well maybe in the binlog viewer after reading the binlog.

The beauty of LazyFormattedBuildEventArgs is that arguments travel separately from the string through the entire system. We haven't been leveraging this until now. Node packet translator already did the right thing, and now the binlog reader and writer do too.

Log.LogMessageFromResources(MessageImportance.Low, "ResolveAssemblyReference.FourSpaceIndent", Log.FormatResourceString("ResolveAssemblyReference.FormattedAssemblyInfo", redistInfo.Path));
}
}
Log.LogMessageFromResources(MessageImportance.Low, "ResolveAssemblyReference.TargetFrameworkSubsetLogHeader");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would we generally benefit from avoiding just-in-time resource retrieving? I seem to recall you saying it was as fast as a dictionary lookup.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resource lookup seems to be fast, but the downside of LogMessageFromResources is that it formats the string and inlines the arguments, which we want to avoid. This is why I had to introduce a Log.LogMessage overload that takes a string with arguments and preserves the arguments separately.

Copy link
Member

@Forgind Forgind left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Really appreciate your changes here.

@@ -820,8 +777,8 @@ public BuildEventContext LogTaskStarted2(BuildEventContext targetBuildEventConte
{
TaskStartedEventArgs buildEvent = new TaskStartedEventArgs
(
ResourceUtilities.FormatResourceStringIgnoreCodeAndKeyword("TaskStarted", taskName),
null, // no help keyword
message: null,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I asked this but got distracted—why do you have to pass null here as opposed to just not passing anything?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to call the proper base constructor I think

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is to call the right constructor overload, which calls the right base constructor.

// ParamName=
// a.txt
// b.txt
string indent = " ";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit:
string indent = parameterName == null ? ...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nah, that’s too smart

@@ -22,6 +22,8 @@ internal enum BuildEventArgsFieldFlags
LineNumber = 1 << 10,
ColumnNumber = 1 << 11,
EndLineNumber = 1 << 12,
EndColumnNumber = 1 << 13
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know it wasn't your typo, but 😒HelpHeyword

Is there some limit to this based on the maximum size of an int?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OUCH! That is totally my typo, from 2017. Well spotted, and how embarrassing. Can't believe I never noticed. Will fix.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And yes, we can only have 32. One tweak we can do when we run out is change the enum base type to long, this will buy us 32 more. But I don't think we'll be adding much more to this.

Comment on lines +416 to +424
case TaskParameterEventArgs taskParameter: Write(taskParameter); break;
case ProjectImportedEventArgs projectImported: Write(projectImported); break;
case TargetSkippedEventArgs targetSkipped: Write(targetSkipped); break;
case PropertyReassignmentEventArgs propertyReassignment: Write(propertyReassignment); break;
case TaskCommandLineEventArgs taskCommandLine: Write(taskCommandLine); break;
case UninitializedPropertyReadEventArgs uninitializedPropertyRead: Write(uninitializedPropertyRead); break;
case EnvironmentVariableReadEventArgs environmentVariableRead: Write(environmentVariableRead); break;
case PropertyInitialValueSetEventArgs propertyInitialValueSet: Write(propertyInitialValueSet); break;
case CriticalBuildMessageEventArgs criticalBuildMessage: Write(criticalBuildMessage); break;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

{
flags |= BuildEventArgsFieldFlags.ThreadId;
}
// ThreadId never seems to be used or useful for anything.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you just delete these sorts of changes instead of commenting them out? Otherwise they'll last forever.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, I want to keep them at least for a while as an indication of what was here in the old formats. We may decide to bring this back. I’ll revisit in a year or so.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please make sure you do. I don't like commented out code.

string GetResource(string name) => log.GetResourceMessage(name);
string GetResourceFourSpaces(string name) => FourSpaces + log.GetResourceMessage(name);

ConsideredAndRejectedBecauseFusionNamesDidntMatch = EightSpaces + GetResource("ResolveAssemblyReference.ConsideredAndRejectedBecauseFusionNamesDidntMatch");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious if we can just remove some of this logging or if customers actually want it. RAR logs a lot, and I have at least a vague understanding of what a lot of these are, but I've seen several cases in which people unfamiliar with RAR report an error and show me a portion of the logs that, to me, pretty clearly states what they should do to resolve the problem, but to them means nothing.

I'm particularly thinking of things like scatter files because I have no idea what they're for, and I asked rainersigwald at one point, and he had no idea what they were for. If no one knows what they're for, why are we logging about them? Are they even helpful, or can we remove them (and calculating them) entirely?

That would clearly make this a potential breaking change, so although I'd be in favor of getting rid of the less understandable log messages, it would have to be under a change wave at minimum and probably shouldn't happen at all. Even figuring out which are the unclear log messages would be difficult, and getting it wrong could really annoy people.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm particularly thinking of things like scatter files because I have no idea what they're for, and I asked rainersigwald at one point, and he had no idea what they were for. If no one knows what they're for, why are we logging about them? Are they even helpful, or can we remove them (and calculating them) entirely?

This sounds worth talking about when Rainer gets back. Maybe a changewave to log "less useful" messages at the lowest verbosity rather than not at all. Unless they're already logged at the lowest. Then I don't think we can avoid someone somewhere getting annoyed.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it's tempting to reduce RAR logging. RAR logging is the thing that bloats the binlog size the most. We should hopefully have an effort to increase more useful logging and reduce useless logging. I couldn't resist and removed the "Image runtime is v4.0.30319" in this PR which is absolutely useless ("gee whiz, this .dll is a managed assembly").

But removing more should be a separate effort.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general, we should fight "Captain Obvious logging" everywhere and ensure that the signal-to-noise ratio is high. NuGet RestoreTask is especially high noise unfortunately: NuGet/Home#10383

RequiredBy = GetResourceFourSpaces("ResolveAssemblyReference.RequiredBy");
Resolved = GetResourceFourSpaces("ResolveAssemblyReference.Resolved");
ResolvedFrom = GetResourceFourSpaces("ResolveAssemblyReference.ResolvedFrom");
SearchedAssemblyFoldersEx = EightSpaces + GetResource("ResolveAssemblyReference.SearchedAssemblyFoldersEx");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this better than the second option, but I like the first option more.

@@ -2774,7 +2890,7 @@ private bool ShouldUseSubsetBlackList()
}

// No subset names were passed in to search for in the targetframework directories and no installed subset tables were provided, we have nothing to use to
// generate the black list with, so do not continue.
// generate the exclusion list with, so do not continue.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would these sorts of changes wreak havoc with our other documentation?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO it's a good start

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we should make these changes everywhere, including the public API eventually. As a first pass I limited this to comments and internal stuff like parameter names and local names, but we'll need a separate pass with an API change when Rainer gets back.

@@ -975,6 +975,9 @@ internal virtual bool ApplyParameter(string parameterName, string parameterValue
case "PERFORMANCESUMMARY":
showPerfSummary = true;
return true;
case "NOPERFORMANCESUMMARY":
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@@ -198,6 +198,43 @@ public void TestDependencyBuildWithError()
Assert.Equal(TargetResultCode.Success, resultsCache.GetResultForRequest(entry.Request)["Baz"].ResultCode);
}

[Fact]
public void TestLoggingForSkippedTargetInputsAndOutputs()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really not already have a test for this? Seems so fundamental...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, surprisingly I didn't find anything.

Copy link
Member

@benvillalobos benvillalobos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, thanks for the continuous perf improvements!

This is safe because the enum is internal and the current name isn't used anywhere.
Also make a separate bool field initialized.
@Forgind Forgind added the merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now. label Apr 9, 2021
@@ -300,6 +300,17 @@ public void LogCommentFromText(BuildEventContext buildEventContext, MessageImpor
_writer(message);
}

/// <inheritdoc />
public void LogCommentFromText(BuildEventContext buildEventContext, MessageImportance importance, string message, params object[] messageArgs)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder what the gains of offering a few overloads for it would be:

public void LogCommentFromText(BuildEventContext buildEventContext, MessageImportance importance, string message, params object[] messageArgs)
public void LogCommentFromText(BuildEventContext buildEventContext, MessageImportance importance, string message, object arg1)
public void LogCommentFromText(BuildEventContext buildEventContext, MessageImportance importance, string message, object arg1, object arg2)

That would avoid most of the allocations for the string.Format calls, I guess.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We’ll need an array later on anyway, to store the arguments on LazyFormattedBuildEventArgs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Logging merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now. performance Performance-Scenario-Build This issue affects build performance. Task: Resolve Assembly References (RAR)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants