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

Code Cleanup: Improved Performance of MudComponentBase, ResizeObserver, and EventListener #8526

Merged
merged 8 commits into from
Apr 6, 2024

Conversation

jperson2000
Copy link
Sponsor Contributor

@jperson2000 jperson2000 commented Mar 29, 2024

Description

This update offers a minor improvement to performance by fixing four instances where Dictionary<T,U> lookups were being performed twice, as well as an optimization regarding unnecessary JSON deserialization. Regarding dictionary lookups, consider the following code:

Dictionary<string, string> values = new();
if(values.ContainsKey("test")) 
{ 
    return values["test"]; 
}

... in the above code, the values dictionary is being searched twice: once via ContainsKey then again via the indexer. By refactoring this to use TryGetValue, we can eliminate the second lookup, like this:

Dictionary<string, string> values = new();
if(values.TryGetValue("test", out var result)) 
{
    return result;
}

The four instances fixed for this PR improve the performance of the following properties and methods:

  • MudComponentBase.FieldId
  • MudDropZone.GetItemIndex()
  • EventListener.OnEventOccur()
  • ResizeObserver.OnSizeChanged()

Additionally, two optimizations were made to EventListener:

  • The @event variable was being deserialized from JSON even if no handler existed to utilize it
  • A new instance of JsonSerializerOptions was being made for each call when a static variable could have been reused

How Has This Been Tested?

This was tested by running unit tests after changes as well as by visually testing affected components such as testing page resizes, testing all form examples, and testing all MudDropZone examples. The EventListener was tested via the MudColorPicker example.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

  • The PR is submitted to the correct branch (dev).
  • My code follows the code style of this project.
  • I've added relevant tests.

Notes for Reviewers

I get a NullReferenceException when using the MudColorPicker for the first time because it's attempting to create a MudColor(null). I wonder if we can handle this a bit better somehow? (This error existed before this PR; I just noticed it during testing.)

@github-actions github-actions bot added bug Something does not work as intended/expected PR: needs review labels Mar 29, 2024
Copy link

codecov bot commented Mar 29, 2024

Codecov Report

Attention: Patch coverage is 94.11765% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 89.77%. Comparing base (9b1ebb5) to head (493d0c2).

Files Patch % Lines
src/MudBlazor/Base/MudComponentBase.cs 66.66% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##              dev    #8526   +/-   ##
=======================================
  Coverage   89.76%   89.77%           
=======================================
  Files         411      411           
  Lines       11824    11830    +6     
  Branches     2362     2362           
=======================================
+ Hits        10614    10620    +6     
  Misses        682      682           
  Partials      528      528           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ScarletKuro ScarletKuro requested a review from henon March 30, 2024 16:10
@ScarletKuro
Copy link
Member

Hi.
Sorry, can you resolve the conflicts?
I do think this change is important since TryGetValue is much more performant than ContainsKey("test") -> values["test"].

danielchalmers added a commit to danielchalmers/MudBlazor that referenced this pull request Apr 4, 2024
@jperson2000
Copy link
Sponsor Contributor Author

Hi. Sorry, can you resolve the conflicts? I do think this change is important since TryGetValue is much more performant than ContainsKey("test") -> values["test"].

You bet; I've just resolved the 2 conflicts

@henon henon merged commit 216509f into MudBlazor:dev Apr 6, 2024
4 checks passed
@henon
Copy link
Collaborator

henon commented Apr 6, 2024

Thanks @jperson2000

danielchalmers pushed a commit to danielchalmers/MudBlazor that referenced this pull request Apr 7, 2024
@jperson2000 jperson2000 deleted the feature/eventmanager-optimization branch April 15, 2024 15:59
biegehydra pushed a commit to biegehydra/MudBlazor that referenced this pull request Apr 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something does not work as intended/expected
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants