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

Performance improvement by fixing hashCode() #849

Merged
merged 1 commit into from
Jun 14, 2021

Conversation

testn
Copy link
Contributor

@testn testn commented Nov 1, 2015

Perform minimal change to hashCode() to reduce hash collision. Similar attempt to the fix in #775 to address #772 but much simpler.

@testn testn force-pushed the hash-perf branch 2 times, most recently from fce4c7d to c91d051 Compare November 1, 2015 18:52
@juherr
Copy link
Member

juherr commented Nov 1, 2015

As I said on #772:

I never feel good perf improvments without something concrete to evaluate it.

@testn
Copy link
Contributor Author

testn commented Nov 1, 2015

Yes the performance improves. I just didn't have to patience to run the full test in #772. It cuts at least a couple seconds from the test that takes around 1-2 minutes to run over maven.

Comment on lines +432 to +434
if (m_instance != null) {
hash = hash * 31 + m_instance.hashCode();
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BaseTestMethod#equals uses identity comparison like getInstance() == other.getInstance(), so the hashCode implementation should use identity hashCode rather than user-provided implementation.

In other words, this should be System.identityHashCode(m_instance)

Other than that, the change is a clear improvement with little-to-no risk, so it is safe to merge if m_instance.hashCode() is replaced with identityHashCode

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm confused. How is this a performance improvement? It does more work than the previous implementation.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extra work is negligible, and the new implementation produces better hash codes, so there are less chances for collision in

   at java.util.HashSet.add
   at org.testng.internal.DynamicGraph.setStatus 
   at org.testng.internal.thread.graph.GraphThreadPoolExecutor.setStatus

@juherr
Copy link
Member

juherr commented Jun 14, 2021

@vlsi Any idea how to measure the improvement "easily"?

@vlsi
Copy link
Contributor

vlsi commented Jun 14, 2021

Is there a real need to measure this change?

It might be fun to add "reduce overheads by 10x when running dataprovider test with 100000 values with 16 threads", however, I would rather work on DI improvements.

Of course it would be nice to have a performance regresssion suite, however, it is not the most important issue at the moment.

@juherr juherr merged commit 88d11eb into testng-team:master Jun 14, 2021
@krmahadevan krmahadevan added this to the 7.5.0 milestone Jun 15, 2021
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.

5 participants