Skip to content

fix(csharp): cache extensions per descriptor - #29707

Open
awe123343 wants to merge 1 commit into
protocolbuffers:mainfrom
awe123343:fix/csharp-extension-cache-race
Open

fix(csharp): cache extensions per descriptor#29707
awe123343 wants to merge 1 commit into
protocolbuffers:mainfrom
awe123343:fix/csharp-extension-cache-race

Conversation

@awe123343

@awe123343 awe123343 commented Sep 10, 2026

Copy link
Copy Markdown

Closes #29696.

  • Cache extensions per descriptor; atomically publish completed lists to prevent concurrent corruption.
  • Tests: Added a RED/GREEN UT

Synthetic retained parent/child initialisation, .NET 8 / ARM64:

Cache Median µs/pair Allocated B/pair
Original 30.300 9,543
Per-descriptor 24.799 9,512

@awe123343
awe123343 requested a review from a team as a code owner September 10, 2026 17:05
@awe123343
awe123343 requested review from amanda-tarafa and removed request for a team September 10, 2026 17:05
@google-cla

google-cla Bot commented Sep 10, 2026

Copy link
Copy Markdown

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@awe123343
awe123343 force-pushed the fix/csharp-extension-cache-race branch 2 times, most recently from e47fa97 to d970869 Compare September 10, 2026 17:38
@awe123343
awe123343 force-pushed the fix/csharp-extension-cache-race branch from d970869 to 071aaa8 Compare September 10, 2026 17:49
private readonly Lazy<Dictionary<IDescriptor, DescriptorDeclaration>> declarations;

private static readonly Dictionary<string, List<Extension>> allDependedExtensionsCache = new();
private List<Extension> allDependedExtensionsCache;

@awe123343 awe123343 Sep 10, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I would argue we should not have started with a static map.

  • The depended extensions are derived per descriptor, so the cache should live and die with the descriptor instance.
  • The map breaks that ownership and keeps entries alive even after the descriptor itself is collectable.

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.

[C#] FileDescriptor.GetAllDependedExtensions' extension cache is not thread-safe, causes Dictionary corruption under concurrent FromGeneratedCode calls

1 participant