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

Use concrete ConcurrentHashSet instead of abstraction for performance #960

Merged
merged 2 commits into from
Sep 20, 2024

Conversation

Shazwazza
Copy link
Contributor

  • You've read the Contributor Guide and Code of Conduct.
  • You've included unit or integration tests for your change, where applicable.
  • You've included inline docs for your change, where applicable.
  • There's an open issue for the PR that you are making. If you'd like to propose a change, please open an issue to discuss the change or find an existing issue.

When it is a concrete type, this is avoided. Since this is an internal field there's no reason to use the abstracted type.

See https://learn.microsoft.com/en-us/dotnet/fundamentals/code-analysis/quality-rules/ca1859

Calling into an interface when its not necessary will result in poorer performance because the JIT needs to do some type analysis. When it is a concrete type, this is avoided. Since this is an internal field there's no reason to use the abstracted type. 

See https://learn.microsoft.com/en-us/dotnet/fundamentals/code-analysis/quality-rules/ca1859
@paulirwin
Copy link
Contributor

@Shazwazza I agree with this change. Even though the upstream Java code uses the abstraction for the field type, we're already deviated from the upstream code by using a set instead of a list. I think this field could use a // LUCENENET-specific: comment on it though to note for future porting work.

And I'm guessing you saw the build failed, so once that is fixed if you wouldn't mind adding that comment I'd be happy to approve.

@Shazwazza
Copy link
Contributor Author

@paulirwin build has been fixed

@Shazwazza
Copy link
Contributor Author

Though, not sure why some tests fail, that seems very strange

@paulirwin
Copy link
Contributor

Looks like it was just a transient error downloading the NuGet package feed, I've seen that before. Re-ran the tests and they passed. Thanks!

@paulirwin paulirwin merged commit 627fccc into master Sep 20, 2024
199 checks passed
@paulirwin paulirwin deleted the Shazwazza-patch-1 branch September 20, 2024 21:34
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