Skip to content

[EventGrid] Generate switch expressions instead of dictionary lookups #49426

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

Merged
merged 3 commits into from
Apr 16, 2025

Conversation

danielmarbach
Copy link
Contributor

@danielmarbach danielmarbach commented Apr 15, 2025

This PR refactors the SystemEventExtensions.AsSystemEventData method by replacing the existing dictionary-based lookup with a switch expression. This change directly maps event type strings to their corresponding deserialization methods.

While the performance gains are relatively small and mostly noticeable in microbenchmarks or hot-path scenarios, this change streamlines the code and enables the runtime to produce more efficient IL. The overall design remains functionally equivalent, ensuring a smooth transition without breaking existing functionality.

Assumptions

  • Performance Improvements: The if statements reduce overhead by eliminating the need for dictionary lookups and indirect delegate calls. This leads to fewer IL-level indirections and leverages direct branch optimizations, which are advantageous in frequently executed code paths.
  • Optimized IL Code: The if statements compiles into more predictable branching logic. This allows the JIT compiler to apply inlining and other optimizations more effectively, further enhancing runtime performance.
  • Code Clarity and Maintainability: Directly associating event types with their deserialization logic in an if statement improves readability. The intent of the code becomes clearer, making it easier to maintain and less error-prone compared to managing a separate dictionary.

Before

    internal class SystemEventExtensions
    {
        public static object AsSystemEventData(string eventType, JsonElement data)
        {
            if (s_systemEventDeserializers.TryGetValue(eventType, out Func<JsonElement, object> systemDeserializationFunction))
            {
                return systemDeserializationFunction(data);
            }
            else
            {
                return null;
            }
        }

        internal static readonly IReadOnlyDictionary<string, Func<JsonElement, object>> s_systemEventDeserializers = new Dictionary<string, Func<JsonElement, object>>(StringComparer.OrdinalIgnoreCase)
        {
            { SystemEventNames.AcsRecordingFileStatusUpdated, AcsRecordingFileStatusUpdatedEventData.DeserializeAcsRecordingFileStatusUpdatedEventData },
            { SystemEventNames.AcsRouterJobClassificationFailed, AcsRouterJobClassificationFailedEventData.DeserializeAcsRouterJobClassificationFailedEventData },
            ...
        }

After

    internal class SystemEventExtensions
    {
        public static object AsSystemEventData(string eventType, JsonElement data)
        {
            var eventTypeSpan = eventType.AsSpan();
            if (eventTypeSpan.Equals(SystemEventNames.AcsRecordingFileStatusUpdated.AsSpan(), StringComparison.OrdinalIgnoreCase))
                return AcsRecordingFileStatusUpdatedEventData.DeserializeAcsRecordingFileStatusUpdatedEventData(data);
            if (eventTypeSpan.Equals(SystemEventNames.AcsRouterJobClassificationFailed.AsSpan(), StringComparison.OrdinalIgnoreCase))
                ...
            return null;
        }
    }       

@github-actions github-actions bot added Community Contribution Community members are working on the issue customer-reported Issues that are reported by GitHub users external to the Azure organization. Event Grid labels Apr 15, 2025
Copy link

Thank you for your contribution @danielmarbach! We will review the pull request and get back to you soon.

@danielmarbach
Copy link
Contributor Author

Pinging @m-nash @jsquire @JoshLove-msft @weshaggard since you have reviewed / worked on the previous PR

@azure-sdk
Copy link
Collaborator

API change check

API changes are not detected in this pull request.

@danielmarbach danielmarbach force-pushed the system-event-data-generator branch from f837c2a to da46ab7 Compare April 15, 2025 20:25
Copy link
Member

@JoshLove-msft JoshLove-msft left a comment

Choose a reason for hiding this comment

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

Thanks for the enhancement @danielmarbach! It looks like there are some test failures. Can you please take a look?

@danielmarbach
Copy link
Contributor Author

@JoshLove-msft I looked at those on my machine on macos and they all passed on Net8 and net9 I can give it another look later

@danielmarbach
Copy link
Contributor Author

@JoshLove-msft this should work. I missed the ordinal ignore case comparison

@JoshLove-msft JoshLove-msft merged commit 1da186b into Azure:main Apr 16, 2025
31 checks passed
@danielmarbach danielmarbach deleted the system-event-data-generator branch April 16, 2025 18:58
@danielmarbach
Copy link
Contributor Author

@JoshLove-msft in case you are bored ;) #49445

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Community Contribution Community members are working on the issue customer-reported Issues that are reported by GitHub users external to the Azure organization. Event Grid
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants