Skip to content

perf: avoid MeasurementUnit enum boxing allocations #5218

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.

Sign up for GitHub

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

Open
jamescrosswell wants to merge 7 commits into
base: main
Choose a base branch
from
Open

perf: avoid MeasurementUnit enum boxing allocations #5218

Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
f0cdc3e
perf: avoid MeasurementUnit enum boxing allocations
jamescrosswell May 11, 2026
4a7a2ec
Generate predefined names at runtime/startup
jamescrosswell May 11, 2026
9e9b00e
Format code
getsentry-bot May 11, 2026
f957b12
fix(metrics): preserve lowercase MeasurementUnit serialization
jamescrosswell May 11, 2026
e56ae80
test(metrics): guard MeasurementUnit enum-name mappings
jamescrosswell May 11, 2026
9e3fe7d
test: make MeasurementUnit enum-coverage tests framework-compatible
jamescrosswell May 12, 2026
7cae1ec
test: make MeasurementUnit enum sync tests net48-compatible
jamescrosswell May 12, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
89 changes: 81 additions & 8 deletions src/Sentry/MeasurementUnit.cs
View file
Open in desktop
Copy link
Copy Markdown
Member

@Flash0ver Flash0ver May 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thought:

This is a similar implementation a coding agent on my end suggested when I gave it a try when looking at it with a friend of mine just last weekend 😉.

I'm not too certain if I'm happy with this approach, though:

  • the size of the struct is increasing
  • we embed more data into the assembly (via the string-arrays)

On the other hand, this (or a similar) implementation is necessary, in order to not change the IEquatable-semantics.

We were then experimenting with the struct only having a string field,
but that would change the Equality-semantics, where

  • MeasurementUnit.Duration.Millisecond == MeasurementUnit.Custom("millisecond")
    • old/this: would not be equal, because constructed differently
    • only with string field: would now become equal, being a behavioral change

Copy link
Copy Markdown
Member

@Flash0ver Flash0ver May 11, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wait ... actually ... I believe I am wrong concerning the size ... at least partially wrong

  • previous
    • System.Enum + System.String
    • on 64-bit: 8-byte-reference + 8-byte reference = 16B
    • on 32-bit: 4-byte-reference + 4-byte reference = 8B
  • this change
    • UnitKind enum (byte) + System.Int32 + System.String
    • on 64-bit: 1-byte enum + 4-byte int + 3-byte padding, 8-byte reference = 16B
    • on 32-bit: 1-byte enum + 3-byte padding + 4-byte int + 4-byte reference + padding = 12B

So we are only increasing the size of the struct by 4B on 32-bit systems/processes.

Yeah ... that's neglectable considering we no longer box to System.Enum.

Copy link
Copy Markdown
Member

@Flash0ver Flash0ver May 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

about the Equals-Semantics

If we would only keep a string field, assigning the string via a switch on constructions, we would simplify the code, reduce the struct size to 4B/8B (32-/64-bit systems); but we would change the Equals, as it would no longer matter whether the MeasurementUnit was constructed via MeasurementUnit.Custom or the respective implicit-ennum-conversion.

This being a behavioral change, we could simplify MeasurementUnit in the next Major ... or should we keep the Equals-Behavior as is, where there is a difference how the MeasurementUnit was constructed.

What do you think?

Copy link
Copy Markdown
Collaborator Author

@jamescrosswell jamescrosswell May 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To err on the side of caution (since this API is probably quite widely used already), maybe we delay the behavioural change (and saving the extra 4B) until the next major release.

Original file line number Diff line number Diff line change
Expand Up @@ -6,18 +6,80 @@ namespace Sentry;
/// <seealso href="https://getsentry.github.io/relay/relay_metrics/enum.MetricUnit.html"/>
public readonly partial struct MeasurementUnit : IEquatable<MeasurementUnit>
{
private readonly Enum? _unit;
private readonly UnitKind _kind;
private readonly int _value;
private readonly string? _name;

private MeasurementUnit(Enum unit)
private enum UnitKind : byte
{
_unit = unit;
None = 0,
Duration = 1,
Information = 2,
Fraction = 3,
Custom = 4
}

private static readonly string[] DurationNames =
Copy link
Copy Markdown
Member

@Flash0ver Flash0ver May 11, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thought: switch instead of Array-Lookups

My AI assistant also suggested a similar solution.

Instead of embedding these arrays into the assembly and then indexing into these arrays,
we could do a switch in all of the three (Duration, Fraction, Information) implicit conversions and assign the name/string directly, while still keeping the new UnitKind enum so that we don't change Equality-semantics on how the struct is created (MeasurementUnit.Custom or via any of the three implicit "from-enum"-conversions).

Copy link
Copy Markdown
Collaborator Author

@jamescrosswell jamescrosswell May 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @Flash0ver. I didn't much like the duplication in the old code at all.

We could do a switch statement, but I think it would suffer from the same duplication and whilst it would reduce heap allocations (likley G2 for statics), we grow the size of the codebase by the same amount we save in heap allocations. I think the end result is having to load more data into L1 cache every time the method executes, which might actually be less performant.

I changed the LLMs code so that the names are generated dynamically in the static constructor (negligible one time hit at startup and allocates a couple of tiny arrays to G2 heap that won't trouble the GC). That makes the code easier to maintain whilst still avoiding the boxing, I believe.

Copy link
Copy Markdown
Collaborator Author

@jamescrosswell jamescrosswell May 11, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dang... Enum.GetNames isn't trim safe.

Seems we have to go with either hard coded arrays or a source generator. Hard coded is probably easier - will add some unit tests to ensure these don't break if we add any enum values in the future.

If we did want to use a source generator, this is actually the example that Andrew Lock uses in his blog about them, so he's already built and tested some we could use OOTB.

[
"nanosecond",
"microsecond",
"millisecond",
"second",
"minute",
"hour",
"day",
"week"
];

private static readonly string[] InformationNames =
[
"bit",
"byte",
"kilobyte",
"kibibyte",
"megabyte",
"mebibyte",
"gigabyte",
"gibibyte",
"terabyte",
"tebibyte",
"petabyte",
"pebibyte",
"exabyte",
"exbibyte"
];

private static readonly string[] FractionNames =
[
"ratio",
"percent"
];

private MeasurementUnit(Duration unit)
{
_kind = UnitKind.Duration;
_value = (int)unit;
_name = null;
}

private MeasurementUnit(Information unit)
{
_kind = UnitKind.Information;
_value = (int)unit;
_name = null;
}

private MeasurementUnit(Fraction unit)
{
_kind = UnitKind.Fraction;
_value = (int)unit;
_name = null;
}

private MeasurementUnit(string name)
{
_unit = null;
_kind = UnitKind.Custom;
_value = default;
_name = name;
}

Expand Down Expand Up @@ -70,21 +132,32 @@ internal static MeasurementUnit Parse(string? name)
return Custom(name);
}

private string? GetPredefinedName()
{
return _kind switch
{
UnitKind.Duration when (uint)_value < (uint)DurationNames.Length => DurationNames[_value],
UnitKind.Information when (uint)_value < (uint)InformationNames.Length => InformationNames[_value],
UnitKind.Fraction when (uint)_value < (uint)FractionNames.Length => FractionNames[_value],
_ => null
};
}

/// <summary>
/// Returns the string representation of the measurement unit, as it will be sent to Sentry.
/// </summary>
public override string ToString() => _unit?.ToString().ToLowerInvariant() ?? _name ?? "";
public override string ToString() => ToNullableString() ?? "";

internal string? ToNullableString() => _unit?.ToString().ToLowerInvariant() ?? _name;
internal string? ToNullableString() => _name ?? GetPredefinedName();

/// <inheritdoc />
public bool Equals(MeasurementUnit other) => Equals(_unit, other._unit) && _name == other._name;
public bool Equals(MeasurementUnit other) => _kind == other._kind && _value == other._value && _name == other._name;

/// <inheritdoc />
public override bool Equals(object? obj) => obj is MeasurementUnit other && Equals(other);

/// <inheritdoc />
public override int GetHashCode() => HashCode.Combine(_unit, _name, _unit?.GetType());
public override int GetHashCode() => HashCode.Combine(_kind, _value, _name);

/// <summary>
/// Returns true if the operands are equal.
Expand Down
Loading