-
Notifications
You must be signed in to change notification settings - Fork 2k
Avoid layout diagnostics allocations without listeners #35475
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,18 +10,52 @@ internal static class DiagnosticInstrumentation | |
| /// </summary> | ||
| /// <param name="view">The view to instrument.</param> | ||
| /// <returns>Returns an instance of <see cref="LayoutMeasureInstrumentation"/> if instrumentation is supported; otherwise, null.</returns> | ||
| public static LayoutMeasureInstrumentation? StartLayoutMeasure(IView view) => | ||
| RuntimeFeature.IsMeterSupported | ||
| ? new LayoutMeasureInstrumentation(view) | ||
| : null; | ||
| public static LayoutMeasureInstrumentation? StartLayoutMeasure(IView view) | ||
| { | ||
| if (!RuntimeFeature.IsMeterSupported) | ||
| { | ||
| return null; | ||
| } | ||
|
|
||
| var diagnostics = view.GetMauiDiagnostics(); | ||
| if (diagnostics is null) | ||
| { | ||
| return null; | ||
| } | ||
|
|
||
| var metrics = diagnostics.GetMetrics<LayoutDiagnosticMetrics>(); | ||
| if (!diagnostics.HasActivityListeners && metrics?.IsMeasureEnabled != true) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [minor] AOT/trim & hot-path DI lookup — Every measure/arrange on every view pays a |
||
| { | ||
| return null; | ||
| } | ||
|
|
||
| return new LayoutMeasureInstrumentation(view, diagnostics, metrics); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Starts layout arrange instrumentation for the specified view. | ||
| /// </summary> | ||
| /// <param name="view">The view to instrument.</param> | ||
| /// <returns>Returns an instance of <see cref="LayoutArrangeInstrumentation"/> if instrumentation is supported; otherwise, null.</returns> | ||
| public static LayoutArrangeInstrumentation? StartLayoutArrange(IView view) => | ||
| RuntimeFeature.IsMeterSupported | ||
| ? new LayoutArrangeInstrumentation(view) | ||
| : null; | ||
| public static LayoutArrangeInstrumentation? StartLayoutArrange(IView view) | ||
| { | ||
| if (!RuntimeFeature.IsMeterSupported) | ||
| { | ||
| return null; | ||
| } | ||
|
|
||
| var diagnostics = view.GetMauiDiagnostics(); | ||
| if (diagnostics is null) | ||
| { | ||
| return null; | ||
| } | ||
|
|
||
| var metrics = diagnostics.GetMetrics<LayoutDiagnosticMetrics>(); | ||
| if (!diagnostics.HasActivityListeners && metrics?.IsArrangeEnabled != true) | ||
| { | ||
| return null; | ||
| } | ||
|
|
||
| return new LayoutArrangeInstrumentation(view, diagnostics, metrics); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,21 +5,59 @@ namespace Microsoft.Maui.Diagnostics; | |
| /// <summary> | ||
| /// Instrumentation for the layout arrange phase of a view. | ||
| /// </summary> | ||
| readonly struct LayoutArrangeInstrumentation(IView view) : IDiagnosticInstrumentation | ||
| readonly struct LayoutArrangeInstrumentation : System.IDisposable | ||
| { | ||
| readonly Activity? _activity = view.StartDiagnosticActivity("Arrange"); | ||
| readonly IView _view; | ||
| readonly IDiagnosticsManager _diagnostics; | ||
| readonly LayoutDiagnosticMetrics? _metrics; | ||
| readonly Activity? _activity; | ||
| readonly bool _metricsDurationStarted; | ||
| readonly long _metricsStartTimestamp; | ||
|
|
||
| public LayoutArrangeInstrumentation(IView view, IDiagnosticsManager diagnostics, LayoutDiagnosticMetrics? metrics) | ||
| { | ||
| _view = view; | ||
| _diagnostics = diagnostics; | ||
| _metrics = metrics; | ||
|
|
||
| if (diagnostics.HasActivityListeners) | ||
| { | ||
| diagnostics.GetTags(view, out var tagList); | ||
| _activity = diagnostics.ActivitySource.StartActivity( | ||
| ActivityKind.Internal, | ||
| name: $"Arrange {view.GetType().Name}", | ||
| tags: tagList); | ||
| } | ||
| else | ||
| { | ||
| _activity = null; | ||
| } | ||
|
|
||
| _metricsDurationStarted = metrics?.IsArrangeDurationEnabled == true; | ||
| _metricsStartTimestamp = _metricsDurationStarted | ||
| ? Stopwatch.GetTimestamp() | ||
| : 0; | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Disposes the instrumentation and stops the diagnostic activity. | ||
| /// </summary> | ||
| public void Dispose() => | ||
| view.StopDiagnostics(_activity, this); | ||
| public void Dispose() | ||
| { | ||
| var metrics = _metrics; | ||
| var recordDuration = _metricsDurationStarted && metrics?.IsArrangeDurationEnabled == true; | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [minor] Logic — |
||
| var duration = recordDuration | ||
| ? LayoutDiagnosticMetrics.GetElapsedNanoseconds(_metricsStartTimestamp) | ||
| : 0; | ||
|
|
||
| /// <summary> | ||
| /// Records the stopping of the instrumentation and publishes various metrics. | ||
| /// </summary> | ||
| /// <param name="diagnostics">The <see cref="IDiagnosticsManager"/> instance.</param> | ||
| /// <param name="tagList">The tags associated with the instrumentation.</param> | ||
| public void Stopped(IDiagnosticsManager diagnostics, in TagList tagList) => | ||
| diagnostics.GetMetrics<LayoutDiagnosticMetrics>()?.RecordArrange(_activity?.Duration, in tagList); | ||
| _activity?.Stop(); | ||
|
|
||
| if (metrics?.IsArrangeEnabled == true) | ||
| { | ||
| _diagnostics.GetTags(_view, out var tagList); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [minor] Performance — duplicated tagger walk — When both |
||
| metrics.RecordArrange(duration, recordDuration, in tagList); | ||
| } | ||
|
|
||
| _activity?.Dispose(); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,3 @@ | ||
| using System; | ||
| using System.Diagnostics; | ||
| using System.Diagnostics.Metrics; | ||
|
|
||
|
|
@@ -29,6 +28,18 @@ internal class LayoutDiagnosticMetrics : IDiagnosticMetrics | |
| /// </summary> | ||
| internal Histogram<int>? ArrangeHistogram { get; private set; } | ||
|
|
||
| internal bool IsMeasureEnabled => | ||
| MeasureCounter?.Enabled == true || | ||
| MeasureHistogram?.Enabled == true; | ||
|
|
||
| internal bool IsMeasureDurationEnabled => MeasureHistogram?.Enabled == true; | ||
|
|
||
| internal bool IsArrangeEnabled => | ||
| ArrangeCounter?.Enabled == true || | ||
| ArrangeHistogram?.Enabled == true; | ||
|
|
||
| internal bool IsArrangeDurationEnabled => ArrangeHistogram?.Enabled == true; | ||
|
|
||
| /// <inheritdoc/> | ||
| public void Create(Meter meter) | ||
| { | ||
|
|
@@ -42,38 +53,52 @@ public void Create(Meter meter) | |
| /// <summary> | ||
| /// Records a measure operation with an optional duration and associated tags. | ||
| /// </summary> | ||
| /// <param name="duration">The duration of the measure operation.</param> | ||
| /// <param name="duration">The duration of the measure operation in nanoseconds.</param> | ||
| /// <param name="recordDuration">Whether a duration should be recorded.</param> | ||
| /// <param name="tagList">The tags associated with the measure operation.</param> | ||
| public void RecordMeasure(TimeSpan? duration, in TagList tagList) | ||
| public void RecordMeasure(int duration, bool recordDuration, in TagList tagList) | ||
| { | ||
| MeasureCounter?.Add(1, tagList); | ||
| if (MeasureCounter?.Enabled == true) | ||
| { | ||
| MeasureCounter.Add(1, tagList); | ||
| } | ||
|
|
||
| if (duration is not null) | ||
| if (recordDuration && MeasureHistogram?.Enabled == true) | ||
| { | ||
| #if NET9_0_OR_GREATER | ||
| MeasureHistogram?.Record((int)duration.Value.TotalNanoseconds, tagList); | ||
| #else | ||
| MeasureHistogram?.Record((int)(duration.Value.TotalMilliseconds * 1_000_000), tagList); | ||
| #endif | ||
| MeasureHistogram.Record(duration, tagList); | ||
| } | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Records an arrange operation with an optional duration and associated tags. | ||
| /// </summary> | ||
| /// <param name="duration">The duration of the arrange operation.</param> | ||
| /// <param name="duration">The duration of the arrange operation in nanoseconds.</param> | ||
| /// <param name="recordDuration">Whether a duration should be recorded.</param> | ||
| /// <param name="tagList">The tags associated with the arrange operation.</param> | ||
| public void RecordArrange(TimeSpan? duration, in TagList tagList) | ||
| public void RecordArrange(int duration, bool recordDuration, in TagList tagList) | ||
| { | ||
| ArrangeCounter?.Add(1, tagList); | ||
| if (ArrangeCounter?.Enabled == true) | ||
| { | ||
| ArrangeCounter.Add(1, tagList); | ||
| } | ||
|
|
||
| if (duration is not null) | ||
| if (recordDuration && ArrangeHistogram?.Enabled == true) | ||
| { | ||
| #if NET9_0_OR_GREATER | ||
| ArrangeHistogram?.Record((int)duration.Value.TotalNanoseconds, tagList); | ||
| #else | ||
| ArrangeHistogram?.Record((int)(duration.Value.TotalMilliseconds * 1_000_000), tagList); | ||
| #endif | ||
| ArrangeHistogram.Record(duration, tagList); | ||
| } | ||
| } | ||
|
|
||
| internal static int GetElapsedNanoseconds(long startTimestamp) | ||
| { | ||
| var elapsedTimestamp = Stopwatch.GetTimestamp() - startTimestamp; | ||
| if (elapsedTimestamp <= 0) | ||
| { | ||
| return 0; | ||
| } | ||
|
|
||
| var elapsedNanoseconds = elapsedTimestamp * (1_000_000_000.0 / Stopwatch.Frequency); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [minor] Performance — recomputed scale factor — |
||
| return elapsedNanoseconds >= int.MaxValue | ||
| ? int.MaxValue | ||
| : (int)elapsedNanoseconds; | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[minor] API surface —
HasActivityListenersduplicatesActivitySource.HasListeners()— Since the interface already exposesActivitySource, callers can writediagnostics.ActivitySource.HasListeners()directly and skip widening the interface. Two reasons to keep the new member anyway, both valid: (a) tighter intent at the gate site, (b) easier mocking in unit tests. Suggest adding a one-line XML doc clarifying it's a hot-path gate so future readers don't treat it as redundant API.