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

Replaced == with equals on two String objects #2102

Merged
merged 1 commit into from
Dec 17, 2024

Conversation

PieterOlivier
Copy link
Contributor

Two String objects where compared using ==, this is hardly ever correct and can be unexpected by the callers of the searchCategory method.

Copy link

codecov bot commented Dec 17, 2024

Codecov Report

Attention: Patch coverage is 0% with 1 line in your changes missing coverage. Please review.

Project coverage is 49%. Comparing base (043b663) to head (400f5f8).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...c/org/rascalmpl/values/parsetrees/TreeAdapter.java 0% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##              main   #2102   +/-   ##
=======================================
- Coverage       49%     49%   -1%     
- Complexity    6317    6320    +3     
=======================================
  Files          666     666           
  Lines        59695   59695           
  Branches      8670    8670           
=======================================
- Hits         29600   29594    -6     
- Misses       27863   27866    +3     
- Partials      2232    2235    +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@DavyLandman
Copy link
Member

Inside vallang and rascal we've taken care to itern certain strings, precisely to cut down on equality costs.

It might not be valid for this specific compare, but it's a local choice we actually depend on in a lot places.

@PieterOlivier
Copy link
Contributor Author

That might be the case, but if the strings are interned, equals should not be much more expensive than ==. I expect any decent jit implementation to inline this check.
And if the function argument is expected to always be an interned String that should be stated clearly in the contract while this (public) method does not have any documentation at all.

This specific method is not used at all in rascal or rascal-core so there will not be a performance hit with this fix. It also probably means nobody cares. Unless some unexpected user stumbles on this function and tries to use it, in which case it is highly likely she will be bitten by this issue.

Also, String interning comes with caveats of its own, but you probably already know that: https://shipilev.net/jvm/anatomy-quarks/10-string-intern/

@DavyLandman
Copy link
Member

DavyLandman commented Dec 17, 2024

Equals is only fast if it's equals, due to the pointer compare. But if not equals, it has to iterate the chars to find out the kast character differs.

And yes, we sometimes don't use the build in interning, but have a Caffeine cache to make sure all strings come from the same pool, without overloading the pool. We've only done it in places where we could measure the improvement, either in perf or in memory consumption.

Copy link
Member

@DavyLandman DavyLandman left a comment

Choose a reason for hiding this comment

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

I approve of this change, especially as it's not in a hot path.

My comments where more intended to explain why we do actually use string reference equality in quite some parts in vallang (and a bit in some parts of rascal).

@DavyLandman DavyLandman merged commit 540416b into main Dec 17, 2024
6 of 7 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.

3 participants