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

ToConcreteType Update #751

Conversation

dwcullop
Copy link
Member

@dwcullop dwcullop commented Nov 7, 2023

Description

This PR continues to effort to replace IChangeSet<TObject, TKey> with ChangeSet<TObject, TKey> in order to avoid allocations that can take place when enumerating with the former instead of the latter.

  • Updates ToConcreteType to throw an exception if a non-ChangeSet implementation of IChangeSet is encountered
  • Changes anywhere enumeration would happen using IChangeSet to invoke ToConcreteType so that it is done with ChangeSet instead.
  • When possible, IChangeSet was completely replaced with ChangeSet in order to ensure more consistent use and reduce the number of places that ToConcreteType needs to be invoked explicitly

Copy link

codecov bot commented Nov 8, 2023

Codecov Report

Merging #751 (de49b48) into main (269828b) will increase coverage by 0.00%.
The diff coverage is 85.71%.

@@           Coverage Diff           @@
##             main     #751   +/-   ##
=======================================
  Coverage   64.74%   64.74%           
=======================================
  Files         226      226           
  Lines       11464    11459    -5     
  Branches     2335     2334    -1     
=======================================
- Hits         7422     7419    -3     
+ Misses       3085     3083    -2     
  Partials      957      957           
Files Coverage Δ
src/DynamicData/Aggregation/AggregateEnumerator.cs 67.64% <100.00%> (ø)
src/DynamicData/Binding/BindingListAdaptor.cs 76.19% <100.00%> (ø)
...DynamicData/Binding/ObservableCollectionAdaptor.cs 83.72% <100.00%> (+4.55%) ⬆️
src/DynamicData/Cache/Internal/AbstractFilter.cs 53.33% <100.00%> (ø)
src/DynamicData/Cache/Internal/Cache.cs 69.69% <100.00%> (ø)
src/DynamicData/Cache/Internal/Combiner.cs 89.65% <100.00%> (ø)
src/DynamicData/Cache/Internal/DisposeMany.cs 95.12% <100.00%> (ø)
src/DynamicData/Cache/Internal/IndexCalculator.cs 81.17% <100.00%> (ø)
.../DynamicData/Cache/Internal/RemoveKeyEnumerator.cs 72.72% <100.00%> (ø)
src/DynamicData/Cache/Internal/TransformMany.cs 95.27% <100.00%> (ø)
... and 4 more

Copy link
Collaborator

@RolandPheasant RolandPheasant left a comment

Choose a reason for hiding this comment

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

Thanks.

@@ -49,7 +49,7 @@ public DisposeMany(IObservable<IChangeSet<TObject, TKey>> source, Action<TObject

private void RegisterForRemoval(IChangeSet<TObject, TKey> changes, Cache<TObject, TKey> cache)
{
changes.ForEach(
changes.ToConcreteType().ForEach(
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this should be changed to the traditional foreach() loop instead of the ForEach extension.

I wrote the ForEach extension, but it was misguided as it also allocates. However, looking at the usages:

image

That should be changed in a distinct PR

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I can do a distinct PR for that. I'd like to understand more about why it allocates because that is news to me. As a fan of ForEach, maybe it can be updated to avoid the allocation instead of needing to be removed.

@RolandPheasant RolandPheasant merged commit 6ac7026 into reactivemarbles:main Nov 9, 2023
3 checks passed
@dwcullop dwcullop deleted the feature/update-concrete-changesets branch November 23, 2023 00:59
Copy link

github-actions bot commented Dec 7, 2023

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 7, 2023
@dwcullop dwcullop self-assigned this Dec 20, 2023
@dwcullop dwcullop added the Housekeeping Pull requests for minor code maintenance issues label Dec 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Housekeeping Pull requests for minor code maintenance issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants