Uh oh!
There was an error while loading. Please reload this page.
Metrics Feature Switch - #91767
Conversation
tarekgh
commented
Sep 7, 2023
noahfalk
left a comment
There was a problem hiding this comment.
LGTM with a couple questions inline
| /// <param name="tags">A span of key-value pair tags associated with the measurement.</param> | ||
| protected void RecordMeasurement(T measurement, ReadOnlySpan<KeyValuePair<string, object?>> tags) | ||
| { | ||
| if (!Meter.IsSupported) |
There was a problem hiding this comment.
Can we avoid adding this check and count on _subscription.First to be null? It looks like you already blocked all the code paths that could have registered a subscription.
There was a problem hiding this comment.
Yes, we can do that. According to my understanding (per @eerhardt explanation) jit is going to optimize that in tier two and will fully remove this check. I was hoping when IsSupported is false, the linker can empty the whole method body including accessing the static fields. Let me know if you still think we should remove this check.
There was a problem hiding this comment.
Can you confirm the JIT does optimize it as expected. As long as that is confirmed then I have no qualms leaving it there :)
There was a problem hiding this comment.
Adding this check will allow the trimmer to remove the rest of the method body, right?
There was a problem hiding this comment.
Adding this check will allow the trimmer to remove the rest of the method body, right?
Correct - and all the methods it calls that aren't called elsewhere.
There was a problem hiding this comment.
I created a simple app using my private built. The app and the runtime were built as a release. I have IsSupported = true to test the normal case. I run it under the debugger, and I am seeing the call Meter.IsSupported is not removed inside RecordMeasurement method. I'll remove this check from there as this is a hot path, we want to avoid regressions there.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
eerhardt
left a comment
There was a problem hiding this comment.
Should we also have a trimming test that ensures certain things are trimmed?
Can you show a before and after size comparison (maybe using ILSpy or https://github.com/MichalStrehovsky/sizoscope) that shows what is all trimmed when this switch is set?
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
tarekgh
commented
Sep 12, 2023
/backport to release/8.0 |
Started backporting to release/8.0: https://github.com/dotnet/runtime/actions/runs/6165846484 |
@tarekgh backporting to release/8.0 failed, the patch most likely resulted in conflicts: $ git am --3way --ignore-whitespace --keep-non-patch changes.patch
Applying: Metrics Feature Switch
Using index info to reconstruct a base tree...
M docs/workflow/trimming/feature-switches.md
M src/libraries/System.Diagnostics.DiagnosticSource/src/System.Diagnostics.DiagnosticSource.csproj
Falling back to patching base and 3-way merge...
Auto-merging src/libraries/System.Diagnostics.DiagnosticSource/src/System.Diagnostics.DiagnosticSource.csproj
CONFLICT (content): Merge conflict in src/libraries/System.Diagnostics.DiagnosticSource/src/System.Diagnostics.DiagnosticSource.csproj
Auto-merging docs/workflow/trimming/feature-switches.md
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Metrics Feature Switch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128Please backport manually! |
@tarekgh an error occurred while backporting to release/8.0, please check the run log for details! Error: git am failed, most likely due to a merge conflict. |
#89880