Skip to content

Conversation

EgorBo
Copy link
Member

@EgorBo EgorBo commented Oct 5, 2025

@Copilot Copilot AI review requested due to automatic review settings October 5, 2025 01:55
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR optimizes GUID creation by replacing string-based parsing with direct constructor calls using hexadecimal values. This change eliminates the runtime overhead of parsing GUID strings while maintaining the same GUID values.

Key changes:

  • Replace new Guid(string) calls with new Guid(uint, ushort, ushort, byte, byte, byte, byte, byte, byte, byte, byte) constructor
  • Add inline comments showing the original GUID string values for reference

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
RuntimeEventSource.cs Converts EventSourceGuid initialization from string parsing to direct constructor with hex values
EventSource.cs Converts Metrics EventSource GUID initialization from string parsing to direct constructor with hex values

@EgorBo EgorBo mentioned this pull request Oct 5, 2025
11 tasks
@jkotas jkotas merged commit 83c947b into dotnet:main Oct 5, 2025
140 of 142 checks passed
@am11
Copy link
Member

am11 commented Oct 5, 2025

Backport candidate? (along with #120364)

@EgorBo
Copy link
Member Author

EgorBo commented Oct 5, 2025

Backport candidate? (along with #120364)

I believe so

@am11
Copy link
Member

am11 commented Oct 5, 2025

It seems other related fixes (i.e. #120422) are also in progress. We can batch them in a backport PR.

@EgorBo
Copy link
Member Author

EgorBo commented Oct 6, 2025

/backport to release/10.0

Copy link
Contributor

github-actions bot commented Oct 6, 2025

@xtqqczze
Copy link
Contributor

xtqqczze commented Oct 6, 2025

There is now an analyser: MA0176: Optimize guid creation

@EgorBo
Copy link
Member Author

EgorBo commented Oct 6, 2025

There is now an analyser: MA0176: Optimize guid creation

I don't think it should be an analyzer. we can optimize this in JIT.
The ctor that accepts integers makes guid difficult to search or copy-paste.

@jkotas
Copy link
Member

jkotas commented Oct 7, 2025

we can optimize this in JIT.

In Tier0 where the Guids are often used for one-time registrations?

I think it is fine for this to be an analyzer.

@EgorBo
Copy link
Member Author

EgorBo commented Oct 7, 2025

we can optimize this in JIT.

In Tier0 where the Guids are often used for one-time registrations?

I think it is fine for this to be an analyzer.

Well, T0 can be fixed by prejitting 🤷 or with a simple jit intrinsic that will be T0 friendly. My concern that it doesn't look nice. Difficult to search, copy and replace with a different Guid.
There are 10k uses of new Guid(" and Guid.Parse(" on grep.app

@xtqqczze
Copy link
Contributor

xtqqczze commented Oct 7, 2025

I wonder if we could do something at compile time using source generators:

[GeneratedGuid("20752bc4-c151-50f5-f27b-df92d8af5a61")]
internal static partial Guid EventSourceGuid;

@am11
Copy link
Member

am11 commented Oct 7, 2025

Well, T0 can be fixed by prejitting 🤷 or with a simple jit intrinsic that will be T0 friendly.

Something like main...am11:runtime:feature/jit/guid-intrinsic? (credit to copilot 🤖)

@jkotas
Copy link
Member

jkotas commented Oct 7, 2025

My concern that it doesn't look nice.

There is nothing nice about users having to inline magic GUIDs into their code in any shape or form. GUIDs should be auto generated and out of sight, and whatever is autogenerating them can make sure to use the efficient constructor. I do not think it is worth burning our complexity budget on things like this.

It would be a different story if GUID parsing got optimized without being special cased in any way, and it was just used as an example of powerful optimizations in the perf blog post. I would be fine with that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants