Skip to content

Conversation

@CyrusNajmabadi
Copy link
Member

No description provided.

@CyrusNajmabadi CyrusNajmabadi requested a review from a team as a code owner April 12, 2025 04:58
@CyrusNajmabadi CyrusNajmabadi requested a review from ToddGrun April 12, 2025 04:58
@ghost ghost added Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead labels Apr 12, 2025
@CyrusNajmabadi
Copy link
Member Author

@ToddGrun @JoeRobich ptal.

@CyrusNajmabadi
Copy link
Member Author

@JoeRobich @dibarbet @ToddGrun @tmat ptal.

@CyrusNajmabadi CyrusNajmabadi requested review from dibarbet and tmat April 12, 2025 17:59
if (_lazyMap == null)
{
_lazyMap = SharedPools.Default<Dictionary<string, object?>>().AllocateAndClear();
_propertySetter.Invoke(_lazyMap, _args);
Copy link
Contributor

Choose a reason for hiding this comment

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

.Invoke(

since we don't check for null, no real need to use invoke

public static GenericKeyValueLogMessage<TArgs> Create(
Action<Dictionary<string, object?>, TArgs> propertySetter, TArgs args, LogLevel logLevel)
{
var logMessage = s_pool.Allocate();
Copy link
Contributor

Choose a reason for hiding this comment

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

var logMessage = s_pool.Allocate();

nit: could just call Create(LogType.Trace, propertySetter, args, logLevel)

LogLevel = logLevel;
}

public static new SimpleKeyValueLogMessage Create(Action<Dictionary<string, object?>> propertySetter, LogLevel logLevel = LogLevel.Information)
Copy link
Contributor

Choose a reason for hiding this comment

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

Create

could just call the other Create overload

{
private static readonly ObjectPool<SimpleKeyValueLogMessage> s_pool = new(() => new());

private Action<Dictionary<string, object?>>? _propertySetter;
Copy link
Contributor

Choose a reason for hiding this comment

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

_propertySetter

Feels like we should be consistent between this member in SimpleKeyValueLogMessage and GenericKeyValueLogMessage

@ToddGrun
Copy link
Contributor

Love it! I notice these allocations when I'm looking at the code, but it never seems to pop in a profile. Death by a thousand papercuts, I suppose.


In reply to: 2798487395

@CyrusNajmabadi
Copy link
Member Author

@ToddGrun ok with signing off? I can handle the mop up in a new pr.

Copy link
Contributor

@ToddGrun ToddGrun left a comment

Choose a reason for hiding this comment

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

:shipit:

@CyrusNajmabadi CyrusNajmabadi merged commit e2700e8 into dotnet:main Apr 13, 2025
25 checks passed
@CyrusNajmabadi CyrusNajmabadi deleted the genericKeyValueMessage branch April 13, 2025 19:21
@dotnet-policy-service dotnet-policy-service bot added this to the Next milestone Apr 13, 2025
@RikkiGibson RikkiGibson modified the milestones: Next, 18.0 P1 Aug 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead VSCode

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants