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 .equals instead of == to compare strings in findVariableName #4164

Closed
wants to merge 2 commits into from

Conversation

jsyeo
Copy link
Contributor

@jsyeo jsyeo commented Sep 21, 2016

The == string comparison works for some cases but the .exists method will break when we try to look for a dynamically created string. Like in this snippet:

    StaticScope scope = StaticScopeFactory.newStaticScope(null, StaticScope.Type.LOCAL, null);
    SimpleSourcePosition pos = new SimpleSourcePosition("<eval>", 0);
    scope.assign(pos, "a", new TrueNode(pos));
    String a = new StringBuilder().append('a').toString();
    assertTrue(scope.exists(a) >= 0); // assertion fails as .exists returns -1

The `==` string comparison works for some cases but the `.exists` method will break when we try to look for a dynamically created string. Like in this snippet:

```java
    StaticScope scope = StaticScopeFactory.newStaticScope(null, StaticScope.Type.LOCAL, null);
    SimpleSourcePosition pos = new SimpleSourcePosition("<eval>", 0);
    scope.assign(pos, "a", new TrueNode(pos));
    String a = new StringBuilder().append('a').toString();
    assertTrue(scope.exists(a) >= 0); // assertion fails as .exists returns -1
```
@jsyeo jsyeo closed this Sep 21, 2016
@jsyeo jsyeo deleted the patch-1 branch September 21, 2016 08:55
@jsyeo
Copy link
Contributor Author

jsyeo commented Sep 21, 2016

Hmmm, seems like that was intentional. Gonna close this PR.

@headius
Copy link
Member

headius commented Sep 21, 2016

The strings in our StaticScope are expected to be interned strings, to allow for == comparison. However on modern JVMs that may be premature optimization. @enebo knows more about what we do and don't intern and why.

@enebo
Copy link
Member

enebo commented Sep 21, 2016

We should revisit this. In Java 4, it was a measurable win but I suspect by Java 7/8/9 this may not show up (many of our older microopts ended up not being important java 5+). There were two parts to why we did this:

  1. cold performance. single == was not something the JVM needed to inline. It is as simple as it gets already.
  2. warm performance. I think we discovered at the time .equals() did not inline due to inlining budget in some places so it was just slower than guaranteeing all strings were intern'd so we could use ==.

Of course we have been seriously talking about using bytelist as a replacement for string to resolve our last M17n support for symbols/methods. If we do that then all this code will probably change as well.

HEH. I guess I recommend we stop relying on == and use equals() in the near term but that I don't know how best to measure the impact of that change. In the past it was pretty obvious but as I said I suspect it will be harder to see (therefore we should do it to avoid programmer errors)

@headius
Copy link
Member

headius commented Sep 21, 2016

@enebo I have removed dependence on == whenever I refactored logic that used it, but I know we still have a LOT of cases that do it. IntelliJ has a way to report that, I'll see if I can come up with something actionable.

@headius
Copy link
Member

headius commented Sep 21, 2016

Here's the report, in the best format I could get it. Only 37 places in codebase were identified. This probably misses places where we're doing Object == Object where they're both String.

https://headius.github.io/jruby-string-equality/

@enebo enebo added this to the Invalid or Duplicate milestone Nov 9, 2016
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.

None yet

3 participants