Move the logging infrastructure from ReactiveUI into Splat #33
Conversation
| void Debug(IFormatProvider formatProvider, [Localizable(false)] string message, params object[] args); | ||
| void Debug([Localizable(false)] string message); | ||
| void Debug([Localizable(false)] string message, params object[] args); | ||
| void Debug<TArgument>(IFormatProvider formatProvider, [Localizable(false)] string message, TArgument argument); |
There was a problem hiding this comment.
Is there anything in particular we gain from using all these generics? I thought params object[] args combined with the right string.Format would be Good Enough...
There was a problem hiding this comment.
With using all the generics, you avoid boxing value types, which normally I wouldn't care about but for logging it can actually make a difference (i.e. a tight loop could end up spewing thousands of boxes)
There was a problem hiding this comment.
Won't these be boxed anyway due to the internal use of String.Format(IFormatProvider, String, Object[])?
There was a problem hiding this comment.
@dahlbyk Probably. 👨 🔫
There was a problem hiding this comment.
👨
It's so cute that they made an emoji of you.
You're absolutely right that avoiding boxing for logging here would be worth it...any clever ideas short of a custom implementation of String.Format()?
There was a problem hiding this comment.
My first thoughts went to using StringBuilder.AppendFormat (which it turns out String.Format uses internally) and I'm not seeing any box/unbox IL instructions there. Will do some more digging but perhaps something's changed there (I'm looking at .NET 4.5.1)...
There was a problem hiding this comment.
But you have to box to use AppendFormat, bah humbug...
| { | ||
| public void Write(string message, LogLevel logLevel) | ||
| { | ||
| if ((int)logLevel < (int)Level) return; |
There was a problem hiding this comment.
To avoid building log messages that are never used, would it be worth making WrappingFullLogger even more dull by inserting this check before calling _inner.Write(...)?
|
@paulcbetts any timeline on this? I am contemplating extending it :) |
|
@SimonCropp Quite soon :) |
|
So, let's punt on the boxing perf for now - since the Interface is still correct, we can always fix the boxing later. |
Move the logging infrastructure from ReactiveUI into Splat
Similar to #32, this PR moves the other super common reason to pull RxUI.Core into an unrelated library, the logging infrastructure.
I'd love to come up with something everyone can agree with so that we can share this code instead of all inflicting our own versions on people - at least the interfaces?
/cc @slodge @lbugnion @nigel-sampson