Skip to content
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

Add IsChainDiscarded thread-static field. #420

Closed
Tracked by #427
fiseni opened this issue Nov 9, 2024 · 0 comments · Fixed by #442
Closed
Tracked by #427

Add IsChainDiscarded thread-static field. #420

fiseni opened this issue Nov 9, 2024 · 0 comments · Fixed by #442
Assignees
Milestone

Comments

@fiseni
Copy link
Collaborator

fiseni commented Nov 9, 2024

All builder extension methods have an overload that accepts a bool condition parameter. If the passed condition is false the expression/value is not added to the specification state. This is convenient in many scenarios and is similar to WhereIf types of LINQ extensions.

However, having this condition complicates the case where we have sub-chains (e.g. Include/ThenInclude, OrderBy/ThenBy, etc). If any parent in the sub-chain has a false condition, we need to dismiss the rest of the sub-chain regardless of their conditions. To implement this we had to define the OrderedSpecificationBuilder, IncludableSpecificationBuilder and CacheSpecificationBuilder sub-classes to SpecificationBuilder, with the sole purpose of "remembering" the previous condition of the sub-chain. The root parent of the chain returns new instances of these builders which act as a container for the sub-chain. This is a bit wasteful, we're spending 32 bytes per instance.

Considering that the constructor will be executed in a single thread (and generally specifications are not intended to be thread-safe), then we may introduce a ThreadStatic field in the specification itself that indicates whether the chain is discarded or not. It will be utilized only during the building stage for the sub-chains. Once the state is built, we don't care about it anymore. The initial value is not important since the value is always initialized by the root of the sub-chain. Therefore, we don't need ThreadLocal (it's more expensive).

We still need the IncludableSpecificationBuilder to remember the generic parameter of the parent, but we won't need the OrderedSpecificationBuilder and CacheSpecificationBuilder instances. This will save us 8 bytes per include builder, and 32 bytes per order/cache builder instances.

Breaking changes:

  • No breaking changes for common usage of the library.
  • Folks that have custom extensions to Include/Order/Cache should adapt their implementations.
@fiseni fiseni mentioned this issue Nov 9, 2024
21 tasks
@fiseni fiseni self-assigned this Dec 20, 2024
@fiseni fiseni added this to the 9.0 milestone Mar 10, 2025
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 a pull request may close this issue.

1 participant