perf(logs): avoid string allocation when no parameters are passed#4697
perf(logs): avoid string allocation when no parameters are passed#4697
Conversation
…be formatted via the template
Instructions and example for changelogPlease add an entry to Example: ## Unreleased
### Performance
- avoid string allocation when no parameters are passed ([#4697](https://github.com/getsentry/sentry-dotnet/pull/4697))If none of the above apply, you can opt out of this check by adding |
Semver Impact of This PR⚪ None (no version bump detected) 📋 Changelog PreviewThis is how your changes will appear in the changelog.
🤖 This preview updates automatically when you update the PR. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4697 +/- ##
==========================================
- Coverage 74.12% 74.01% -0.12%
==========================================
Files 499 499
Lines 18067 18071 +4
Branches 3520 3518 -2
==========================================
- Hits 13392 13375 -17
- Misses 3813 3838 +25
+ Partials 862 858 -4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| | Method | Mean | Error | StdDev | Gen0 | Gen1 | Allocated | | ||
| |--------------------- |---------:|--------:|--------:|-------:|-------:|----------:| | ||
| | LogWithoutParameters | 102.3 ns | 1.28 ns | 1.19 ns | 0.0640 | 0.0001 | 536 B | | ||
| | LogWithParameters | 248.5 ns | 4.86 ns | 5.40 ns | 0.1087 | - | 912 B | |
There was a problem hiding this comment.
note: perf comparison
Before this change:
| Method | Mean | Error | StdDev | Median | Gen0 | Allocated |
|---|---|---|---|---|---|---|
| LogWithoutParameters | 122.2 ns | 2.31 ns | 5.87 ns | 120.5 ns | 0.0696 | 584 B |
| LogWithParameters | 242.6 ns | 0.79 ns | 0.74 ns | 242.5 ns | 0.1087 | 912 B |
After this change:
| Method | Mean | Error | StdDev | Gen0 | Gen1 | Allocated |
|---|---|---|---|---|---|---|
| LogWithoutParameters | 102.3 ns | 1.28 ns | 1.19 ns | 0.0640 | 0.0001 | 536 B |
| LogWithParameters | 248.5 ns | 4.86 ns | 5.40 ns | 0.1087 | - | 912 B |
Result:
- A tiny pessimization in time (IMO neglectable) with params/args
- one more jump and assignment
- A notable optimization (a bit of time and 48 bytes in space) without params/args
- no invocation of
System.String.Format
- no invocation of
| message = template; | ||
| template = null!; | ||
| @params = ImmutableArray<KeyValuePair<string, object>>.Empty; |
There was a problem hiding this comment.
question: Breaking Change?
This improvement is - technically speaking - also a behavioral (breaking?) change, when Logging a plain message without positional arguments, observable by user code through the SetBeforeSendLog callback:
options.SetBeforeSendLog(static (SentryLog log) =>
{
_ = (log.Message, log.Template, log.Parameters);
return log;
});No changes to the resulting serialized payload, though - we have a check in place that does not serialize a sentry.message.template when there is no sentry.message.parameter ... but there are user-code observable changes to the SentryLog.Template and SentryLog.Parameters properties.
when Logging without positional Parameters: e.g. SentrySdk.Logger.LogError("Without parameters");
- Before
log.Template == log.Messagelog.Parameters.IsDefault == true
- After
log.Template == nulllog.Parameters.IsDefault == false
- Justification
SentryLog.Templatewas already declared nullable (string?)- no contractual change
- and this now also mirrors what is actually serialized for the payload
- the
ImmutableArray`1ofParametersmay no longer bedefault- when an
ImmutableArray`1isdefault, invoking any instance member other thanIsDefaultorIsDefaultOrEmptywill throw aNullReferenceException, because the underlying Array isnull - this makes user-callbacks "safer"
- but also the down-stream
sentry-unitySDK
- when an
Another benefit of ImmutableArray<KeyValuePair<string, object>> Parameters no longer being default when logging without positional arguments is that we can omit IsDefault and IsDefaultOrEmpty checks in the ISentryJsonSerializable.WriteTo serialization logic, which makes it slightly faster, too.
when Logging with positional Parameters: e.g. SentrySdk.Logger.LogError("With parameters {0} {1} {2}", 1, 2, 3);
- no observable changes
See also https://develop.sentry.dev/sdk/telemetry/logs/#default-attributes.
There was a problem hiding this comment.
and a follow-up question:
Shall we note this in the CHANGELOG?
| // ensure the ImmutableArray`1 is not default, so we can omit IsDefault checks before accessing other members | ||
| Parameters = ImmutableArray<KeyValuePair<string, object>>.Empty; |
There was a problem hiding this comment.
note: this ensures that the ImmutableArray`1 of Parameters is never default
In sentry-dotnet, there is currently no code path that would require this, as we always set the instance to at least "Empty".
But in the down-stream sentry-unity, there actually is a code path where it may be default.
- see https://github.com/getsentry/sentry-unity/blob/e2f2db400d776b26108759688003b8a120094538/src/Sentry.Unity/Integrations/UnityApplicationLoggingIntegration.cs#L158-L159
- this is also the main scenario this optimization is for
- as in
sentry-dotnet, we assume most usages viaILoggerand other Logging-Integrations
With this assignment, we ensure that it's never default, and both us and users can omit IsDefault/IsDefaultOrEmpty checks.
|
|
||
| internal void WriteTo(Utf8JsonWriter writer, IDiagnosticLogger? logger) | ||
| { | ||
| Debug.Assert(!Parameters.IsDefault); |
There was a problem hiding this comment.
note: omit default checks
Since we ensured in the internal .ctor, as well as the init-only property, that the ImmutableArray is never default, and with that the underlying array never null, we can omit further IsDefault and IsDefaultOrEmpty checks.
| [Fact] | ||
| public void Create_Default_HasMinimalSpecification() |
There was a problem hiding this comment.
note: I wanted to keep the tests for both Logs and Metrics in sync
Closes #5118.
Avoid a string allocation when there are no parameters that the template string should be formatted with.
Also, ensure that the
Parametersof typeImmutableArray`1are never thedefault-struct, so that we can avoidImmutableArray.IsDefaultchecks afterwards.@bitsandfoxes, we talked about this optimization quite a while back. It's mainly for this code path: