Cleanup more EventSource calls in SPC to work better with illinker #40539
Conversation
marek-safar
commented
Aug 7, 2020
marek-safar
commented
- Adds missing IsEnabled guard
- Tweak the logic to be detectable by linker
- Adds missing IsEnabled guard - Tweak the logic to be detectable by linker
|
I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label. |
| @@ -3253,6 +3249,8 @@ internal void FinishContinuations() | |||
| lock (continuations) { } | |||
| int continuationCount = continuations.Count; | |||
|
|
|||
| bool etwIsEnabled = log.IsEnabled(); | |||
There was a problem hiding this comment.
Are we concerned that IsEnabled() may have changed in-between the above check and now? I assume that is why the existing code is written the way it was.
cc @stephentoub
There was a problem hiding this comment.
I was actually looking at the existing cases and we are pretty inconsistent on this
There was a problem hiding this comment.
Why not store this Boolean at the top of the method, where it was storing log in a variable?
There was a problem hiding this comment.
I assume that is why the existing code is written the way it was.
Yes. The goal was to avoid output the subsequent events if and only if the starting event was written.
| @@ -311,7 +311,7 @@ internal override void Run(Task completedTask, bool canInlineContinuationTask) | |||
| // If the task was cancel before running (e.g a ContinueWhenAll with a cancelled caancelation token) | |||
| // we will still flow it to ScheduleAndStart() were it will check the status before running | |||
| // We check here to avoid faulty logs that contain a join event to an operation that was already set as completed. | |||
| if (!continuationTask.IsCanceled && TplEventSource.Log.IsEnabled()) | |||
| if (TplEventSource.Log.IsEnabled() && !continuationTask.IsCanceled) | |||
There was a problem hiding this comment.
Does the linker not handle something && false should just be false?
There was a problem hiding this comment.
It does, it just didn't look right to me to do the call and then scrap it.
There was a problem hiding this comment.
I don’t think I understand, to do what call? This was checking a property.
There was a problem hiding this comment.
Yep but why to check the property if it's needed only when tracing is enabled. I don't feel strongly about this and could revert to the original logic event though continuationTask.IsCanceled is probably most of time false
|
How much savings are you seeing with this change? I didn't dig deep into this area originally, because it looked like the savings were not significant vs the risk for potential behavior change. |
|
Not a lot (few kB) but that's because there are still few cases which bring EventSource |
src/libraries/System.Private.CoreLib/src/System/Threading/ThreadPool.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/Threading/ThreadPool.cs
Outdated
Show resolved
Hide resolved
…adPool.cs Co-authored-by: Koundinya Veluri <[email protected]>
…adPool.cs Co-authored-by: Koundinya Veluri <[email protected]>
|
Timeouts are unrelated, merging |
ghost
locked as resolved and limited conversation to collaborators
