8267870: Remove unnecessary char_converter during class loading #4173

coleenp · 2021-05-24T20:24:44Z

We create a String object from a Symbol, then swap the '/' for '.' characters, and create another String object. Seems wasteful. Removed java_lang_String::char_converted and made externalize_classname take a Symbol.

Tested with tier1-3 in mach5.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed

Issue

JDK-8267870: Remove unnecessary char_converter during class loading

Reviewers

David Holmes (@dholmes-ora - Reviewer) ⚠️ Review applies to 00cd12f
Ioi Lam (@iklam - Reviewer) ⚠️ Review applies to 00cd12f

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/4173/head:pull/4173
$ git checkout pull/4173

Update a local copy of the PR:
$ git checkout pull/4173
$ git pull https://git.openjdk.java.net/jdk pull/4173/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 4173

View PR using the GUI difftool:
$ git pr show -t 4173

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/4173.diff

bridgekeeper · 2021-05-24T20:34:19Z

👋 Welcome back coleenp! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2021-05-24T22:12:38Z

@coleenp The following label will be automatically applied to this pull request:

hotspot-runtime

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

mlbridge · 2021-06-15T00:53:50Z

Webrevs

dholmes-ora

Hi Coleen,

Looks like a nice simplification if we never need a general purpose conversion utility.

I take it that Symbol::as_klass_external_name works correctly regardless of whether the name may be unicode or not? i.e that a / is always truly a slash and not part of some unicode sequence?

Thanks,
David

dholmes-ora · 2021-06-15T03:10:12Z

src/hotspot/share/classfile/javaAssertions.cpp

@@ -135,8 +136,8 @@ objArrayHandle names, typeArrayHandle enabled, TRAPS) {
  int index;
  for (index = len - 1; p != 0; p = p->next(), --index) {
    assert(index >= 0, "length does not match list");
-    Handle s = java_lang_String::create_from_str(p->name(), CHECK);
-    s = java_lang_String::char_converter(s, JVM_SIGNATURE_SLASH, JVM_SIGNATURE_DOT, CHECK);
+    TempNewSymbol name = SymbolTable::new_symbol(p->name());


Why do we need a temp symbol?

Because the new_symbol call will increment the Symbol* refcount in the SymbolTable, it's then copied to the java.lang.String, so the name isn't needed after this.

Ah I see. The types were confusing me. We start with a char* convert to a Symbol, then convert that back to char* but with the externalized format. The irony there being that it seems these Options start in external format but then get stored in internal format!
No issue.

Yes, Ioi and I discussed this part of the change but since we substitute slashes for dots and then back again, it is correct. Thanks!

src/hotspot/share/classfile/javaClasses.hpp

openjdk · 2021-06-15T03:25:08Z

@coleenp This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8267870: Remove unnecessary char_converter during class loading

Reviewed-by: dholmes, iklam

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 25 new commits pushed to the master branch:

3ad6586: 8268078: ClassListParser::_interfaces should be freed
48d4562: 8268780: Use 'print_cr' instead of 'print' for the message 'eliminated '
e0f6f70: Merge
788b309: 8268768: idea.sh has been updated in surprising and incompatible ways
9ac0609: 8268828: ProblemList compiler/intrinsics/VectorizedMismatchTest.java on win-x64
fe7a44d: 8268723: Problem list SA core file tests on OSX when using ZGC
31a055e: 8268736: Use apiNote in AutoCloseable.close javadoc
76cad4b: 8263321: Regression 8% in javadoc-steady in 17-b11
e36136f: 8268125: ZGC: Clone oop array gets wrong acopy stub
35d867d: 8268663: Crash when guards contain boolean expression
... and 15 more: https://git.openjdk.java.net/jdk/compare/17295b1bb02b2121978f1459b2e75c5e1031e7ea...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

coleenp · 2021-06-15T14:56:31Z

Since Symbol* comes from class_name_symbol, it's verified to be valid UTF8 and not unicode, so I believe it's valid to translate '/' to '.' using char conversion.
Thanks for the review!

iklam

Looks good to me. I check the code paths and they seem to be equivalent to the existing code. A small nit with CHECK_NULL.

src/hotspot/share/classfile/systemDictionary.cpp

coleenp · 2021-06-16T12:40:38Z

Thanks David and Ioi for the code reviews.
/integrate

openjdk · 2021-06-16T12:42:08Z

Going to push as commit af13fe1.
Since your change was applied there have been 25 commits pushed to the master branch:

3ad6586: 8268078: ClassListParser::_interfaces should be freed
48d4562: 8268780: Use 'print_cr' instead of 'print' for the message 'eliminated '
e0f6f70: Merge
788b309: 8268768: idea.sh has been updated in surprising and incompatible ways
9ac0609: 8268828: ProblemList compiler/intrinsics/VectorizedMismatchTest.java on win-x64
fe7a44d: 8268723: Problem list SA core file tests on OSX when using ZGC
31a055e: 8268736: Use apiNote in AutoCloseable.close javadoc
76cad4b: 8263321: Regression 8% in javadoc-steady in 17-b11
e36136f: 8268125: ZGC: Clone oop array gets wrong acopy stub
35d867d: 8268663: Crash when guards contain boolean expression
... and 15 more: https://git.openjdk.java.net/jdk/compare/17295b1bb02b2121978f1459b2e75c5e1031e7ea...master

Your commit was automatically rebased without conflicts.

openjdk · 2021-06-16T12:42:27Z

@coleenp Pushed as commit af13fe1.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

openjdk bot added the hotspot-runtime label May 24, 2021

Remove unnecessary char_converter

00cd12f

coleenp force-pushed the random branch from 4478127 to 00cd12f Compare June 14, 2021 23:45

coleenp changed the title ~~Remove unnecessary char_converter~~ 8267870: Remove unnecessary char_converter Jun 15, 2021

openjdk bot changed the title ~~8267870: Remove unnecessary char_converter~~ 8267870: Remove unnecessary char_converter during class loading Jun 15, 2021

coleenp marked this pull request as ready for review June 15, 2021 00:35

openjdk bot added the rfr label Jun 15, 2021

dholmes-ora approved these changes Jun 15, 2021

View reviewed changes

openjdk bot added the ready label Jun 15, 2021

Changed name of parameter, some nearby indentation.

afbffd4

iklam approved these changes Jun 16, 2021

View reviewed changes

src/hotspot/share/classfile/systemDictionary.cpp Outdated Show resolved Hide resolved

Fix CHECK_NULL.

d4ec027

openjdk bot closed this Jun 16, 2021

openjdk bot added integrated and removed ready rfr labels Jun 16, 2021

coleenp deleted the random branch June 16, 2021 12:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8267870: Remove unnecessary char_converter during class loading #4173

8267870: Remove unnecessary char_converter during class loading #4173

coleenp commented May 24, 2021 •

edited by openjdk bot

Loading

bridgekeeper bot commented May 24, 2021

openjdk bot commented May 24, 2021

mlbridge bot commented Jun 15, 2021 •

edited

Loading

dholmes-ora left a comment

dholmes-ora Jun 15, 2021

coleenp Jun 15, 2021

dholmes-ora Jun 16, 2021

coleenp Jun 16, 2021

openjdk bot commented Jun 15, 2021 •

edited

Loading

coleenp commented Jun 15, 2021

iklam left a comment

coleenp commented Jun 16, 2021

openjdk bot commented Jun 16, 2021

openjdk bot commented Jun 16, 2021

8267870: Remove unnecessary char_converter during class loading #4173

8267870: Remove unnecessary char_converter during class loading #4173

Conversation

coleenp commented May 24, 2021 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented May 24, 2021

openjdk bot commented May 24, 2021

mlbridge bot commented Jun 15, 2021 • edited Loading

Webrevs

dholmes-ora left a comment

Choose a reason for hiding this comment

dholmes-ora Jun 15, 2021

Choose a reason for hiding this comment

coleenp Jun 15, 2021

Choose a reason for hiding this comment

dholmes-ora Jun 16, 2021

Choose a reason for hiding this comment

coleenp Jun 16, 2021

Choose a reason for hiding this comment

openjdk bot commented Jun 15, 2021 • edited Loading

coleenp commented Jun 15, 2021

iklam left a comment

Choose a reason for hiding this comment

coleenp commented Jun 16, 2021

openjdk bot commented Jun 16, 2021

openjdk bot commented Jun 16, 2021

coleenp commented May 24, 2021 •

edited by openjdk bot

Loading

mlbridge bot commented Jun 15, 2021 •

edited

Loading

openjdk bot commented Jun 15, 2021 •

edited

Loading