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.

Sign up for GitHub

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

Jump to bottom

Replace consts with static readonly #428

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Replace consts with static readonly #428

wants to merge 4 commits into from

Conversation

AndreyTretyak
Copy link
Contributor

Using const results in a new allocation each time it's used. The most common exception would be health check markers we have that used each time we mark activity as healthcheck or check it.

This PR replaces const with static readonly to avoid additional allocations.

@AndreyTretyak AndreyTretyak requested a review from a team as a code owner November 8, 2021 13:53
syphe
syphe approved these changes Nov 8, 2021
View reviewed changes
Copy link
Contributor

@syphe syphe left a comment

Choose a reason for hiding this comment

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

Another reason this is a good idea, is that const is determined at compile time when being referenced elsewhere, where as 'static readonly' will cause the value to be referenced at runtime instead.

@artempushkin
Copy link
Contributor

Let's look closer at those extra allocations, I don't see where they would be coming from.

K-Cully
K-Cully reviewed Nov 9, 2021
View reviewed changes
src/Extensions/Activities/ServiceCollectionExtensions.cs
@@ -18,8 +15,8 @@ namespace Microsoft.Extensions.DependencyInjection
/// </summary>
public static class ServiceCollectionExtensions
{
private const string ActivitySourceName = "OmexActivitySource";
private const string ActivitySourceVersion = "1.0.0.0";
private static readonly string s_activitySourceName = "OmexActivitySource";
Copy link
Member

@K-Cully K-Cully Nov 9, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

This class is already static so I expect these shouldn't be changed.

K-Cully
K-Cully reviewed Nov 9, 2021
View reviewed changes
src/Extensions/Logging/Internal/Replayable/ReplayableActivityExtensions.cs
@@ -11,10 +11,10 @@ namespace Microsoft.Omex.Extensions.Logging.Internal.Replayable
{
internal static class ReplayebleActivityExtensions
{
private const string ReplayableLogKey = "ReplayableLogKey";
private static readonly string s_replayableLogKey = "ReplayableLogKey";
Copy link
Member

Choose a reason for hiding this comment

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

class is already static

src/Extensions/Services.Remoting/Internal/Diagnostics.cs
@@ -14,15 +14,15 @@ internal static class Diagnostics
/// <remarks>
/// Changing this name will mean that consumers might miss telemetry events
/// </remarks>
internal const string DiagnosticListenerName = "Microsoft.Omex.Extensions.Services.Remoting";
internal static readonly string DiagnosticListenerName = "Microsoft.Omex.Extensions.Services.Remoting";
Copy link
Member

Choose a reason for hiding this comment

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

class is already static

src/Extensions/Services.Remoting/OmexRemotingHeadersExtensions.cs
@@ -29,7 +29,7 @@ public static class OmexRemotingHeadersExtensions
/// W3C standard for trace context parent 'traceparent', so we've added omex to avoid conflicts if support would be added in future
/// link: https://www.w3.org/TR/trace-context/
/// </remarks>
private const string TraceParentHeaderName = "omex-traceparent";
private static readonly string s_traceParentHeaderName = "omex-traceparent";
Copy link
Member

Choose a reason for hiding this comment

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

class is static

@K-Cully
Copy link
Member

K-Cully commented Nov 9, 2021

Let's look closer at those extra allocations, I don't see where they would be coming from.

I take it that the issue is that as transient instances get created and destroyed as part of their DI lifecycle, the const values are being repeatedly allocated.
However, this change appears to be over scoped, since that would not affect static classes.

@artempushkin
Copy link
Contributor

Let's look closer at those extra allocations, I don't see where they would be coming from.

I take it that the issue is that as transient instances get created and destroyed as part of their DI lifecycle, the const values are being repeatedly allocated. However, this change appears to be over scoped, since that would not affect static classes.

They shouldn't be, CLI ensures that each string constant exists as a singular object (though you can trick it if you try hard). I was talking to Andrey separately, profiler detects multiple allocations of some strings but I'm sure this PR won't fix it.

artempushkin
artempushkin requested changes Nov 19, 2021
View reviewed changes
Copy link
Contributor

@artempushkin artempushkin left a comment

Choose a reason for hiding this comment

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

I'll block this to prevent accidental merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@K-Cully K-Cully K-Cully left review comments

@artempushkin artempushkin artempushkin requested changes

@syphe syphe syphe approved these changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@AndreyTretyak @artempushkin @K-Cully @syphe @neilr81