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

fix: High CPU usage in FluentDataGrid #1621

Closed
JamesNK opened this issue Mar 2, 2024 · 2 comments · Fixed by #1631 or #1635
Closed

fix: High CPU usage in FluentDataGrid #1621

JamesNK opened this issue Mar 2, 2024 · 2 comments · Fixed by #1631 or #1635
Milestone

Comments

@JamesNK
Copy link
Member

JamesNK commented Mar 2, 2024

🐛 Bug Report

I stress tested Aspire and had a grid with many rows that updated once a second. I noticed very high CPU usage. I profiled and found uncached usage of Expression.Compile().

Profile:
image

Likely offender:

protected internal override string? RawCellContent(TGridItem item)
=> TooltipText?.Compile()(item);

Which is called from RenderRow:

private void RenderRow(RenderTreeBuilder __builder, int rowIndex, TGridItem item)
{
var rowClass = RowClass?.Invoke(item) ?? null;
var rowStyle = RowStyle?.Invoke(item) ?? null;
Loading = false;
<FluentDataGridRow @key="@(ItemKey(item))" GridTemplateColumns=@GridTemplateColumns aria-rowindex="@rowIndex" Class="@rowClass" Style="@rowStyle" TGridItem="TGridItem" Item="@item">
@for (var colIndex = 0; colIndex < _columns.Count; colIndex++)
{
var col = _columns[colIndex];
string? tooltip = col.Tooltip ? @col.RawCellContent(item) : null;
<FluentDataGridCell GridColumn=@(colIndex+1) Class="@ColumnClass(col)" Style="@col.Style" @key="@col" TGridItem="TGridItem" Item="@item" title="@tooltip" aria-label="@tooltip">
@((RenderFragment)(__builder => col.CellContent(__builder, item)))
</FluentDataGridCell>
}
</FluentDataGridRow>
}

💻 Repro or Code Sample

Create a big grid with tooltips. Have it update often.

🤔 Expected Behavior

Renders quickly.

😯 Current Behavior

Very high CPU usage.

💁 Possible Solution

Cache compiled expression.

🔦 Context

🌍 Your Environment

  • OS & Device: [e.g. MacOS, iOS, Windows, Linux] on [iPhone 7, PC]
  • Browser [e.g. Microsoft Edge, Google Chrome, Apple Safari, Mozilla FireFox]
  • .NET and Fluent UI Blazor library Version [e.g. 8.0.2 and 4.4.1]
@JamesNK
Copy link
Member Author

JamesNK commented Mar 5, 2024

Can you reopen this? I think the current solution will create a dictionary for every combination of row type and property type. It would probably be better to have one dictionary for all expressions, and then cast the returned type. It's not as strongly typed, but will return memory usage.

Also, there are some other places where there is Expression.Compile that might need work to improve caching:

if (_lastAssignedProperty != Property)
{
_lastAssignedProperty = Property;
var compiledPropertyExpression = Property.Compile();
if (!string.IsNullOrEmpty(Format))
{
// TODO: Consider using reflection to avoid having to box every value just to call IFormattable.ToString
// For example, define a method "string Type<U>(Func<TGridItem, U> property) where U: IFormattable", and
// then construct the closed type here with U=TProp when we know TProp implements IFormattable
// If the type is nullable, we're interested in formatting the underlying type
var nullableUnderlyingTypeOrNull = Nullable.GetUnderlyingType(typeof(TProp));
if (!typeof(IFormattable).IsAssignableFrom(nullableUnderlyingTypeOrNull ?? typeof(TProp)))
{
throw new InvalidOperationException($"A '{nameof(Format)}' parameter was supplied, but the type '{typeof(TProp)}' does not implement '{typeof(IFormattable)}'.");
}
_cellTextFunc = item => ((IFormattable?)compiledPropertyExpression!(item))?.ToString(Format, null);
}
else
{
_cellTextFunc = item => compiledPropertyExpression!(item)?.ToString();
}
_sortBuilder = Comparer is not null ? GridSort<TGridItem>.ByAscending(Property, Comparer) : GridSort<TGridItem>.ByAscending(Property);
}
Func<TGridItem, string?>? compiledTooltipTextExpression = TooltipText?.Compile();
_cellTooltipTextFunc = compiledTooltipTextExpression is not null
? (item => compiledTooltipTextExpression(item)?.ToString())
: _cellTextFunc;
if (Property.Body is MemberExpression memberExpression)
{
if (Title is null)
{
PropertyInfo = memberExpression.Member as PropertyInfo;
var daText = memberExpression.Member.DeclaringType?.GetDisplayAttributeString(memberExpression.Member.Name);
if (!string.IsNullOrEmpty(daText))
{
Title = daText;
}
else
{
Title = memberExpression.Member.Name;
}
}
}

Some research is probably needed into the best way to cache expressions. This seems like a common problem so there will be people who have solved it before.

@vnbaaij vnbaaij reopened this Mar 5, 2024
@vnbaaij vnbaaij linked a pull request Mar 6, 2024 that will close this issue
@vnbaaij vnbaaij added this to the V4.5.0 milestone Mar 6, 2024
@vnbaaij
Copy link
Collaborator

vnbaaij commented Mar 6, 2024

Expression and Compile logic has been removed from the TooltipText parameter and handling.

@vnbaaij vnbaaij closed this as completed Mar 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants