This is great! There are more places to apply this, like all the WriteVerbose and WriteWarning calls in engine. This will save a great deal of string allocations.

This is great! There are more places to apply this, like all the WriteVerbose and WriteWarning calls in engine. This will save a great deal of string allocations.

I suggest doing this in some follow PRs because we need to change dozens of files, which will make it difficult to review this PR.

Leveraging the interpolated string handler for WriteLine doesn't further reduce allocations for the common cases where tracing is disabled, because your previous PR #10052 already eliminated allocation (mostly) for WriteLine when tracing is disabled.

The current change in this PR will reduce more allocations when tracing is enabled, but that's not a common scenario and thus not very interesting (reducing allocation is always good, it's just not as important in this case).

Back to the design of the current change, it would require one handler implementation per every logging option, because the handler has built-in assumption of the tracing option to check against. This is obviously not desired. What we want is a single handler implementation that can work for all logging options. To achieve this goal, we need to first do significant refactoring to the existing PSTraceSource methods -- changing method like TraceWarning(xxx), WriteLine(xxx) and etc. to Trace(Option option, xxxx) -- so that each method doesn't have built-in assumption but instead the passed-in option instructs what to do with this tracing operation. However, the refactoring work will be massive.

Leveraging the interpolated string handler for WriteLine doesn't further reduce allocations for the common cases where tracing is disabled, because your previous PR #10052 already eliminated allocation (mostly) for WriteLine when tracing is disabled.

We still do early evaluating of arguments (there is even StringBuilder!). We have reduced boxing with a lot of helpers, but now we can remove them and use a single method, which is much easier and clearer.
When tracing is disabled we reduce allocations even more significantly.
All this already makes this change very useful.

However, the refactoring work will be massive.

I am trying to keep the PR as small as possible.
My plan:

1. Merge the PR
2. Remove all WriteLine() helpers and replace with new one in follow PR
3. Think whether we want to continue and refactor other trace helpers.
   They are so rarely used that I thought it might not be necessary. But we can unify all methods with one Trace(Option option, xxxx) in the next PR if you want.

When tracing is disabled, we reduce allocations even more significantly.
All this already makes this change very useful.

Can you please point me to some examples to prove the bold part?

Also, back to the design of the current change, I have concern about having one handler implementation per every logging option. See the 3rd paragraph in my comment #18246 (comment) for details.

When tracing is disabled, we ideally should never call to the WriteLine methods. That's doable for the parameter binders but may be harder for the uses in some other places.

When tracing is disabled, we reduce allocations even more significantly.
All this already makes this change very useful.

Can you please point me to some examples to prove the bold part?

You can look how WriteLine(string format, object arg1) is used (384 times). Most is strings but there are FileInfo, Uri, StringBuilder, ...

Also, back to the design of the current change, I have concern about having one handler implementation per every logging option. See the 3rd paragraph in my comment #18246 (comment) for details.

Options:

1. Most benefits we get from WriteLine() refactoring. Rest of trace methods we could ignore.
2. Create some handlers since they are very simple.
3. I'd prefer to convert all methods to Trace(Option option, xxxx) and use one handler.

This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days.
Maintainer, please provide feedback and/or mark it as Waiting on Author

You can look how WriteLine(string format, object arg1) is used (384 times). Most is strings but there are FileInfo, Uri, StringBuilder, ...

Having FileInfo, Uri, StringBuilder passed in to arg1 in this case doesn't cause any allocation when the tracing is disabled, so it's still not clear to me where the big saving would be from.

I'd prefer to convert all methods to Trace(Option option, xxxx) and use one handler.

This would be most ideal -- being consistent and modern. That will also allow the pattern to be easily copied in other places. The problem is that the saving in allocation will not be obvious when the tracing is disabled (this is the common scenario), which will make the value/cost of this work less attractive :(

Again, I think when tracing is disabled, we ideally should never call to the WriteLine methods. That's doable for the parameter binders but may be harder for the uses in some other places.

Again, I think when tracing is disabled, we ideally should never call to the WriteLine methods. That's doable for the parameter binders but may be harder for the uses in some other places.

Can you clarify how we could achieve this?
Since it is in run time we should check if (_flags.HasFlag(PSTraceSourceOptions.WriteLine)) in every callsite. Do you say we need to move the check out of WriteLine method?

Having FileInfo, Uri, StringBuilder passed in to arg1 in this case doesn't cause any allocation when the tracing is disabled, so it's still not clear to me where the big saving would be from.

We seem to be in slightly different contexts. :-) I must have made a bad initial description.
Earlier we added many helper methods to remove unnecessary allocations. It was a workaround that didn't eliminate the allocation problem completely.

Now we can remove all these helper methods and use a single method that completely solves the problem. There are still limitations there, but they are not critical since the API is not public.

I'd prefer to convert all methods to Trace(Option option, xxxx) and use one handler.

This would be most ideal -- being consistent and modern. That will also allow the pattern to be easily copied in other places.

We still can keep using specific names like WriteLine and have one handler. For this we should have one more optional parameter in the handler and in the methods. What is pattern from the two ones you like more?

The problem is that the saving in allocation will not be obvious when the tracing is disabled (this is the common scenario), which will make the value/cost of this work less attractive :(

The benefit is that the new pattern eliminates allocations and errors like s_tracer.WriteLine("DSC ClassCache: loading file '{0}' added the following classes to the cache: {1}", path, sb.ToString());.

This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days.
Maintainer, please provide feedback and/or mark it as Waiting on Author

This PR has 3098 quantified lines of changes. In general, a change size of upto 200 lines is ideal for the best PR experience!

Label      : Extra Large
Size       : +1243 -1855
Percentile : 100%

Total files changed: 67

Change summary by file extension:
.cs : +1242 -1855
.ps1 : +1 -0

Was this comment helpful? 👍  :ok_hand:  :thumbsdown: (Email)
Customize PullRequestQuantifier for this repository.