RFR (XS) 8060485: (str) contentEquals checks the String contents twice on mismatch

Discussion:

Aleksey Shipilev

2014-10-14 16:05:56 UTC

Hi,

Please review a trivial change in String.contentEquals:
https://bugs.openjdk.java.net/browse/JDK-8060485
http://cr.openjdk.java.net/~shade/8060485/webrev.00/

It improves the performance drastically:
http://cr.openjdk.java.net/~shade/8060485/perf.txt

...not to mention it improves the code readability, and protects us from
rogue CharSequence-s.

Testing: microbenchmarks, jdk/test/String* jtreg.

Thanks,
-Aleksey.

Martin Buchholz

2014-10-14 16:33:42 UTC

Permalink

Looks good to me!

On Tue, Oct 14, 2014 at 9:05 AM, Aleksey Shipilev <

Post by Aleksey Shipilev
Hi,
https://bugs.openjdk.java.net/browse/JDK-8060485
http://cr.openjdk.java.net/~shade/8060485/webrev.00/
http://cr.openjdk.java.net/~shade/8060485/perf.txt
...not to mention it improves the code readability, and protects us from
rogue CharSequence-s.
Testing: microbenchmarks, jdk/test/String* jtreg.
Thanks,
-Aleksey.

Chris Hegarty

2014-10-14 17:13:54 UTC

Permalink

Post by Martin Buchholz
Looks good to me!

+1 'noreg-hard'

-Chris.

Post by Martin Buchholz
On Tue, Oct 14, 2014 at 9:05 AM, Aleksey Shipilev <

Stanimir Simeonoff

2014-10-14 17:32:26 UTC

Permalink

Hi,

This is an unrelated issue, yet is there any reason for the inner loop of
equals to be written in such a (confusing) way?

if (n == anotherString.value.length) {
char v1[] = value;
char v2[] = anotherString.value;
int i = 0;
while (n-- != 0) {
if (v1[i] != v2[i])
return false;
i++;
}
return true;
}

instead of just "for (int i=0;i<n;i++) if (v1[i]!=v2[i]) return false;"

Stanimir

On Tue, Oct 14, 2014 at 8:13 PM, Chris Hegarty <chris.hegarty at oracle.com>

Post by Chris Hegarty

Post by Martin Buchholz
Looks good to me!

+1 'noreg-hard'
-Chris.

Post by Martin Buchholz
On Tue, Oct 14, 2014 at 9:05 AM, Aleksey Shipilev <

Aleksey Shipilev

2014-10-14 17:41:08 UTC

Permalink

Post by Stanimir Simeonoff
Hi,
This is an unrelated issue, yet is there any reason for the inner loop
of equals to be written in such a (confusing) way?
if (n == anotherString.value.length) {
char v1[] = value;
char v2[] = anotherString.value;
int i = 0;
while (n-- != 0) {
if (v1[i] != v2[i])
return false;
i++;
}
return true;
}
instead of just "for (int i=0;i<n;i++) if (v1[i]!=v2[i]) return false;"

I think nobody just cares enough. Submit a patch for this?

The String.equals implementation is still intrinsified by C2 anyhow.

-Aleksey.

Aleksey Shipilev

2014-10-14 17:38:22 UTC

Permalink

Thanks guys!

And of course, I managed to do two minor mistakes in a two-line change:
the indentation is a bit wrong, and cast to String is redundant. Here is
the updated webrev and the changeset (need a Sponsor!):
http://cr.openjdk.java.net/~shade/8060485/webrev.01/
http://cr.openjdk.java.net/~shade/8060485/8060485.changeset

-Aleksey.

Post by Chris Hegarty

Post by Martin Buchholz
Looks good to me!

+1 'noreg-hard'
-Chris.

Post by Martin Buchholz
On Tue, Oct 14, 2014 at 9:05 AM, Aleksey Shipilev <

Alan Bateman

2014-10-14 19:55:59 UTC

Permalink

Post by Aleksey Shipilev
Thanks guys!
the indentation is a bit wrong, and cast to String is redundant. Here is
http://cr.openjdk.java.net/~shade/8060485/webrev.01/
http://cr.openjdk.java.net/~shade/8060485/8060485.changeset
-Aleksey.

Updated version looks okay. I wonder how common it might be to call this
method with String vs. a StringBuilder, just wondering if it should
check for a String first (although the type check should be really quick
and probably wouldn't make a difference).

-Alan

Stanimir Simeonoff

2014-10-14 20:20:20 UTC

Permalink

a

On Tue, Oct 14, 2014 at 10:55 PM, Alan Bateman <Alan.Bateman at oracle.com>

Post by Alan Bateman

Updated version looks okay. I wonder how common it might be to call this
method with String vs. a StringBuilder, just wondering if it should check
for a String first (although the type check should be really quick and
probably wouldn't make a difference).
I was wondering the exactly same but usually allowing CharSequence in the

code means "likely no String", so while there is no statistical evidence it
seems reasonable.

Another minor issue is the legacy contentEquals(StringBuffer sb) that can
call directly synchronized(sb){nonSyncContentEquals(sb);} but C2 should be
able to inline it pretty good just in case.

Stanimir

Aleksey Shipilev

2014-10-15 07:54:24 UTC

Permalink

Post by Alan Bateman

Thanks Alan.

One of my Twitter followers has pointed this thing to me, so I presume
they got hit by this quirk while calling contentEquals with String. I
concur the instanceof check is very cheap, and so it does not matter to
check the String first. In either case, I think the most preferred way
for checking the String vs. String equality is equals(), not
contentEquals().

Added you as the reviewer:
http://cr.openjdk.java.net/~shade/8060485/8060485.changeset

Need a sponsor to push!

-Aleksey.

Paul Sandoz

2014-10-15 08:57:36 UTC

Permalink

Post by Aleksey Shipilev

Post by Alan Bateman

Thanks Alan.
One of my Twitter followers has pointed this thing to me, so I presume
they got hit by this quirk while calling contentEquals with String. I
concur the instanceof check is very cheap, and so it does not matter to
check the String first. In either case, I think the most preferred way
for checking the String vs. String equality is equals(), not
contentEquals().
http://cr.openjdk.java.net/~shade/8060485/8060485.changeset
Need a sponsor to push!

I can do it later today.

Paul.

Paul Sandoz

2014-10-15 08:59:21 UTC

Permalink

Post by Paul Sandoz

Post by Aleksey Shipilev
Need a sponsor to push!

I can do it later today.

I knew i should of checked before hand, i see Alan has already pushed it,
Pauk.