ParameterState: Add GetState extension #8450
Conversation
|
Without tests for now, as first I want to gather opinions. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #8450 +/- ##
==========================================
+ Coverage 89.50% 89.54% +0.03%
==========================================
Files 411 411
Lines 11832 11846 +14
Branches 2349 2358 +9
==========================================
+ Hits 10590 10607 +17
+ Misses 718 712 -6
- Partials 524 527 +3 ☔ View full report in Codecov by Sentry. |
|
I don't like this MudBlazor/src/MudBlazor/State/ParameterSet.cs Line 111 in 1519898 But there is no other way with HashSet. I guess we can use Dictionary<paramterName, IParameterComponentLifeCycle>, but I think HashSet could be slightly faster due to the simpler data structure? I'm not sure how much, that requires benchmark.
I also do not like this: but this is the fastest cast in C# without type checking etc and we know it implements that interface anyway, and we can't store generics anyway so I dont have better ideas. I think it's still better than boxing the value (or the class) to object or using the non generic lists, here the Unsafe.As<T> should be the less evil.
I guess this is ok It's not hardcore reflection it's just retrieving the metadata. Alternative can use the CallerArgumentExpression but it's question what is faster Expression + reflection or using compiler name - but then you'd need to replace string / regex to extract the parameter name from the expression and I'm unsure what is faster.
|
or |
I will try to bench... Just will take more time since I gotta do a lot of work during the week. |
|
Yes, we definitely have to do some benchmarking as this framework will be used in many components. Don't worry, it isn't time critical at all. In the meantime we expose the state as internal and changing to |
|
Another idea to avoid internal interface IParameterComponentLifeCycle
{
IReadOnlyParameterState<T> GetState<T>();
}internal class ParameterState<T> : IParameterComponentLifeCycle
{
public IReadOnlyParameterState<TParameter> GetState<TParameter>()
{
return (IReadOnlyParameterState<TParameter>)this;
}
} var parameterState = lifeCycle.GetState<T>();Problem is here that we cannot use the existing |
|
I don't know much about optimization. But it all is irrelevant until we have numbers to compare. This should be very easy to measure though. Like calling GetState a million times |
|
Yeah, i just mainly use the issue as sticky note for now, because I can forget what ideas I had |
|
first benchmark
upd: nvm, somehow dotnet run was running cached version of the compiled code. I will update results. |
|
Updated benchmarks: Ok, now we can clearly see |
(IReadOnlyParameterState<TParameter>)this;
Unsafe.As<ParameterState<TParameter>>(this)
Unsafe.As<ParameterState<T>, IReadOnlyParameterState<TParameter>>(ref source)I'd say the difference is not relevant. |
Usage is gonna be more like this, or similar: if (component.GetSate(x => x.Expanded).Value) { ... }The consumer of if (component.GetSate(x => x.Expanded)) { ... } |
|
Just left to bench Expression vs string replace / regex if (lifeCycle is ParameterState<T> parameterState)
{
return parameterState.Value;
}And it was slightly faster than the 3 previous ones. |
|
Just substring wins, but The problem is that the expression is int lastDotIndex = propertyName.LastIndexOf('.');
if (lastDotIndex != -1)
{
string pName = propertyName[(lastDotIndex + 1)..];
return pName;
}pretty simply huh, but I don't know how much cases this covers, if further enchantment will be required for But for extensive reading I would probably just prefer component.GetSate<bool>(nameof(component.Expanded));Below is the code that I used for the different get property name scenarios. Code internal static partial class Extensions
{
[GeneratedRegex(@"(?:\w+=>\s*)*[\w.]+\.(\w+)")]
private static partial Regex LambdaExpressionRegex();
public static string GetPropertyNameSubstringSpan<TComponent, T>(this TComponent component, Func<TComponent, T> propertyExpression, [CallerArgumentExpression(nameof(propertyExpression))] string? propertyName = null) where TComponent : MudComponentBase
{
ArgumentNullException.ThrowIfNull(propertyName);
ReadOnlySpan<char> propertyNameSpan = propertyName.AsSpan();
int lastDotIndex = propertyNameSpan.LastIndexOf('.');
if (lastDotIndex != -1)
{
ReadOnlySpan<char> pName= propertyNameSpan.Slice(lastDotIndex + 1);
return pName.ToString();
}
return propertyName;
}
public static string GetPropertyNameSubstring<TComponent, T>(this TComponent component, Func<TComponent, T> propertyExpression, [CallerArgumentExpression(nameof(propertyExpression))] string? propertyName = null) where TComponent : MudComponentBase
{
ArgumentNullException.ThrowIfNull(propertyName);
int lastDotIndex = propertyName.LastIndexOf('.');
if (lastDotIndex != -1)
{
string pName = propertyName[(lastDotIndex + 1)..];
return pName;
}
return propertyName;
}
public static string GetPropertyNameRegexCallerArgumentExpression<TComponent, T>(this TComponent component, Func<TComponent, T> propertyExpression, [CallerArgumentExpression(nameof(propertyExpression))] string? propertyName = null) where TComponent : MudComponentBase
{
ArgumentNullException.ThrowIfNull(propertyName);
var match = LambdaExpressionRegex().Match(propertyName);
return match.Groups[1].Value;
}
public static string GetPropertyNameReflectionExpression<TComponent, T>(this TComponent component, Expression<Func<TComponent, T>> propertyExpression) where TComponent : MudComponentBase
{
var propertyName = GetPropertyName(propertyExpression);
return propertyName;
}
private static string GetPropertyName<TComponent, T>(Expression<Func<TComponent, T>> propertyExpression) where TComponent : MudComponentBase
{
ArgumentNullException.ThrowIfNull(nameof(propertyExpression));
if (propertyExpression.Body is not MemberExpression body)
{
throw new ArgumentException(@"Invalid argument", nameof(propertyExpression));
}
if (body.Member is not PropertyInfo property)
{
throw new ArgumentException(@"Argument is not a property", nameof(propertyExpression));
}
return property.Name;
}
}I also didn't expect |
Yes you are absolutely right. It is equally usable as the expression syntax, still quite readable and has the best performance. |
|
Will add tests later. |
|
Added tests. |
|
Awesome |
|
I think now that returning |
|
How about adding another |
Description
Usage:
How Has This Been Tested?
None for now
Types of changes
Checklist:
dev).