-
Notifications
You must be signed in to change notification settings - Fork 721
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
Fix implementation of String equalsIgnoreCase() and regionMatches() #11423
Conversation
@JasonFengJ9 can you please review this? |
a2d453c
to
a3b46a4
Compare
Updated to use It seems we may also want to update
Right now, we're doing
|
Using a similar test case to #11399, I'm getting the following for
but the RI returns |
a3b46a4
to
0278132
Compare
Updated with |
test/functional/Java8andUp/src/org/openj9/test/java/lang/Test_String.java
Outdated
Show resolved
Hide resolved
fcbd73d
to
4899053
Compare
Addressed review comments and ran test cases with and without |
4899053
to
0d017f1
Compare
8440704
to
e3cbba5
Compare
Updated PR with fixes. Ran personal build with the |
91c9e0a
to
c51e721
Compare
c51e721
to
823e1c6
Compare
FYI tagging @0xdaryl. I forget if |
7a3bd02
to
ce0fa8c
Compare
jenkins test sanity plinux jdk8,jdk11 |
Agree. We won't know its impact until we measure. It seems prudent to perform some performance runs on workloads OpenJ9 cares about before this is merged. |
@sharon-wang pls follow up with Piyush on this. |
According to the javadoc, equalsIgnoreCase() and regionMatches() both involve calling `Character.toLowerCase(Character.toUpperCase(character))` on each character in the string. Add tests with Turkish characters for equalsIgnoreCase(), regionMatches() and compareToIgnoreCase(). Use new helper charValuesEqualIgnoreCase() to compare if two characters are equal (case insensitive). [skip ci] Signed-off-by: Sharon Wang <sharon-wang-cpsc@outlook.com>
ce0fa8c
to
093e96f
Compare
LibertyStartupDT7 ResultsThese are the aggregate startup results from 6 iterations of the benchmark.
Is this regression in startup performance considered slight (acceptable), or will more optimization need to be done? |
@mpirvu: can you provide some guidance on what is considered acceptable for an OpenJ9 startup regression? In Sharon's results above, it is a little under 1% on average for DT7. |
Typically 0.85% is considered noise if you do just 6 iterations of the benchmark, but the quoted confidence interval is suspiciously low. If that holds, then this is a true regression. I can do those runs if I am provided with two builds: before and after. |
FYI @vijaysun-omr |
I dug up some old profiles of DayTrader7. Here are the top methods:
What I was concerned about was the third one, i.e. Going back to the changes being made here, they are in the case sensitive version of this API. This will not be on the critical path of at least DayTrader7. I think we should be ok here. |
My own DT7 startup experiments show virtually no difference between the two builds provided by @sharon-wang
|
I think we should be ok to proceed here. |
Thank you all for the help with investigating perf! @pshipton Looks like this should be ready to merge. |
@JasonFengJ9 any further concerns? @sharon-wang was your last push 2 days ago just a rebase, or did something change? |
Yes, it was a rebase to bring sources up to date for the perf tests. |
In that case I won't bother re-running the PR testing, which passed. It lgtm, once Jason signs off this can be merged. We should also add the change to the 0.24 release. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Thanks @sharon-wang
Opened #11497 to add this to 0.24. |
Fixes: #11399
According to the javadoc,
equalsIgnoreCase()
andregionMatches()
both involve callingCharacter.toLowerCase(Character.toUpperCase(character))
on each character in the string. This PR matches that expected behaviour.Added tests with Turkish characters for
equalsIgnoreCase()
,regionMatches()
andcompareToIgnoreCase()
.Uses new helper
charValuesEqualIgnoreCase()
to compare if two characters are equal (case insensitive).