Skip to content

Conversation

@TimothyMakkison
Copy link
Contributor

Most attributes types are unique in ToAttributeDictionary so we default to a single array and then switch to a List<Attribute> when a type has more that one attributes. Lists with 1 element added are 32 bytes with an additional 4 element, 56 byte Attribute[].

Might be able to pre size the dictionary by using an average of all previous final dictionaries, It might be as easy as a static long Accumulator and long Count and the average is inital capacity for Dictionary

  • Were you aware that the attributes in Playground.AssemblyInfo were appearing in every tests AttributeFactory
    • I assume all global assemblies are added to each tests AttributeFactory, Is this in case the attribute is needed in the test or some AOT thing?
  • Is it necessary to wrap the dictionary in ReadOnlyDictionary?

Before

image

After

image

@thomhurst
Copy link
Owner

Summary

Performance optimization to reduce allocations in attribute dictionary creation by using a single-element array instead of a List for the common case of one attribute per type.

Critical Issues

None found ✅

Suggestions

Minor typo in comment (line 29):
addtional should be additional

Consider adding unit tests:
While not blocking, ToAttributeDictionary has no direct unit tests. Consider adding tests to verify:

  • Single attribute per type (array path)
  • Multiple attributes of same type (list path)
  • Mixed scenario (some types with 1, others with multiple)
  • Empty array input

This would help document the behavior and catch any future regressions.

Performance validation:
The PR shows nice memory reduction in the screenshots. Consider documenting the typical case (e.g., "most attribute types appear once per test, so this saves ~32 bytes per unique attribute type") to help future maintainers understand the optimization.

Verdict

APPROVE - No critical issues

This is a solid performance improvement for a hot path (test discovery/building). The logic is correct:

  • First attribute → single-element array (lower allocation)
  • Second attribute → convert to List
  • Subsequent attributes → append to List

The implementation maintains backward compatibility since both Attribute[] and List<Attribute> implement IReadOnlyList<Attribute>.

Great work on the performance optimization! 🚀

@thomhurst thomhurst merged commit 4024a4b into thomhurst:main Jan 11, 2026
8 of 10 checks passed
@TimothyMakkison
Copy link
Contributor Author

TimothyMakkison commented Jan 12, 2026

@thomhurst Do you have any thoughts about removing ReadOnlyDictionary usage or doing some clever inital capacity on the Dictionary? Resizing the dictionary costs around 14MB although the test project is unique in how many attributes are used.

This was referenced Jan 12, 2026
This was referenced Jan 12, 2026
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.

2 participants