Skip to content

Conversation

@ccastanedaucf
Copy link
Contributor

@ccastanedaucf ccastanedaucf commented Jun 7, 2025

Fixes

Millions of delegate allocations in EventSourceSink by inlining callback control flow into a couple switch statements.

Context

EventSourceSink currently routes each event type to the appropriate handler with an optional follow-up:

switch (buildEvent)
{
	case BuildMessageEventArgs buildMessageEvent:
		RaiseEvent(buildMessageEvent, args => MessageRaised?.Invoke(null, args), RaiseAnyEvent);
		break;
	case TaskStartedEventArgs taskStartedEvent:
		ArgsHandler<TaskStartedEventArgs> taskStartedFollowUp = args => RaiseEvent(args, args => StatusEventRaised?.Invoke(null, args), RaiseAnyEvent);
		RaiseEvent(taskStartedEvent, args => TaskStarted?.Invoke(null, args), taskStartedFollowUp);
		break;
case TaskFinishedEventArgs taskFinishedEvent:
	// ect...
private void RaiseEvent<TArgs>(TArgs buildEvent, ArgsHandler<TArgs> handler, ArgsHandler<TArgs>? followUpHandler)
	where TArgs : BuildEventArgs
{
	handler(buildEvent)
	// error handling...
	followUpHandler?.Invoke(buildEvent);
}

Since these need to be typed and share some common logic, the current implementation creates a delegate for each of these.

Right now though, this essentially causes an allocation per-event since these are state-capturing (the delegate needs a reference to the instance).

Here's a trace scoped to own allocations coming from EventSourceSink (so the top Consume is in addition to all the others!).

image

Changes Made

One realization to make is that other than the typed handler, the follow up logic is either:

  • Call StatusRaisedEvent.Invoke with the core exception handling
  • Call RaiseAnyEvent(buildEvent)
  • Do nothing

Therefore, the above can be reduced to a couple switches:

switch (buildEvent)
{
	case BuildMessageEventArgs buildMessageEvent:
		MessageRaised?.Invoke(null, buildMessageEvent);
		break;
	case TaskStartedEventArgs taskStartedEvent:
		TaskStarted?.Invoke(null, taskStartedEvent);
                StatusEventRaised?.Invoke(null, taskStartedEvent);
		break;
	case TaskFinishedEventArgs taskFinishedEvent:
	// ect...
}

// Common exception handling

switch (buildEvent)
{
	case BuildMessageEventArgs:
	case TaskStartedEventArgs:
	case TaskFinishedEventArgs:
	// fall through...
		RaiseAnyEvent(buildEvent);
		break;
	// other cases - do nothing

Testing

And after, it's allocation free.

image

Notes

Copilot AI review requested due to automatic review settings June 7, 2025 01:49
Copy link
Contributor

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 the EventSourceSink by inlining event handler calls to eliminate millions of delegate allocations while preserving the original exception handling behavior.

  • Inlines event handler invocations for various BuildEventArgs types.
  • Consolidates exception handling with a try-catch block that unregisters all event handlers on failure.
  • Introduces a secondary switch to route events to RaiseAnyEvent for select event types.
Comments suppressed due to low confidence (2)

src/Build/BackEnd/Components/Logging/EventSourceSink.cs:321

  • [nitpick] Add a comment explaining why these specific event types are routed to RaiseAnyEvent in the secondary switch, which will clarify the design intent for future maintainers.
case BuildMessageEventArgs:

src/Build/BackEnd/Components/Logging/EventSourceSink.cs:305

  • [nitpick] Document the reasoning for filtering out InternalErrorException in the catch block to aid future debugging and ensure consistent understanding of the exception handling strategy.
catch (Exception exception) when (exception is not InternalErrorException)

@ccastanedaucf ccastanedaucf force-pushed the dev/chcasta-perf-eventsourcesink branch from 3d68e7a to 6912e19 Compare June 7, 2025 02:24
@SimaTian SimaTian merged commit 90df666 into dotnet:main Jun 10, 2025
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants