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
KirillOsenkov added 11 commits Apr 5, 2021
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

@KirillOsenkov KirillOsenkov commented Apr 5, 2021

Fixes #6199

@danmoseley
Copy link
Member

@danmoseley danmoseley commented Apr 5, 2021

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

@danmoseley danmoseley commented Apr 5, 2021

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

@KirillOsenkov KirillOsenkov commented Apr 5, 2021

@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

@KirillOsenkov KirillOsenkov commented Apr 5, 2021

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

@KirillOsenkov KirillOsenkov commented Apr 5, 2021

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

@danmoseley
Copy link
Member

@danmoseley danmoseley commented Apr 5, 2021

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

@KirillOsenkov KirillOsenkov commented Apr 5, 2021

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

Copy link
Member

@BenVillalobos BenVillalobos left a comment

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))

This comment has been minimized.

@BenVillalobos

BenVillalobos Apr 7, 2021
Member

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

This comment has been minimized.

@KirillOsenkov

KirillOsenkov Apr 8, 2021
Author Member

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

This comment has been minimized.

@Forgind

Forgind Apr 8, 2021
Member

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))

This comment has been minimized.

@BenVillalobos

BenVillalobos Apr 7, 2021
Member

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?

This comment has been minimized.

@BenVillalobos

BenVillalobos Apr 7, 2021
Member

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

This comment has been minimized.

@KirillOsenkov

KirillOsenkov Apr 8, 2021
Author Member

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

This comment has been minimized.

@Forgind

Forgind Apr 8, 2021
Member

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

This comment has been minimized.

@Forgind

Forgind Apr 8, 2021
Member

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?

This comment has been minimized.

@KirillOsenkov

KirillOsenkov Apr 8, 2021
Author Member

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

This comment has been minimized.

@KirillOsenkov

KirillOsenkov Apr 8, 2021
Author Member

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");

This comment has been minimized.

@BenVillalobos

BenVillalobos Apr 7, 2021
Member

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

This comment has been minimized.

@Forgind

Forgind Apr 8, 2021
Member

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

This comment has been minimized.

@KirillOsenkov

KirillOsenkov Apr 8, 2021
Author Member

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

This comment has been minimized.

@BenVillalobos

BenVillalobos Apr 7, 2021
Member

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?

This comment has been minimized.

@KirillOsenkov

KirillOsenkov Apr 8, 2021
Author Member

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


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

This comment has been minimized.

@BenVillalobos

BenVillalobos Apr 7, 2021
Member

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.

This comment has been minimized.

@KirillOsenkov

KirillOsenkov Apr 8, 2021
Author Member

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

This comment has been minimized.

@KirillOsenkov

KirillOsenkov Apr 8, 2021
Author Member

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");

This comment has been minimized.

@BenVillalobos

BenVillalobos Apr 7, 2021
Member

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?

This comment has been minimized.

@KirillOsenkov

KirillOsenkov Apr 8, 2021
Author Member

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");

This comment has been minimized.

@BenVillalobos

BenVillalobos Apr 7, 2021
Member

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.

This comment has been minimized.

@KirillOsenkov

KirillOsenkov Apr 8, 2021
Author Member

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.

@Forgind
Forgind approved these changes Apr 8, 2021
Copy link
Member

@Forgind Forgind left a comment

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,

This comment has been minimized.

@Forgind

Forgind Apr 8, 2021
Member

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

This comment has been minimized.

@KirillOsenkov

KirillOsenkov Apr 8, 2021
Author Member

to call the proper base constructor I think

This comment has been minimized.

@KirillOsenkov

KirillOsenkov Apr 8, 2021
Author Member

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

// ParamName=
// a.txt
// b.txt
string indent = " ";

This comment has been minimized.

@Forgind

Forgind Apr 8, 2021
Member

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

This comment has been minimized.

@KirillOsenkov

KirillOsenkov Apr 8, 2021
Author Member

nah, that’s too smart

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

This comment has been minimized.

@Forgind

Forgind Apr 8, 2021
Member

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

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

This comment has been minimized.

@KirillOsenkov

KirillOsenkov Apr 8, 2021
Author Member

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

This comment has been minimized.

@KirillOsenkov

KirillOsenkov Apr 8, 2021
Author Member

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.

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;
Comment on lines +416 to +424

This comment has been minimized.

@Forgind

Forgind Apr 8, 2021
Member

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

This comment has been minimized.

@Forgind

Forgind Apr 8, 2021
Member

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

This comment has been minimized.

@KirillOsenkov

KirillOsenkov Apr 8, 2021
Author Member

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.

This comment has been minimized.

@Forgind

Forgind Apr 9, 2021
Member

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");

This comment has been minimized.

@Forgind

Forgind Apr 8, 2021
Member

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.

This comment has been minimized.

@BenVillalobos

BenVillalobos Apr 8, 2021
Member

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.

This comment has been minimized.

@KirillOsenkov

KirillOsenkov Apr 8, 2021
Author Member

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.

This comment has been minimized.

@KirillOsenkov

KirillOsenkov Apr 8, 2021
Author Member

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");

This comment has been minimized.

@Forgind

Forgind Apr 8, 2021
Member

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.

This comment has been minimized.

@Forgind

Forgind Apr 8, 2021
Member

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

This comment has been minimized.

@BenVillalobos

BenVillalobos Apr 8, 2021
Member

IMO it's a good start

This comment has been minimized.

@KirillOsenkov

KirillOsenkov Apr 8, 2021
Author Member

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":

This comment has been minimized.

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

[Fact]
public void TestLoggingForSkippedTargetInputsAndOutputs()

This comment has been minimized.

@Forgind

Forgind Apr 8, 2021
Member

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

This comment has been minimized.

@KirillOsenkov

KirillOsenkov Apr 8, 2021
Author Member

Yes, surprisingly I didn't find anything.

Copy link
Member

@BenVillalobos BenVillalobos left a comment

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.
@cdmihai
cdmihai approved these changes Apr 8, 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)

This comment has been minimized.

@Therzok

Therzok Apr 14, 2021
Contributor

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.

This comment has been minimized.

@KirillOsenkov

KirillOsenkov Apr 14, 2021
Author Member

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

@BenVillalobos BenVillalobos merged commit 4920537 into main Apr 20, 2021
7 checks passed
7 checks passed
license/cla All CLA requirements met.
Details
@azure-pipelines
msbuild-pr Build #20210408.7 succeeded
Details
@azure-pipelines
msbuild-pr (Linux Core) Linux Core succeeded
Details
@azure-pipelines
msbuild-pr (Windows Core) Windows Core succeeded
Details
@azure-pipelines
msbuild-pr (Windows Full Release (no bootstrap)) Windows Full Release (no bootstrap) succeeded
Details
@azure-pipelines
msbuild-pr (Windows Full) Windows Full succeeded
Details
@azure-pipelines
msbuild-pr (macOS Core) macOS Core succeeded
Details
@BenVillalobos BenVillalobos deleted the dev/kirillo/logless branch Apr 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants