perf: cache reflection results to eliminate PropertyInfo regeneration overhead

[baal2000]

Fixes #3112

Problem Summary

Repeated reflection (property enumeration plus attribute lookup) was being performed on every request. In some workloads this can create significant allocation/JIT/GC pressure and contribute to long Gen 2 GC pauses.

Solution

ReflectionCache infrastructure (internal, in Google.Apis.Core)

• Caches request-parameter PropertyInfo and RequestParameterAttribute per request type using ConcurrentDictionary<Type, ...>
• Thread-safe for concurrent access
• Intentionally unbounded cache: keys are request types decorated with RequestParameterAttribute, which are expected to be a finite set for the lifetime of an application

Type-preserving parameter expansion

• ExpandParametersWithTypes() expands IEnumerable / Repeatable<T> values into repeated parameters while preserving RequestParameterType (Query/Path/etc.)
• Centralizes expansion logic to avoid duplicated/diverging implementations

Dual InitParameters methods

• ParameterUtils.InitParameters(...) (public): preserved for backward compatibility; does not expand IEnumerable by default to avoid changing existing serialization semantics
• ParameterUtils.InitParametersWithExpansion(...) (internal): uses ReflectionCache and expands IEnumerable/Repeatable<T> while preserving RequestParameterType
• Used by upload-related code in the Google.Apis assembly via InternalsVisibleTo

Backward compatibility

Google.Apis (generated APIs)	Google.Apis.Core (runtime)	Result
New	Old	❌ Compile error (intentional; prevents silent runtime failures for repeatable parameters)
Old	New	✅ Works (InitParameters unchanged)
New	New	✅ Works with caching + repeatable parameter expansion

Testing

Added 16 tests covering:

• ReflectionCache behavior (property/attribute caching and reuse)
• IEnumerable / Repeatable<T> expansion
• Type conversion (DateTime/RFC3339, Enum/StringValue, Boolean/lowercase)
• Null elements in enumerables (asserted dynamically against Utilities.ConvertToString(null))

All new types are internal; no public API surface changes.

[google-cla]

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.

[gemini-code-assist]

Summary of Changes

Hello @baal2000, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request significantly improves the performance of API request parameter processing by introducing a robust caching mechanism for reflection results. Previously, reflection operations were performed repeatedly, leading to potential performance bottlenecks. The new ReflectionCache efficiently stores property and attribute information, reducing allocation and GC pressure. Additionally, the PR centralizes and refines the logic for expanding enumerable parameters, ensuring consistent and correct serialization. These internal changes enhance the library's efficiency without altering its public API surface, providing a more optimized experience for users.

Highlights

• Performance Optimization: Implemented a ReflectionCache to store PropertyInfo and RequestParameterAttribute results, eliminating repeated reflection overhead for API request parameter processing.
• Centralized Parameter Expansion: Introduced ExpandParametersWithTypes to standardize the expansion of IEnumerable and Repeatable values into multiple parameters, preserving their RequestParameterType.
• Dual Parameter Initialization Methods: Created InitParametersWithExpansion (internal) for new code paths that benefit from caching and expansion, while preserving the existing InitParameters (public) for backward compatibility.
• Internal API Enhancements: Added new internal structs (ParameterValue, TypedParameter, PropertyWithAttribute) and the ReflectionCache class to support the new parameter processing logic.
• Extensive Testing: Included 16 new unit tests to validate the ReflectionCache behavior, enumerable expansion, various type conversions (DateTime, Enum, Boolean), and null handling.
• Controlled Access: Utilized InternalsVisibleTo to allow the Google.Apis assembly to access the new internal functionality in Google.Apis.Core.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[gemini-code-assist]

Code Review

This is an excellent pull request that addresses a significant performance issue by caching reflection results. The introduction of ReflectionCache is well-implemented using a ConcurrentDictionary for thread safety. I appreciate the refactoring to centralize parameter expansion logic in ParameterUtils, which improves code reuse and maintainability across ParameterCollection and ResumableUpload. The changes are supported by a comprehensive set of new tests that cover caching, parameter expansion, and edge cases. Overall, this is a high-quality contribution. I have one minor suggestion to simplify a LINQ expression for improved readability.

[amanda-tarafa]

Thanks for this. It might still be a few days before we can get to looking into this issue and possibly reviewing the PR as we have other priorities at the moment.

Also, you'd need to sign the CLA if you wish to contribute.

[baal2000]

@amanda-tarafa I am still working on submitting a CLA for my employer: not a very straightforward process TBH.

Update

this has been resolved, I am now covered by an CLA.

[amanda-tarafa]

@amanda-tarafa I am still working on submitting a CLA for my employer: not a very straightforward process TBH.

Update

this has been resolved, I am now covered by an CLA.

Thanks, it might still be a few days until we can get to this.

[baal2000]

@amanda-tarafa Is there an ETA for reviewing this?

The slow finalizer issue has been resolved in our environment as a fixed forked library. It is a cumbersome solution though when it comes to overriding a public higher level package dependency with a private lower level one. We would like get back to the public as soon as possible.
Thank you.

[amanda-tarafa]

Thanks, I'll try to look this week, but I can't make any promises.

[baal2000]

Hi @amanda-tarafa , friendly follow-up — it's been about a week since you mentioned you'd try to look at this.

I understand the team has priorities, but I want to flag the severity: this bug causes 30-60 second application freezes in production due to uncached reflection flooding the finalizer thread with DestroyScout cleanup. It affects any application making repeated API calls, which is... most of them.

We're currently running a private fork to work around it, which is not ideal for either side.

Regarding PR size — the 692 added lines may look large, but ~370 of those are tests (which the affected code previously lacked). The actual fix is a ~62-line ReflectionCache class plus straightforward refactoring of the two call sites. Thanks for your time.

[amanda-tarafa]

Again, I'm sorry, I will try to look at this as soon as possible but it's not high priority for us. I'm less concerned about the PR size and more about understanding the problem and the proposed solution. It's definitely on my radar.

[baal2000]

The issue has a full root cause analysis with traces: #3112. That should cover the understanding part. Let me know if you have questions after reading it.

[amanda-tarafa]

We wouldn't do this. We don't want to force version dependencies based on internal surface. Either we make some of this public or the cache is duplicated in both packages, which would still solve your issue.

[baal2000]

I would prefer making the classes public. While code duplication avoids public API commitment, the drawbacks outweigh that benefit:

• Double memory usage (~84 KB vs ~42 KB)
• Code duplication across assemblies
• Ongoing maintenance burden

[baal2000]

Previously-internal types (ReflectionCache, PropertyWithAttribute, InitParametersWithExpansion) are now public, which allowed removing the InternalsVisibleTo("Google.Apis", …) attribute.

[amanda-tarafa]

This worries me, it's definitely finite, and eventually stable, but I'm not sure it' small enough in all cases that it wouldn't cause issues. For instance, imagine a CI that tests all the 400+ libraries, and makes a request for each of the methods on each of the libraries. Plus, this adds a bottleneck (you need the concurrent dictionary) that will affect 100% of requests, and even if your app makes a single type of request, on high loads latency may increase.

These are all corner cases, but yours is also a corner case (no one has encountered this issue in 10+ years) so I'm wary of swapping the implementation in ways that may create other problems.

A solution where calling code can select beforehand whether to use a cache or not is preferable. The default implementation would continue to be the one we currently have. I will think about how that looks like.

[baal2000]

Let me know what your approach should look like and I am going to follow. We probably need to resolve this before addressing that other concern by duplicating the cache or making it public.

[baal2000]

I'd suggest making the cache opt-in through a public static property that controls whether caching is enabled:

public static class ReflectionCacheSettings
{
    /// <summary>
    /// Gets or sets whether to enable reflection caching for request parameters.
    /// Default is false. Set to true early in application startup to improve performance
    /// when making many requests with the same request types.
    /// </summary>
    public static bool EnableReflectionCache { get; set; } = false;
}

internal static class ReflectionCache
{
   ... 
    internal static PropertyWithAttribute[] GetRequestParameterProperties(Type type)
    {
        if (ReflectionCacheSettings.EnableReflectionCache)
        {
            return RequestParameterPropertiesCache.GetOrAdd(type, ComputeProperties);
        }
        
        // Default behavior: no caching
        return ComputeProperties(type);
    }

    private static PropertyWithAttribute[] ComputeProperties(Type type)
    {
        return type.GetProperties(BindingFlags.Instance | BindingFlags.Public)
            .Select(prop => new PropertyWithAttribute(prop, prop.GetCustomAttribute<RequestParameterAttribute>(inherit: false)))
            .Where(pwa => pwa.Attribute != null)
            .ToArray();
    }
}

For the following reasons:

• Matches the requirement of the default behavior left unchanged, opt-in mechanism
• Transparent and simple for users: one line of code at startup to enable
• No public API surface changes: the ReflectionCacheSettings class would be new and public, but it doesn't change any existing APIs

[baal2000]

I've implemented the opt-in approach. The cache is now disabled by default and enabled with:

ApplicationContext.EnableReflectionCache = true;

rather than introducing a new ReflectionCacheSettings class, A property in the existing ApplicationContext is consistent with the library's established pattern for startup configuration, avoids new public API surface and is easier to obsolete later if caching becomes the default.

A couple of clarifications:

• On the CI scenario: The cache is per-process and keyed by Type. Only properties decorated with RequestParameterAttribute are cached — not all properties on a type, and not all types in an assembly. Even a single process exercising all 400+ libraries would accumulate a few thousand entries in the worst case (trivial memory). When CI runs each library in a separate process, each process holds only the request types it tests — a much smaller subset — and the memory is reclaimed when the process exits. Without the cache, the runtime already creates transient PropertyInfo arrays and IL stubs per request that are temporarily stored in an internal weakly-referenced cache — which itself adds GC pressure and is the root cause of the DestroyScout finalizer delays under the linked issue.
• On ConcurrentDictionary: GetOrAdd on a hit is a Volatile.Read — no lock taken, ~20–50 ns. The un-cached path (GetProperties + GetCustomAttribute per request) costs more than that. This is the standard pattern used by ASP.NET Core, EF Core, and System.Text.Json for reflection caching.

[amanda-tarafa]

Just to say, this is on my radar still. I'll take a look this week, but also, we are considering an alternative where we generate this information so it's statically included on each request type. I believe that would be the best solution but also the most involved in terms of work and release complexity. Can I ask which libraries exactly are you using, that is, for which APIs.

[baal2000]

Per #3112: Google.Apis.Upload.ResumableUpload.SetAllPropertyValues and Google.Apis.Requests.Parameters.ParameterUtils.IterateParameters

Higher level APIs: Google.Apis.Requests and Google.Cloud.Storage.V1

[amanda-tarafa]

Google.Cloud.Storage.V1

I was looking for this, thanks.

[baal2000]

Just to say, this is on my radar still. I'll take a look this week, but also, we are considering an alternative where we generate this information so it's statically included on each request type. I believe that would be the best solution but also the most involved in terms of work and release complexity. Can I ask which libraries exactly are you using, that is, for which APIs.

Could we apply the narrowly scoped reflection cache patch and then think of larger refactoring?