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

Made some small optimizations to Dictionaries and Enumerables #724

Merged
merged 5 commits into from
Feb 12, 2025

Conversation

michelebastione
Copy link
Contributor

Hello, I was going through the repository and noticed some potential improvements so I made the following edits:

  • Switched most ContainsKey calls to TryGetValue to avoid going through a dictionary's hashmap twice
  • Materialized some Enumerables to make sure not to evaluate them multiple times in a row
  • Changed IEnumerable's Count() method to ICollection's Count property to avoid unnecessary enumeration of the collection
  • Added safe cast of IDisposable to IEnumerator to prevent possible memory leak scenarios of implementations not getting garbage collected correctly

I hope it doesn't look presumptuous of me to propose these many modifications at once but I figured at least some of them could be useful. I ran all the tests multiple times and everything seems to be in order. What do you think?

Fetching a value from a Dictionary after checking if it contains the required key means going through its hashmap twice. It's not a very significant overhead at all but it's completely unnecessary.
Materialization done to avoid possible evaluations of the same IEnumerables multiple times in a row
The IEnumerable Count() method usually goes through a full enumeration of all the elements while the ICollection Count property gets the value from a field.
The field _enumerator, being defined as an IEnumerator, does not directly suppoort IDisposable, however most of its implementations also inherit from IEnumerator<T>, which does. This could lead to potential memory leak problems if not handled with a Dispose call.
@michelebastione michelebastione merged commit 9b37e22 into mini-software:master Feb 12, 2025
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants