Re-design for ToObservableChangeSet()
#771
Conversation
|
btw @JakenVeina you are a maintainer now. So you should be able to create branches directly on this repo. that will allow the test coverage to work. I need to work out a way of having the test coverage to work on forks. |
Duly noted, thanks. Would running coverage on dev branches just be for convenience, or would it actually support the main branch or PRs, in some fashion? Like, I believe I saw recently that coverage runs only periodically on the main branch, and results are cached for comparison against PRs, and that's how the bot reports changes in coverage. Is that accurate? I have my own tooling in VS for running coverage locally, during development, before submitting PRs. |
|
Mostly would allow others to see your code coverage. Swapping from codecov to sonarcloud, sonarcloud uses a secured token to upload results for the coverage. Codecov would upload without the token but the results were all over the place. |
Looks like the token is actually missing?
That same error is the one that was failing in the README PR yesterday. |
|
Only missing when running on a fork. If you allowed GitHub action secrets to run on forks it'd be a massive security flaw since people can get access to your tokens. |
|
Ohhhhh, okay, I see what you mean. Somehow I thought it wouldn't count as running on a fork, within the PR build. |
|
I'm trying a new feature that runs results locally after a fork build has finished. Will see how it works. |
| _source = Observable.Create<IEnumerable<TObject>>(observer => | ||
| { | ||
| // Reusable buffer, to avoid allocating per-item | ||
| var buffer = new TObject[1]; |
| private bool _hasSourceCompleted; | ||
| private ScheduledExpiration? _scheduledExpiration; | ||
|
|
||
| private struct ItemState |
There was a problem hiding this comment.
Shouldn't a struct always be immutable, and preferably read only? It would be interesting to change these as such and run the benchmarks again,
There was a problem hiding this comment.
Also a structure should implement equality especially if contained within a collection / dictionary. Otherwise reflection would be used for operations that check equality. Records would come to the rescue here, but alas the 46 framework target prevents that.
My main concern is with ItemState being contained in a dictionary.
Or alternatively, can you be sure that my concerns are ill founded in this case?
There was a problem hiding this comment.
I do generally use readonly structs, and definitely records, but since the tests need to run on at least .NET 6, I wasn't able to use all the features I usually do, like required and init. Since I made a PR for those, I was gonna do another pass over this, anyway, depending on who gets merged first.
The pitfall with equality of structs is when using them as a TKey within keyed collections, E.G. Dictionary, usually in the interest of creating a composite key. Like you say, they'll work but really poorly.
As a rudimentary proof, you can do what I just did and toss IEquatable<T> on all those structs and override .Equals() and .GetHashCode() just to call the base methods. Breakpoints show that none of those methods ever get called in any of the tests.
| observer: observer, | ||
| scheduler: _scheduler)); | ||
|
|
||
| private readonly Func<TObject, TimeSpan?>? _expireAfter; |
There was a problem hiding this comment.
I'd prefer the fields before the constructor. That way I can see them without have to scroll through a long section of code to see the local state. The same applies to the Subscription class and the equivalent list implementation.
There was a problem hiding this comment.
I will create a PR to tighten up the requirements in the editorConfig file, hopefully this will aid the code to follow the expected format.
There was a problem hiding this comment.
Me personally, I feel the exact opposite. State is the least-likely thing I wanna look at when inspecting a class, I'm probably more interested in seeing its public surface, at a glance.
I'm happy to follow whatever style rules we want to have, though, so long as they're either documented, or enforced by tooling. I'll swap this up.
| private bool _hasSourceCompleted; | ||
| private ScheduledExpiration? _scheduledExpiration; | ||
|
|
||
| private struct ItemState |
There was a problem hiding this comment.
Also a structure should implement equality especially if contained within a collection / dictionary. Otherwise reflection would be used for operations that check equality. Records would come to the rescue here, but alas the 46 framework target prevents that.
My main concern is with ItemState being contained in a dictionary.
Or alternatively, can you be sure that my concerns are ill founded in this case?
| @@ -77,7 +77,7 @@ private IObservable<IChangeSet<TDestination>> RunImpl() | |||
| case ListChangeReason.Add: | |||
| { | |||
| var change = item.Item; | |||
| if (change.CurrentIndex < 0 | change.CurrentIndex >= transformed.Count) | |||
| if (change.CurrentIndex < 0 || change.CurrentIndex >= transformed.Count) | |||
There was a problem hiding this comment.
I noticed this the other day, Thanks for fixing,
There was a problem hiding this comment.
I spotted that one too, and fixed it along with some formatting.
There was a problem hiding this comment.
For a moment, I thought that was something I'd written.
ChrisPulman
dismissed
their stale review
Further Discussions underway, awaiting outcome.
Use 'record struct' instead of struct if immutable. They include the ability to mutate into a new immutable value with the 'with' operator. |
|
Not to mention records and 'record struct' are much more concise by default. You can use them just like normal structs though. Eg public record MyRecord(string Hello); And public record MyRecord Are the same. The with operator allows you to modify the record/record struct with just selected properties changed in a new immutable value. var newValue = oldValue with { MyPropery = 123 }; Will create a new value keeping existing property values and only MyProperty as 123 |
|
@glennawatson regarding records, I have been using them for years. However what I did not know until our conversation on slack was that it only required a polyfill to make them work on Net462. @JakenVeina has already enabled it here #772, which I just merged, We're in a happy record enabled space now |
I'm actually not sure if records are good-to-go now, they're quite a bit more involved than just the |
There was a problem hiding this comment.
@JakenVeina your call with checking out the structures / records etc. However if you want to merge I've approved, so feel free to do so.
Also I can confirm that we can use records now, I added one into my PR yesterday and it works well.
|
I wouldn't mind using this PR to see if I can get that coverage tester working on this branch before merge if possible |
|
@RolandPheasant I actually did that yesterday, and then apparently never checked it in. @glennawatson go for it. |
…nd lists, for better independence, proper error handling, and improved performance. Resolves reactivemarbles#635.
|
@glennawatson I went ahead and created a dedicated dummy branch for testing the code coverage workflow, so this PR can move forward. |
src\DynamicData.Tests\Cache\TransformFixtureParallel.cs line 95 - Test Needs fixing or updated Functionality needs checking
* Strengthen EditorConfig to help enforce coding standards Update code to match coding standards * Update ObservableSpy.cs Fix release version * Further code fixes after SonarCloud tests * Fix issues picked up by SonarCloud * Update new code * introduce ThrowArgumentNullExceptionIfNull Reduce code bloat by using extension method for ArgumentNullException's. This was chosen due to current target frameworks. If in the future netstandard2.1 + compliant frameworks are used we can switch methods. * Update API checks * Update remaining ArgumentNullException to use extension method * Fix code after merge * Update code after merge * The PR #771 introduced inconsistent results src\DynamicData.Tests\Cache\TransformFixtureParallel.cs line 95 - Test Needs fixing or updated Functionality needs checking * Skip removed to ensure fix is applied before merge Test - src\DynamicData.Tests\Cache\TransformFixtureParallel.cs SameKeyChanges on line 95 needs attention * Fixed intermittent test failures introduced by 6395e7d --------- Co-authored-by: JakenVeina <[email protected]>
|
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. |
Re-designed the .ToObservableChangeSet() operator for both caches and lists, for better independence, proper error handling, and improved performance. Resolves #635.
I worry I went a little overboard with the complexity, so I would definitely appreciate a couple reviews on this.
The good news is the complexity isn't without benefit, according to the benchmarks I wrote previously:
ToObservableChangeSet_Cache (Before)
ToObservableChangeSet_Cache (After)
ToObservableChangeSet_List (Before)
ToObservableChangeSet_List (After)
TL;DR, this shows a solid 50% reduction in runtime and memory usage, for the list operator, and 70% reduction in runtime with a 50% reduction in memory usage, for the cache operator.
Although, the values in the "Allocations" column are suspicious, to me. It says we're allocating 1.39kB for constructing a stream that we then never emit any items into? I dunno how that possibly makes sense, but I'm assuming the numbers are still relevant, in comparison to what they were before.