Bringup Java 11 b04 - implement j.l.String.valueOfCodePoint() #1466

JasonFengJ9 · 2018-03-16T16:06:11Z

Bringup Java 11 b04 - implement j.l.String.valueOfCodePoint()

Implemented j.l.String.valueOfCodePoint(), manually verified that the output of String.valueOfCodePoint(codePoint).codePointAt(0) equals codePoint for all Unicode code point.

closes: #1439

Reviewer @DanHeidinga @fjeremic

Signed-off-by: Jason Feng fengj@ca.ibm.com

DanHeidinga · 2018-03-16T16:20:13Z

Change looks reasonable to me. @fjeremic Any comments on the string compression part?

pshipton · 2018-03-20T00:37:46Z

Rather than manual verification, please add the automated test.

Did you manually test with string compression enabled?

JasonFengJ9 · 2018-03-20T01:10:58Z

Manually verified with both command line options -XX:+CompactStrings and -XX:-CompactStrings.

Will work on automated tests tomorrow.

fjeremic · 2018-03-20T12:03:34Z

jcl/src/java.base/share/classes/java/lang/String.java

+			if (enableCompression) {
+				string = new String(compressedAsciiTable[codePoint], 0, 1, true);
+			} else {
+				string = new String(decompressedAsciiTable[codePoint], 0, 1, false);


Would it be possible to use:

https://github.com/JasonFengJ9/openj9/blob/c864cc795b78b3b1c573e8bc5684c7f9f413c6af/jcl/src/java.base/share/classes/java/lang/String.java#L402

Constructor here instead? The one currently being used will have to perform many of the checks again. Similarly for all other assignments to string in this method.

String(byteArray, coder) works as well, will update the PR accordingly.

fjeremic · 2018-03-20T12:05:18Z

jcl/src/java.base/share/classes/java/lang/String.java

+		String string;
+		if ((codePoint < Character.MIN_CODE_POINT) || (codePoint > Character.MAX_CODE_POINT)) {
+			/*[MSG "K0800", "Invalid Unicode code point"]*/
+			throw new IllegalArgumentException(com.ibm.oti.util.Msg.getString("K0800"));	//$NON-NLS-1$


Would it be possible to put case as the last else case as it occurs least often. The JIT will have to do less work when block shuffling and if it every got it wrong we wouldn't pay the performance penalty.

Wouldn't the next check for <= 255 need to expand then? It would also need to validate codePoint > MIN_CODE_POINT.

Will following checking help JIT performance?

if ((codePoint < Character.MIN_CODE_POINT) { // error handling } else if (codePoint <= 255) { } else if (codePoint < Character.MIN_SUPPLEMENTARY_CODE_POINT) { } else if (codePoint <= Character.MAX_CODE_POINT) { } else { // error handling }

This saves the less often check (codePoint > Character.MAX_CODE_POINT) to the last.

JasonFengJ9 · 2018-03-21T12:20:12Z

@fjeremic the PR is ready for another review with following updates:

Uses constructor String(byteArray, coder);
Modified the order to save less often check (codePoint > Character.MAX_CODE_POINT) last;
Added tests. Note: this test suite is for Java 11 hence will run when Java 11 are introduced to the build jobs. Manually verified that it passes against Java 11 raw builds with modified command line options.

@llxia please arrange a test team review.

Additional note: three project files failed Copyright check however this kind of files don't have copyright header either such as Java[10|9|8]andUp.

DanHeidinga · 2018-03-21T13:50:30Z

jcl/src/java.base/share/classes/java/lang/String.java

+		// Save less often check (codePoint > Character.MAX_CODE_POINT) last
+		if (codePoint < Character.MIN_CODE_POINT) {
+			/*[MSG "K0800", "Invalid Unicode code point"]*/
+			throw new IllegalArgumentException(com.ibm.oti.util.Msg.getString("K0800"));	//$NON-NLS-1$


Can you include the string or the the code point in the exception? It will make it much easier to debug one of these issues.

Yup, now an exception looks like:

Standalone tests: Caused by: java.lang.IllegalArgumentException: Invalid Unicode code point - -1 at java.lang.String.valueOfCodePoint(java.base@11-internal/String.java:4589) Caused by: java.lang.IllegalArgumentException: Invalid Unicode code point - 1114112 at java.lang.String.valueOfCodePoint(java.base@11-internal/String.java:4589) Automated tests: FAILED: testValueOfCodePoint java.lang.AssertionError: Unicode code point mismatch at 0 expected [1] but found [0] at org.testng.Assert.fail(Assert.java:96) at org.testng.Assert.failNotEquals(Assert.java:776) at org.testng.Assert.assertEqualsImpl(Assert.java:137) at org.testng.Assert.assertEquals(Assert.java:118) at org.testng.Assert.assertEquals(Assert.java:652) at org.openj9.test.java_lang.Test_String.testValueOfCodePoint(Test_String.java:49)

DanHeidinga · 2018-03-21T13:52:18Z

test/Java11andUp/src/org/openj9/test/java_lang/Test_String.java

+				try {
+					String str = (String) method.invoke(null, i);
+					Assert.assertEquals(i, str.codePointAt(0), "Unicode code point mismatch at " + i);
+				} catch (IllegalAccessException | IllegalArgumentException | InvocationTargetException e) {


Test cases shouldn't catch unexpected exceptions. Let the test code throw them and the test harness will (usually) print the relevant stacktrace.

Fixed, removed catching of unexpected exceptions.

llxia · 2018-03-21T14:09:54Z

@renfeiw , could you review the test change? Thanks.

fjeremic · 2018-03-21T14:49:56Z

jcl/src/java.base/share/classes/java/lang/String.java

+	static String valueOfCodePoint(int codePoint) {
+		String string;
+		// Save less often check (codePoint > Character.MAX_CODE_POINT) last
+		if (codePoint < Character.MIN_CODE_POINT) {


I would vouch to say this is also an uncommon path and should be moved with the else path. Do we agree?

We need to check that before checking } else if (codePoint <= 255) { or we need to move it into the check for <= 255. Either way, its got to be first.

Sure, but merging the two checks has the benefit of smaller footprint (since the first and the last blocks throw the same exception they can be merged) and less code to jump over to get to the 0 <= codePoint <= 255 case. i.e.

Current:

if (x < 0) { X; } else if (x < 55) { Y; } else if (x < 888) { Z; } else { X; }

can be simplified to:

if (x >= 0 && x < 55) { Y; } else if (x >= 0 && x < 888) { Z; } else { X; }

Which both reduces footprint and makes the lest often taken case the last (X;). Though we do technically introduce another check. The JIT should do the right thing here though in whichever way it is written. I suppose if the JIT does not do this then we have a JIT bug we need to address. I'm fine with the code being either way.

I strongly dislike the duplication of the second form. If we can't optimize the first form reasonably then we're going to have problems with most user code.

It seems you are suggesting code like following:

if (codePoint >= Character.MIN_CODE_POINT && codePoint <= 255) { ... } else if (codePoint >= Character.MIN_CODE_POINT && codePoint < Character.MIN_SUPPLEMENTARY_CODE_POINT) { ... } else if (codePoint >= Character.MIN_CODE_POINT && codePoint <= Character.MAX_CODE_POINT) { ... } else { error handling }

This introduces extra code and confusion (later reader need lots of explanation about the rational of this code logic). I don't think any performance gain here worth the potential issue later.
Now I feel this code should go back to original structure, i.e.,

if ((codePoint < Character.MIN_CODE_POINT) || (codePoint > Character.MAX_CODE_POINT)) { error handling } else if (codePoint <= 255) { ... } else if (codePoint < Character.MIN_SUPPLEMENTARY_CODE_POINT) { ... } else { ... }

It is clear and easy to understand. Can we modify it to improve performance later if this becomes a hot spot?

Yes, I hadn't thought about the extra checks introduced in my original comment. I'm fine either keeping as is or reverting back to your latest suggestion @JasonFengJ9. Apologies for the churn!

No problem, updated PR as latest suggestion above.

renfeiw · 2018-03-21T15:01:20Z

test/Java11andUp/build.xml

+
+	<target name="build" >
+		<if>
+			<equals arg1="${JAVA_VERSION}" arg2="SE110"/>


You might want to change this conditional check to "not SE80 && not SE90 && not SE100". This makes the build.xml to work for SE120 and up without any change.

Agreed, changed the condition to not ^SE(8|9|10)0$$.

renfeiw · 2018-03-21T15:14:47Z

To enable SE110, we need to make an update in testKitGen. (#1495)

JasonFengJ9 · 2018-03-21T20:38:44Z

The PR has been updated to address review comments.
Though Java 11 hasn't been built and run in any test job yet, I manually verified that Java11andUp test project can be built, run against a Java 11 raw build and detect problem as expected.
Please review again.

DanHeidinga · 2018-03-21T22:15:55Z

jcl/src/java.base/share/classes/java/lang/String.java

+	 */
+	static String valueOfCodePoint(int codePoint) {
+		String string;
+		// Save less often check (codePoint > Character.MAX_CODE_POINT) last


Comment is no longer accurate

The comment in question has been removed.

DanHeidinga · 2018-03-21T22:19:04Z

test/Java11andUp/src/org/openj9/test/java_lang/Test_String.java

+		// Character.MIN_CODE_POINT - 1 & Character.MAX_CODE_POINT + 1
+		try {
+			method.invoke(null, Character.MIN_CODE_POINT - 1);
+			method.invoke(null, Character.MAX_CODE_POINT + 1);


The second method.invoke will never run as the first show throw. We need this to be two tests or to have it written like:

try { method.invoke(null, Character.MIN_CODE_POINT - 1); fail() } catch (IllegalArgumentException | InvocationTargetException e) {} try { method.invoke(null, Character.MIN_CODE_POINT - 2); fail() } catch (IllegalArgumentException | InvocationTargetException e) {}

Updated the test with suggested format above.
Please have another look.

Implemented j.l.String.valueOfCodePoint(); Added tests to verify that String.valueOfCodePoint(codePoint).codePointAt(0) equals codePoint for all valid Unicode code point. Signed-off-by: Jason Feng <fengj@ca.ibm.com>

DanHeidinga · 2018-03-22T11:16:05Z

test/Java11andUp/src/org/openj9/test/java_lang/Test_String.java

+		}
+		// Verify that IllegalArgumentException is thrown for Character.MIN_CODE_POINT - 1.
+		try {
+			method.invoke(null, Character.MIN_CODE_POINT - 1);


This needs an explicit Assert.fail() call or the method could succeed and pass the test when we know it should throw the exception.

Agreed, added Assert.fail(message).

DanHeidinga · 2018-03-22T11:16:10Z

test/Java11andUp/src/org/openj9/test/java_lang/Test_String.java

+		}
+		// Verify that IllegalArgumentException is thrown for Character.MAX_CODE_POINT + 1.
+		try {
+			method.invoke(null, Character.MAX_CODE_POINT + 1);


This needs an explicit Assert.fail() call or the method could succeed and pass the test when we know it should throw the exception.

Fixed as well. Please have another look, thanks.

DanHeidinga · 2018-03-22T12:53:01Z

Jenkins test sanity plinux

DanHeidinga · 2018-03-22T12:53:30Z

Running one builds worth of sanity to ensure that this is correctly ignored by older Java levels.

JasonFengJ9 mentioned this pull request Mar 16, 2018

Bringup Java 11 b04 & raw builds #1460

Merged

DanHeidinga requested review from DanHeidinga and fjeremic March 16, 2018 16:19

DanHeidinga added comp:vm jdk11 labels Mar 16, 2018

fjeremic reviewed Mar 20, 2018

View reviewed changes

JasonFengJ9 force-pushed the valueofcp branch 2 times, most recently from ceaaaf3 to 7e7b607 Compare March 21, 2018 12:09

DanHeidinga reviewed Mar 21, 2018

View reviewed changes

fjeremic reviewed Mar 21, 2018

View reviewed changes

renfeiw reviewed Mar 21, 2018

View reviewed changes

JasonFengJ9 force-pushed the valueofcp branch from 7e7b607 to de4dc38 Compare March 21, 2018 20:29

renfeiw approved these changes Mar 21, 2018

View reviewed changes

DanHeidinga reviewed Mar 21, 2018

View reviewed changes

JasonFengJ9 force-pushed the valueofcp branch from de4dc38 to 7b587d4 Compare March 22, 2018 01:37

Bringup Java 11 b04 - implement j.l.String.valueOfCodePoint()

f4b3d6f

Implemented j.l.String.valueOfCodePoint(); Added tests to verify that String.valueOfCodePoint(codePoint).codePointAt(0) equals codePoint for all valid Unicode code point. Signed-off-by: Jason Feng <fengj@ca.ibm.com>

JasonFengJ9 force-pushed the valueofcp branch from 7b587d4 to f4b3d6f Compare March 22, 2018 01:42

pshipton mentioned this pull request Mar 22, 2018

JDK11 build fails during "exploded-image-optimize" target trying to invoke built Java #1478

Closed

DanHeidinga reviewed Mar 22, 2018

View reviewed changes

Signed-off-by: Jason Feng <fengj@ca.ibm.com>

d8068bf

DanHeidinga approved these changes Mar 22, 2018

View reviewed changes

DanHeidinga self-assigned this Mar 22, 2018

DanHeidinga merged commit 4a8df41 into eclipse-openj9:master Mar 22, 2018

fjeremic mentioned this pull request Mar 28, 2018

Created files have a filename of only the first requested character #1563

Closed

JasonFengJ9 deleted the valueofcp branch June 20, 2018 18:06

Bringup Java 11 b04 - implement j.l.String.valueOfCodePoint() #1466

Bringup Java 11 b04 - implement j.l.String.valueOfCodePoint() #1466

Conversation

JasonFengJ9 commented Mar 16, 2018 • edited by pshipton Loading

DanHeidinga commented Mar 16, 2018

pshipton commented Mar 20, 2018

JasonFengJ9 commented Mar 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JasonFengJ9 commented Mar 21, 2018

Choose a reason for hiding this comment

JasonFengJ9 Mar 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

llxia commented Mar 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fjeremic Mar 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

renfeiw commented Mar 21, 2018

JasonFengJ9 commented Mar 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DanHeidinga commented Mar 22, 2018

DanHeidinga commented Mar 22, 2018

JasonFengJ9 commented Mar 16, 2018 •

edited by pshipton

Loading

JasonFengJ9 Mar 21, 2018 •

edited

Loading

fjeremic Mar 21, 2018 •

edited

Loading

JasonFengJ9 commented Mar 21, 2018 •

edited

Loading