Skip to content

Use structs for element factory #1844

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

twsouthwick
Copy link
Member

@twsouthwick twsouthwick commented Nov 27, 2024

This helps a bit with performance:

Before

Method Mean Error StdDev Gen0 Gen1 Allocated
Create 41.71 us 0.776 us 0.762 us 6.1035 0.2441 51.52 KB
CreateNoSave 40.32 us 0.729 us 0.780 us 6.1035 0.2441 51.52 KB
ReadFile 847.70 us 16.828 us 29.474 us 29.2969 3.9063 255.22 KB

After

Method Mean Error StdDev Gen0 Gen1 Allocated
Create 40.80 us 0.665 us 0.589 us 6.1035 0.2441 51.52 KB
CreateNoSave 40.50 us 0.561 us 0.438 us 6.1035 0.2441 51.52 KB
ReadFile 830.07 us 14.996 us 22.446 us 29.2969 3.9063 255.22 KB

@twsouthwick twsouthwick changed the title Minimize allocations for creating element lookup Use structs for element factory Nov 27, 2024
@twsouthwick twsouthwick marked this pull request as ready for review April 3, 2025 17:21
@twsouthwick twsouthwick changed the base branch from add-child to main April 3, 2025 17:21
Copy link

There was an error handling pipeline event 018f3cc4-a623-46cc-878d-73bfd5be29a3.

@twsouthwick twsouthwick force-pushed the struct-element-lookup branch from 7f0f8fc to f4289d1 Compare April 3, 2025 17:23
@twsouthwick twsouthwick requested a review from Copilot April 3, 2025 17:23
Copy link

@Copilot 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 refactors the element factory implementation to use structs and file-scoped namespaces for a slight performance boost.

  • Converted ElementFactory from a class to a readonly struct and removed the generic Create() method.
  • Updated ElementFactoryCollection to use a List instead of an array and applied file-scoped namespaces.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
src/DocumentFormat.OpenXml.Framework/Framework/Metadata/ElementFactoryCollection.cs Replaced array with List, updated sorting and binary search, and switched to a file-scoped namespace.
src/DocumentFormat.OpenXml.Framework/Framework/Metadata/ElementFactory.cs Changed from a sealed class to a readonly struct and removed the generic Create() factory method.
Comments suppressed due to low confidence (1)

src/DocumentFormat.OpenXml.Framework/Framework/Metadata/ElementFactoryCollection.cs:18

  • Changing the constructor parameter from IEnumerable to List restricts the accepted input type; ensure that all call sites are updated to pass a List to prevent potential runtime issues.
public ElementFactoryCollection(List<ElementFactory> lookup)

Comment on lines +10 to +11
internal readonly struct ElementFactory(OpenXmlSchemaType type, Func<OpenXmlElement> factory)
{
Copy link
Preview

Copilot AI Apr 3, 2025

Choose a reason for hiding this comment

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

Changing ElementFactory from a class to a struct introduces value semantics that can lead to unintended copying; please verify that all consumers handle these semantics correctly and that removing the generic Create() method is intentional.

Suggested change
internal readonly struct ElementFactory(OpenXmlSchemaType type, Func<OpenXmlElement> factory)
{
internal class ElementFactory
{
private readonly OpenXmlSchemaType type;
private readonly Func<OpenXmlElement> factory;
public ElementFactory(OpenXmlSchemaType type, Func<OpenXmlElement> factory)
{
this.type = type;
this.factory = factory;
}

Copilot uses AI. Check for mistakes.

Copy link

github-actions bot commented Apr 3, 2025

Test Results

0 files   -     56  0 suites   - 56   0s ⏱️ - 57m 13s
0 tests  -  2 050  0 ✅  -  2 047  0 💤  -  3  0 ❌ ±0 
0 runs   - 31 992  0 ✅  - 31 956  0 💤  - 36  0 ❌ ±0 

Results for commit 336ee59. ± Comparison against base commit b127a66.

♻️ This comment has been updated with latest results.

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