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

LruCache faster snapshot #5397

Merged
merged 1 commit into from
Mar 8, 2023
Merged

LruCache faster snapshot #5397

merged 1 commit into from
Mar 8, 2023

Conversation

Scooletz
Copy link
Contributor

@Scooletz Scooletz commented Mar 7, 2023

This PR changes LruCache snapshot into an array. As the snapshot is used only in pruning and requires no Dictionary interface, it can be provided with an array that saves on comparisons, hashing and more.

Changes

  • LruCache.Clone is array returning

Types of changes

What types of changes does your code introduce?

  • Bugfix (a non-breaking change that fixes an issue)
  • New feature (a non-breaking change that adds functionality)
  • Breaking change (a change that causes existing functionality not to work as expected)
  • Optimization
  • Refactoring
  • Documentation update
  • Build-related changes
  • Other: Description

Testing

Requires testing

  • Yes - running a pruning would not hurt a lot
  • No

If yes, did you write tests?

  • Yes
  • No

@Scooletz Scooletz marked this pull request as ready for review March 8, 2023 10:45
@Scooletz Scooletz merged commit a3d2c44 into master Mar 8, 2023
@Scooletz Scooletz deleted the lru-cache-clone branch March 8, 2023 10:46
@OneOnlyMechanic
Copy link

@Scooletz just came across this PR. It may be interesting to consider ValueTuple instead of KeyValuePair here. I believe ValueTuple in general is slightly faster.

@Scooletz
Copy link
Contributor Author

Scooletz commented Mar 8, 2023

@OneOnlyMechanic What do you mean by the following?

ValueTuple in general is slightly faster

Can you elaborate, provide a benchmark or a link to some article about it? The PR was changing much heavier heavy dictionary that was not needed to a plain an array.

@OneOnlyMechanic
Copy link

@OneOnlyMechanic What do you mean by the following?

ValueTuple in general is slightly faster

Can you elaborate, provide a benchmark or a link to some article about it? The PR was changing much heavier heavy dictionary that was not needed to a plain an array.

Agreed that changing from ToDictionary to an Array of KeyValuePair should give a performance boost.

As for value tuple, see the benchmark section here: https://www.dotnetperls.com/tuple
One caveat is the benchmark uses tuple of strings. It may be interesting to rerun the test with different type, such as byte[] to confirm.

@Scooletz
Copy link
Contributor Author

Scooletz commented Mar 9, 2023

Interesting. I run it with Sharplab and the resulting asm is exactly the same for KV and tuple. Won't be pursuing this.

@OneOnlyMechanic
Copy link

Nice to know the asm is the same!
BTW, I did some test. Your change to using Array compared to ToDictionary is about 8 times faster.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants