Skip to content

Conversation

@snakefoot
Copy link

@snakefoot snakefoot commented Dec 7, 2025

Removed unnecessary casting to array, since it will automatically match IReadOnlyList<object>.

Curious why you also convert into FrozenDictionary (another allocation that hurt performance). Since a LogEvent is a very temporary object, that is thrown away. FrozenDictionary are "expensive" to create, because you plan to hold on to them.

#if NET8_0_OR_GREATER
// Use FrozenDictionary for optimal read performance on .NET 8+
return System.Collections.Frozen.FrozenDictionary.ToFrozenDictionary(dict);
#else
return dict;
#endif

I find it strange that when calling Akka's LogMessage.ToString() and using semantic logging, then Akka-Formatter explodes in your face (Because string.Format only handles positional parameters).

Also strange that Akka LogMessage only supports message-template-properties. I find message-template properties very organic and error-prone (case-sensitive, spelling, etc.) compared to explicit "building" the LogEvent.

@Aaronontheweb
Copy link
Member

Curious why you also convert into FrozenDictionary (another allocation that hurt performance). Since a LogEvent is a very temporary object, that is thrown away. FrozenDictionary are "expensive" to create, because you plan to hold on to them.

We can totally get rid of that, especially because right now we don't even target .NET 8 so that compiler directive never gets hit. We're probably going to go straight to .NET 10 for v1.6.

I find it strange that when calling Akka's LogMessage.ToString() and using semantic logging, then Akka-Formatter explodes in your face (Because string.Format only handles positional parameters).

That's probably a bug that our test suite doesn't hit because rendering isn't done via that method, but rather through the template extraction system.

@Aaronontheweb
Copy link
Member

Appreciate the PR @snakefoot - happy to accept more for some of the other suggestions you have RE: the semantic logger. That's new code so there's probably room for improvement in there.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants