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.

Sign up for GitHub

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

ExpireAfter Redesign #868

Merged
merged 3 commits into from
Feb 26, 2024
Merged

ExpireAfter Redesign #868

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
89dd436
Fixed potential infinite-looping in the "SchedulerIsInaccurate" Expir…
JakenVeina Feb 25, 2024
5b14497
Enhanced benchmarks for ExpireAfter operators to improve code coverag…
JakenVeina Feb 25, 2024
7d2db60
Re-designed ExpireAfter operators, from scratch, eliminating a variet…
JakenVeina Feb 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions .editorconfig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -370,7 +370,7 @@ dotnet_diagnostic.SA1136.severity = error
dotnet_diagnostic.SA1137.severity = error
dotnet_diagnostic.SA1139.severity = error
dotnet_diagnostic.SA1200.severity = none
dotnet_diagnostic.SA1201.severity = error
dotnet_diagnostic.SA1201.severity = none
dotnet_diagnostic.SA1202.severity = error
dotnet_diagnostic.SA1203.severity = error
dotnet_diagnostic.SA1204.severity = error
Expand Down Expand Up @@ -424,10 +424,10 @@ dotnet_diagnostic.SA1508.severity = error
dotnet_diagnostic.SA1509.severity = error
dotnet_diagnostic.SA1510.severity = error
dotnet_diagnostic.SA1511.severity = error
dotnet_diagnostic.SA1512.severity = error
Copy link
Member

Choose a reason for hiding this comment

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

What are these? If they're important, don't just turn them off for the entire project. Add an exception for each case so that people still try to avoid them if they can.

Copy link
Member

Choose a reason for hiding this comment

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

SA1512 is new lines after a comment. You can use a SuppressMessageAttribute to disable where its appropriate but generally you don't want new lines after comments anyway.

Copy link
Collaborator Author

@JakenVeina JakenVeina Feb 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Let's see...

RCS1085 is for always using auto-implemented properties, when possible. Violations are here, here, and here. In each of these, we have a complex object where there's far more value in being able to see all of its persistent state (I.E. private fields) in one place, than in reducing the file size by 2 lines. This is a common-enough scenario, in my mind, that it warrants disabling the rule.

SA1201 says "a property should not follow a method", but it ignores the fact that it's a protected property, and I've always encountered standard practice to be that protected instance members don't come before public instance members. Violations are here, here, and here.

SA1512 says "single-line comments should not be followed by blank line", with violations here, here, here, and here. Intuitively, it's standard practice to "attach" comments to the lines of code that they apply to, which I imagine is the origin of this rule, but all of these are examples of comments that DON'T apply to a specific line of code. They are actually commenting on the LACK of lines of code that one might expect to be here. Since the rule applies to a command, and you can't disable rules upon individual comments, I think the rule should be disabled entirely.

SA1513 says "Closing brace should be followed by blank line", and is violated here and here. In these cases, I'm attaching a line of code to the end of a loop, in an effort to indicate that it fundamentally is a finalization action for that loop. Not a big deal, I probably would have just fixed these two if I hadn't already disabled the rule for the 11 other violations (I'll just link this one, they're all the same), which consist of having a break; immediately following a bracketed case block. In my mind the closing bracket itself serves the purpose that a blank line normally would, for a multi-line case block, and inserting an additional blank line here makes it subtly harder to read.

SA1515 says "Single line comments should be preceeded by blank line", and is violated here, here, here, and here. Also here and here, but those are within the .Tests project. In all of these cases, we're commenting on a single line withing a multi-line action, where the comment is too long to put at the end of the line, and where the multi-line action would normally have no blank lines in it. These are all valid in my mind, because of the implicit rule that comments should be directly attached to the thing they're commenting on, not potentially many lines away from it, and with so many valid use-cases being flagged, this seems like a rule that should be disabled.

Copy link
Collaborator

@RolandPheasant RolandPheasant Feb 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I got to be honest, I really dislike style enforcement.

I have been tempted to turn it off entirely in DD as it drives me insane how slow it is to run and how it tries to force me to comment code which I have not even written yet!!!! I always end up turning it off, do my code, then turn it on to see what it complains about. The worst thing is that it also slows my development down.

We're grown ups and surely we should be allowed to express our own style. There are a few things that are important like comments, but surely we could enforce that as part of the PR process.

In a better version of of the net eco-system we'd all be using automatic linting.

Perhaps we should get rid of it completely - thoughts?

Copy link
Collaborator Author

@JakenVeina JakenVeina Feb 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I got to be honest, I really dislike style enforcement.

I fully agree. I think style-enforcement is kind of a fundamentally-flawed concept. Good styling is about promoting human-readability of code, computers are fundamentally ill-equipped to understand what is "human readable", across the board. At least, I have yet to see any system that implements rules granular enough to promote readability, instead of harm it.

Perhaps we should get rid of it completely - thoughts?

I wouldn't lose any sleep, personally. But on a large collaborative project, logically, I know there's a bigger picture than just my opinions, and it would probably be a mistake to remove completely. I think we can focus on de-tuning it, and reducing the impact it has on the developer experience. There are some rules that are nearly-universal, like member ordering and naming, and we gain value by having those. I would point to whitespace rules in particular as usually being best left to a judgement call of the writer.

I always end up turning it off, do my code, then turn it on to see what it complains about.

What's the quick-and-easy way to do this? I've just been manually tossing out #pragma warning disable as they pop up, and it's quite annoying.

we'd all be using automatic linting.

So, wait, what is linting if not this? Like, automatically fixing violations, instead of just reporting them?

dotnet_diagnostic.SA1513.severity = error
dotnet_diagnostic.SA1512.severity = none
dotnet_diagnostic.SA1513.severity = none
dotnet_diagnostic.SA1514.severity = error
dotnet_diagnostic.SA1515.severity = error
dotnet_diagnostic.SA1515.severity = none
dotnet_diagnostic.SA1516.severity = error
dotnet_diagnostic.SA1517.severity = error
dotnet_diagnostic.SA1518.severity = error
Expand Down Expand Up @@ -503,7 +503,7 @@ dotnet_diagnostic.RCS1058.severity=warning
dotnet_diagnostic.RCS1068.severity=warning
dotnet_diagnostic.RCS1073.severity=warning
dotnet_diagnostic.RCS1084.severity=error
dotnet_diagnostic.RCS1085.severity=error
dotnet_diagnostic.RCS1085.severity=none
dotnet_diagnostic.RCS1105.severity=error
dotnet_diagnostic.RCS1112.severity=error
dotnet_diagnostic.RCS1128.severity=error
Expand Down Expand Up @@ -547,4 +547,4 @@ end_of_line = lf
[*.{cmd, bat}]
end_of_line = crlf

vsspell_dictionary_languages = en-US
vsspell_dictionary_languages = en-US
132 changes: 102 additions & 30 deletions src/DynamicData.Benchmarks/Cache/ExpireAfter_Cache_ForSource.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,62 +1,134 @@
using System;
using System.Collections.Generic;
using System.Collections.Immutable;
using System.Linq;

using BenchmarkDotNet.Attributes;

using Bogus;

namespace DynamicData.Benchmarks.Cache;

[MemoryDiagnoser]
[MarkdownExporterAttribute.GitHub]
public class ExpireAfter_Cache_ForSource
{
public ExpireAfter_Cache_ForSource()
=> _items = Enumerable
.Range(1, 1_000)
{
// Not exercising Moved, since SourceCache<> doesn't support it.
_changeReasons =
[
ChangeReason.Add,
ChangeReason.Refresh,
ChangeReason.Remove,
ChangeReason.Update
];

// Weights are chosen to make the cache size likely to grow over time,
// exerting more pressure on the system the longer the benchmark runs.
// Also, to prevent bogus operations (E.G. you can't remove an item from an empty cache).
_changeReasonWeightsWhenCountIs0 =
[
1f, // Add
0f, // Refresh
0f, // Remove
0f // Update
];

_changeReasonWeightsOtherwise =
[
0.30f, // Add
0.25f, // Refresh
0.20f, // Remove
0.25f // Update
];

_editCount = 5_000;
_maxChangeCount = 20;

var randomizer = new Randomizer(1234567);

var minItemLifetime = TimeSpan.FromMilliseconds(1);
var maxItemLifetime = TimeSpan.FromMilliseconds(10);
_items = Enumerable.Range(1, _editCount * _maxChangeCount)
.Select(id => new Item()
{
Id = id
Id = id,
Lifetime = randomizer.Bool()
? TimeSpan.FromTicks(randomizer.Long(minItemLifetime.Ticks, maxItemLifetime.Ticks))
: null
})
.ToArray();
.ToImmutableArray();
}

[Benchmark]
[Arguments(1, 0)]
[Arguments(1, 1)]
[Arguments(10, 0)]
[Arguments(10, 1)]
[Arguments(10, 10)]
[Arguments(100, 0)]
[Arguments(100, 1)]
[Arguments(100, 10)]
[Arguments(100, 100)]
[Arguments(1_000, 0)]
[Arguments(1_000, 1)]
[Arguments(1_000, 10)]
[Arguments(1_000, 100)]
[Arguments(1_000, 1_000)]
public void AddsRemovesAndFinalization(int addCount, int removeCount)
public void RandomizedEditsAndExpirations()
{
using var source = new SourceCache<Item, int>(static item => item.Id);

using var subscription = source
.ExpireAfter(
timeSelector: static _ => TimeSpan.FromMinutes(60),
interval: null)
timeSelector: static item => item.Lifetime,
interval: null)
.Subscribe();

for (var i = 0; i < addCount; ++i)
source.AddOrUpdate(_items[i]);

for (var i = 0; i < removeCount; ++i)
source.RemoveKey(_items[i].Id);
PerformRandomizedEdits(source);

subscription.Dispose();
}

private readonly IReadOnlyList<Item> _items;
private void PerformRandomizedEdits(SourceCache<Item, int> source)
{
var randomizer = new Randomizer(1234567);

var nextItemIndex = 0;

private sealed class Item
for (var i = 0; i < _editCount; ++i)
{
source.Edit(updater =>
{
var changeCount = randomizer.Int(1, _maxChangeCount);
for (var i = 0; i < changeCount; ++i)
{
var changeReason = randomizer.WeightedRandom(_changeReasons, updater.Count switch
{
0 => _changeReasonWeightsWhenCountIs0,
_ => _changeReasonWeightsOtherwise
});

switch (changeReason)
{
case ChangeReason.Add:
updater.AddOrUpdate(_items[nextItemIndex++]);
break;

case ChangeReason.Refresh:
updater.Refresh(updater.Keys.ElementAt(randomizer.Int(0, updater.Count - 1)));
break;

case ChangeReason.Remove:
updater.RemoveKey(updater.Keys.ElementAt(randomizer.Int(0, updater.Count - 1)));
break;

case ChangeReason.Update:
updater.AddOrUpdate(updater.Items.ElementAt(randomizer.Int(0, updater.Count - 1)));
break;
}
}
});
}
}

private readonly ChangeReason[] _changeReasons;
private readonly float[] _changeReasonWeightsOtherwise;
private readonly float[] _changeReasonWeightsWhenCountIs0;
private readonly int _editCount;
private readonly ImmutableArray<Item> _items;
private readonly int _maxChangeCount;

private sealed record Item
{
public int Id { get; init; }
public required int Id { get; init; }

public required TimeSpan? Lifetime { get; init; }
}
}
151 changes: 101 additions & 50 deletions src/DynamicData.Benchmarks/Cache/ExpireAfter_Cache_ForStream.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,12 @@
using System;
using System.Collections.Generic;
using System.Collections.Immutable;
using System.Linq;
using System.Reactive.Subjects;

using BenchmarkDotNet.Attributes;

using Bogus;

namespace DynamicData.Benchmarks.Cache;

[MemoryDiagnoser]
Expand All @@ -12,76 +15,124 @@ public class ExpireAfter_Cache_ForStream
{
public ExpireAfter_Cache_ForStream()
{
var additions = new List<IChangeSet<Item, int>>(capacity: 1_000);
var removals = new List<IChangeSet<Item, int>>(capacity: 1_000);
// Not exercising Moved, since ChangeAwareCache<> doesn't support it, and I'm too lazy to implement it by hand.
var changeReasons = new[]
{
ChangeReason.Add,
ChangeReason.Refresh,
ChangeReason.Remove,
ChangeReason.Update
};

// Weights are chosen to make the cache size likely to grow over time,
// exerting more pressure on the system the longer the benchmark runs.
// Also, to prevent bogus operations (E.G. you can't remove an item from an empty cache).
var changeReasonWeightsWhenCountIs0 = new[]
{
1f, // Add
0f, // Refresh
0f, // Remove
0f // Update
};

for (var id = 1; id <= 1_000; ++id)
var changeReasonWeightsOtherwise = new[]
{
var item = new Item()
{
Id = id
};
0.30f, // Add
0.25f, // Refresh
0.20f, // Remove
0.25f // Update
};

additions.Add(new ChangeSet<Item, int>(capacity: 1)
{
new(reason: ChangeReason.Add,
key: id,
current: item)
});
var maxChangeCount = 20;
var minItemLifetime = TimeSpan.FromMilliseconds(1);
var maxItemLifetime = TimeSpan.FromMilliseconds(10);

var randomizer = new Randomizer(1234567);

var nextItemId = 1;

removals.Add(new ChangeSet<Item, int>()
var changeSets = ImmutableArray.CreateBuilder<IChangeSet<Item, int>>(initialCapacity: 5_000);

var cache = new ChangeAwareCache<Item, int>();

while (changeSets.Count < changeSets.Capacity)
{
var changeCount = randomizer.Int(1, maxChangeCount);
for (var i = 0; i < changeCount; ++i)
{
new(reason: ChangeReason.Remove,
key: item.Id,
current: item)
});
var changeReason = randomizer.WeightedRandom(changeReasons, cache.Count switch
{
0 => changeReasonWeightsWhenCountIs0,
_ => changeReasonWeightsOtherwise
});

switch (changeReason)
{
case ChangeReason.Add:
cache.AddOrUpdate(
item: new Item()
{
Id = nextItemId,
Lifetime = randomizer.Bool()
? TimeSpan.FromTicks(randomizer.Long(minItemLifetime.Ticks, maxItemLifetime.Ticks))
: null
},
key: nextItemId);
++nextItemId;
break;

case ChangeReason.Refresh:
cache.Refresh(cache.Keys.ElementAt(randomizer.Int(0, cache.Count - 1)));
break;

case ChangeReason.Remove:
cache.Remove(cache.Keys.ElementAt(randomizer.Int(0, cache.Count - 1)));
break;

case ChangeReason.Update:
var id = cache.Keys.ElementAt(randomizer.Int(0, cache.Count - 1));
cache.AddOrUpdate(
item: new Item()
{
Id = id,
Lifetime = randomizer.Bool()
? TimeSpan.FromTicks(randomizer.Long(minItemLifetime.Ticks, maxItemLifetime.Ticks))
: null
},
key: id);
break;
}
}

changeSets.Add(cache.CaptureChanges());
}

_additions = additions;
_removals = removals;
_changeSets = changeSets.MoveToImmutable();
}

[Benchmark]
[Arguments(1, 0)]
[Arguments(1, 1)]
[Arguments(10, 0)]
[Arguments(10, 1)]
[Arguments(10, 10)]
[Arguments(100, 0)]
[Arguments(100, 1)]
[Arguments(100, 10)]
[Arguments(100, 100)]
[Arguments(1_000, 0)]
[Arguments(1_000, 1)]
[Arguments(1_000, 10)]
[Arguments(1_000, 100)]
[Arguments(1_000, 1_000)]
public void AddsRemovesAndFinalization(int addCount, int removeCount)
public void RandomizedEditsAndExpirations()
{
using var source = new Subject<IChangeSet<Item, int>>();

using var subscription = source
.ExpireAfter(static _ => TimeSpan.FromMinutes(60))
.ExpireAfter(
timeSelector: static item => item.Lifetime,
pollingInterval: null)
.Subscribe();

var itemLifetime = TimeSpan.FromMilliseconds(1);

var itemsToRemove = new List<Item>();

for (var i = 0; i < addCount; ++i)
source.OnNext(_additions[i]);

for (var i = 0; i < removeCount; ++i)
source.OnNext(_removals[i]);
foreach (var changeSet in _changeSets)
source.OnNext(changeSet);

subscription.Dispose();
}

private readonly IReadOnlyList<IChangeSet<Item, int>> _additions;
private readonly IReadOnlyList<IChangeSet<Item, int>> _removals;
private readonly ImmutableArray<IChangeSet<Item, int>> _changeSets;

private sealed class Item
private sealed record Item
{
public int Id { get; init; }
public required int Id { get; init; }

public required TimeSpan? Lifetime { get; init; }
}
}
Loading