You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@lucenenet.apache.org by "paulirwin (via GitHub)" <gi...@apache.org> on 2024/12/18 05:22:59 UTC
[PR] SWEEP: Add unchecked to GetHashCode, #1065 [lucenenet]
paulirwin opened a new pull request, #1068:
URL: https://github.com/apache/lucenenet/pull/1068
- [X] You've read the [Contributor Guide](https://github.com/apache/lucenenet/blob/main/CONTRIBUTING.md) and [Code of Conduct](https://www.apache.org/foundation/policies/conduct.html).
- [ ] You've included unit or integration tests for your change, where applicable.
- [ ] You've included inline docs for your change, where applicable.
- [X] There's an open issue for the PR that you are making. If you'd like to propose a change, please [open an issue](https://github.com/apache/lucenenet/issues/new/choose) to discuss the change or find an existing issue.
Adds `unchecked` to all GetHashCode implementations where it is applicable
Fixes #1065
## Description
GetHashCode should not throw an exception on overflow. This PR adds `unchecked` blocks to all GetHashCode implementations that do addition or multiplication that could overflow.
During review, it was determined that GetHashCode methods that return constants, simply delegate GetHashCode to another object, or solely do bitwise xor operations do not need `unchecked` added.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@lucenenet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] SWEEP: Add unchecked to GetHashCode, #1065 [lucenenet]
Posted by "NightOwl888 (via GitHub)" <gi...@apache.org>.
NightOwl888 merged PR #1068:
URL: https://github.com/apache/lucenenet/pull/1068
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@lucenenet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] SWEEP: Add unchecked to GetHashCode, #1065 [lucenenet]
Posted by "paulirwin (via GitHub)" <gi...@apache.org>.
paulirwin commented on PR #1068:
URL: https://github.com/apache/lucenenet/pull/1068#issuecomment-2551852700
Aside: we seem to be more regularly be getting transient build failures downloading from nuget.org and MyGet again. This possibly is a result of adding the .NET 9 tests with an extra ~70 actions running in parallel. Fortunately the workaround is simple, to just choose the option to re-run failed jobs.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@lucenenet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] SWEEP: Add unchecked to GetHashCode, #1065 [lucenenet]
Posted by "NightOwl888 (via GitHub)" <gi...@apache.org>.
NightOwl888 commented on PR #1068:
URL: https://github.com/apache/lucenenet/pull/1068#issuecomment-2552657461
> Aside: we seem to be more regularly be getting transient build failures downloading from nuget.org and MyGet again. This possibly is a result of adding the .NET 9 tests with an extra ~70 actions running in parallel. Fortunately the workaround is simple, to just choose the option to re-run failed jobs.
I suspect this is due to extra holiday traffic, however, it would probably help if we enabled NuGet caching (#856 and #857). I am not convinced we need to generate an extra file and figure out how to maintain it just so we can update packages, though. Our first attempt at doing so (on Azure DevOps) didn't need to, but the key didn't include the `dependencies.props` file (which missed the entire point) and apparently the only way to fix that is to completely rename the cache key so it can include that file (it should include all `.props`, `.targets`, and '.*proj` files just to be on the safe side).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@lucenenet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] SWEEP: Add unchecked to GetHashCode, #1065 [lucenenet]
Posted by "paulirwin (via GitHub)" <gi...@apache.org>.
paulirwin commented on code in PR #1068:
URL: https://github.com/apache/lucenenet/pull/1068#discussion_r1890546061
##########
src/Lucene.Net/Util/FixedBitSet.cs:
##########
@@ -724,15 +724,18 @@ public override bool Equals(object o)
public override int GetHashCode()
{
- long h = 0;
- for (int i = numWords; --i >= 0; )
+ unchecked
{
- h ^= bits[i];
- h = (h << 1) | (h >>> 63); // rotate left
+ long h = 0;
+ for (int i = numWords; --i >= 0; )
+ {
+ h ^= bits[i];
+ h = (h << 1) | (h >>> 63); // rotate left
+ }
+ // fold leftmost bits into right and add a constant to prevent
+ // empty sets from returning 0, which is too common.
+ return (int)((h >> 32) ^ h) + unchecked((int)0x98761234);
Review Comment:
Good catch on these, thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@lucenenet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] SWEEP: Add unchecked to GetHashCode, #1065 [lucenenet]
Posted by "NightOwl888 (via GitHub)" <gi...@apache.org>.
NightOwl888 commented on code in PR #1068:
URL: https://github.com/apache/lucenenet/pull/1068#discussion_r1890412653
##########
src/Lucene.Net/Util/OpenBitSet.cs:
##########
@@ -1104,17 +1104,20 @@ public override bool Equals(object o)
public override int GetHashCode()
{
- // Start with a zero hash and use a mix that results in zero if the input is zero.
- // this effectively truncates trailing zeros without an explicit check.
- long h = 0;
- for (int i = m_bits.Length; --i >= 0; )
+ unchecked
{
- h ^= m_bits[i];
- h = (h << 1) | (h >>> 63); // rotate left
+ // Start with a zero hash and use a mix that results in zero if the input is zero.
+ // this effectively truncates trailing zeros without an explicit check.
+ long h = 0;
+ for (int i = m_bits.Length; --i >= 0; )
+ {
+ h ^= m_bits[i];
+ h = (h << 1) | (h >>> 63); // rotate left
+ }
+ // fold leftmost bits into right and add a constant to prevent
+ // empty sets from returning 0, which is too common.
+ return (int)((h >> 32) ^ h) + unchecked((int)0x98761234);
Review Comment:
We can remove the unchecked cast to `int` from `uint` here.
##########
src/Lucene.Net/Util/LongBitSet.cs:
##########
@@ -452,15 +452,18 @@ public override bool Equals(object o)
public override int GetHashCode()
{
- long h = 0;
- for (int i = numWords; --i >= 0; )
+ unchecked
{
- h ^= bits[i];
- h = (h << 1) | (h >>> 63); // rotate left
+ long h = 0;
+ for (int i = numWords; --i >= 0; )
+ {
+ h ^= bits[i];
+ h = (h << 1) | (h >>> 63); // rotate left
+ }
+ // fold leftmost bits into right and add a constant to prevent
+ // empty sets from returning 0, which is too common.
+ return (int)((h >> 32) ^ h) + unchecked((int)0x98761234);
Review Comment:
We can remove the unchecked cast to `int` from `uint` here.
##########
src/Lucene.Net/Util/FixedBitSet.cs:
##########
@@ -724,15 +724,18 @@ public override bool Equals(object o)
public override int GetHashCode()
{
- long h = 0;
- for (int i = numWords; --i >= 0; )
+ unchecked
{
- h ^= bits[i];
- h = (h << 1) | (h >>> 63); // rotate left
+ long h = 0;
+ for (int i = numWords; --i >= 0; )
+ {
+ h ^= bits[i];
+ h = (h << 1) | (h >>> 63); // rotate left
+ }
+ // fold leftmost bits into right and add a constant to prevent
+ // empty sets from returning 0, which is too common.
+ return (int)((h >> 32) ^ h) + unchecked((int)0x98761234);
Review Comment:
We can remove the unchecked cast to `int` from `uint` here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@lucenenet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] SWEEP: Add unchecked to GetHashCode, #1065 [lucenenet]
Posted by "NightOwl888 (via GitHub)" <gi...@apache.org>.
NightOwl888 commented on PR #1068:
URL: https://github.com/apache/lucenenet/pull/1068#issuecomment-2551620616
Wouldn't this be categorized as a performance improvement rather than a bugfix?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@lucenenet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] SWEEP: Add unchecked to GetHashCode, #1065 [lucenenet]
Posted by "paulirwin (via GitHub)" <gi...@apache.org>.
paulirwin commented on PR #1068:
URL: https://github.com/apache/lucenenet/pull/1068#issuecomment-2551782910
> Wouldn't this be categorized as a performance improvement rather than a bugfix?
I'm not sure there would be any performance impact. Originally I had assumed that we needed to do this to avoid overflow exceptions, but I realized just now that our projects do not have the "Check for arithmetic overflow" compiler option checked, so that reasoning is not valid alone. Perhaps an argument could be made that it helps ensure best practices and might help users if they want to recompile the library with that option for some reason, as well as to be consistent with other places in the codebase where we were already using `unchecked` blocks in GetHashCode... but in terms of performance, it looks like it generates the same opcodes without these blocks (i.e. `add` vs `add.ovf`), so there's no perf benefit here either. I'm not opposed to still moving forward with this PR for consistency's sake, but it's not the bugfix I thought it was. Perhaps "notes:ignore" would be a better label then.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@lucenenet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org