Trivial fixes #185
Trivial fixes #185
Conversation
northtyphoon
commented
May 9, 2017
northtyphoon
commented
- Make TraceLoggingEventId to implement IDisposable.
- Optimize some codes in TraceLoggingEventId for perf.
- Fix typos.
1. Make TraceLoggingEventId to implement IDisposable. 2. Optimize some codes in TraceLoggingEventId for perf. 3. Fix typos.
| { | ||
| /// <summary> | ||
| /// Checks to see if eventRecord has TraceLogging meta data associated with it (EVENT_HEADER_EXT_TYPE_EVENT_SCHEMA_TL) | ||
| /// and if so updates EventHeader.Id to be an event ID unique to that provider/opcode/meta-data blob. | ||
| /// </summary> | ||
| [MethodImpl(MethodImplOptions.AggressiveInlining)] |
There was a problem hiding this comment.
❓ Were you encountering situations where this method was not already getting inlined? I would prefer to omit this change in light of requests like #153 unless there is a demonstrated problem with the current approach.
There was a problem hiding this comment.
I didn't try to check if the method is inlined or not in runtime. Since I see @vancem has changed TraceEvent lib to target 4.5 and therefore I made the change.
For 3.5 support, do we consider to create a release branch and only do bug fix as needed?
There was a problem hiding this comment.
@northtyphoon Currently the library does not support .NET 3.5, and there is no concrete plan to do so. My suggestion here is to only include this change if it is needed, e.g. if:
- PerfView revealed this method in traces (a clear indication that not only is it not being inlined, but it's heavily used)
- You saw this method in disassembled JIT code and observed that it wasn't inlined, and knew separately that it's heavily used so it should be
- Some other indicator suggesting that the attribute provides value that a user cares about
For me, the bar here is low. I just don't want to include it if no one will notice that it was included, because it makes things a tiny bit more complicated if we ever decide to go back and address the specific request I linked to.
| @@ -10,19 +11,20 @@ namespace Microsoft.Diagnostics.Tracing | |||
| /// <summary> | |||
| /// TraceLoggingEvnetId is a class that manages assigning event IDs (small 64k numbers) | |||
| /// to TraceLogging Style events (which don't have them). Because TraceEvent uses EventIDs | |||
| /// so fundamentally this difficiency is very problematic. | |||
| /// so fundamentally this deficiency is very problematic. | |||
There was a problem hiding this comment.
💡 In the future, it helps to place all formatting-only changes and spelling corrections in their own commit, and preferably in their own independent pull request. The primary advantages this offers are:
- It's easier to expedite spelling corrections so they don't get tied up in code review
- It makes the code review process less complex for the actual code changes (for example, this pull request would contain a one-line change in one file, and nothing else)
The scope in this pull request is fairly small so I don't see it causing a problem, but it adds up pretty quickly so it's a good thing to keep in mind for the future. 👍
There was a problem hiding this comment.
I agree. As you said, the scope of this PR is very small, so I combined the changes. I will follow the guidance in the future.
Can you also add the guidance to https://github.com/Microsoft/perfview/blob/master/CONTRIBUTING.md
There was a problem hiding this comment.
@northtyphoon It seems to be covered well enough already. I mostly assume that people don't read that, or do a cursory look at best. I don't let it bother me when deviations happen and I'm always willing to help if someone isn't sure how to break things up. 😄
| foreach (var key in m_traceLoggingEventMap.Keys) | ||
| Marshal.FreeHGlobal((IntPtr)key.Provider); | ||
| foreach (var kvp in m_traceLoggingEventMap) | ||
| Marshal.FreeHGlobal((IntPtr)kvp.Key.Provider); |
There was a problem hiding this comment.
📝 For those unaware, this change eliminates an unnecessary allocation during the Dispose call. One of the things I'm enjoying about C# 7 (which we aren't currently using on this project) is the ability to gain the efficiency of this form without losing the clarity of the original code by writing this:
foreach (var (key, _) in m_traceLoggingEventMap)
Marshal.FreeHGlobal((IntPtr)key.Provider);This form applies a deconstruction to the KeyValuePair<TKey, TValue> in the declaration of key and an unused placeholder _.
There was a problem hiding this comment.
Thanks for sharing the C# 7 tips.
There was a problem hiding this comment.
@sharwell Pedantic: You can't deconstruct a KeyValuePair like that. You'll need a custom Deconstruct method.
There was a problem hiding this comment.
@pharring It depends on where things are running. It's already added to coreclr in dotnet/coreclr#8560 (not sure what release version that corresponds to). For Roslyn I added the Deconstruct extension method when I used the feature in dotnet/roslyn#19126.
|
@northtyphoon Just making sure you saw my question on the use of |
…tential issue to support 3.5
|
LGTM |
Trivial fixes
