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

Optimize AddPropertyIfAbsent #1872

Merged
merged 6 commits into from May 3, 2023
Merged

Optimize AddPropertyIfAbsent #1872

merged 6 commits into from May 3, 2023

Conversation

sungam3r
Copy link
Contributor

@sungam3r sungam3r commented Mar 4, 2023

No actual API changes, only a few fixes for NRT annotations.
Eliminates allocation of LogEventProperty wrapper for LogEventPropertyValue.

dotnet test -c Release -f net7.0 --filter "FullyQualifiedName=Serilog.PerformanceTests.Harness.LogContextEnrichment"

BenchmarkDotNet=v0.13.5, OS=Windows 11 (10.0.22621.1265/22H2/2022Update/SunValley2)
AMD Ryzen 7 6800H with Radeon Graphics, 1 CPU, 16 logical and 8 physical cores
.NET SDK=7.0.201
  [Host]     : .NET 7.0.3 (7.0.323.6910), X64 RyuJIT AVX2
  DefaultJob : .NET 7.0.3 (7.0.323.6910), X64 RyuJIT AVX2

before

Method Mean Error StdDev Ratio RatioSD Gen0 Allocated Alloc Ratio
Bare 6.864 ns 0.1439 ns 0.1346 ns 1.00 0.00 - - NA
PushProperty 61.231 ns 1.2249 ns 1.3107 ns 8.94 0.28 0.0296 248 B NA
PushPropertyNested 118.867 ns 2.4084 ns 2.6769 ns 17.34 0.62 0.0591 496 B NA
PushPropertyEnriched 106.414 ns 1.7542 ns 1.6409 ns 15.51 0.36 0.0362 304 B NA

after

Method Mean Error StdDev Ratio RatioSD Gen0 Allocated Alloc Ratio
Bare 6.810 ns 0.0722 ns 0.0603 ns 1.00 0.00 - - NA
PushProperty 62.395 ns 1.2570 ns 2.5393 ns 9.52 0.26 0.0296 248 B NA
PushPropertyNested 128.455 ns 2.5705 ns 4.6351 ns 19.13 0.87 0.0591 496 B NA
PushPropertyEnriched 93.311 ns 1.8405 ns 2.1909 ns 13.74 0.41 0.0296 248 B NA

See Allocated column - 18% less heap allocation for case when event is logged with some pushed property.

@@ -14,7 +14,7 @@

namespace Serilog.Capturing;

class MessageTemplateProcessor : ILogEventPropertyFactory
class MessageTemplateProcessor : ILogEventPropertyFactory, ILogEventPropertyValueFactory
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this interface was just missed.

@@ -192,3 +194,18 @@ internal LogEvent Copy()
properties);
}
}

internal static class LogEventExtensions
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nblumhardt For now it's internal but it may be changed to public for use in dependent packages. For example, in serilog-extensions-logging, see EnrichAndCreateScopeItem:

var property = propertyFactory.CreateProperty(key, value, destructureObject);
logEvent.AddPropertyIfAbsent(property);


internal static class LogEventExtensions
{
public static void AddPropertyIfAbsent(this LogEvent evt, ILogEventPropertyFactory factory, string name, object? value, bool destructureObjects = false)
Copy link
Member

Choose a reason for hiding this comment

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

Since we already end up adding an internal property to LogEvent, we could just skip that and make this overload of AddPropertyIfAbsent an internal method on LogEvent itself, for now?

I can see how dependent libraries would benefit from making this accessible, but in that case we'd have to expose something on LogEvent anyway, so we might as well keep everything tight/encapsulated for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@nblumhardt nblumhardt merged commit 5291b4b into serilog:dev May 3, 2023
1 check passed
@sungam3r sungam3r deleted the factory branch May 3, 2023 05:22
@nblumhardt nblumhardt mentioned this pull request Jun 19, 2023
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.

None yet

2 participants