8333103: Re-examine the console provider loading #19467

naotoj · 2024-05-29T19:51:36Z

There is an initialization code in Console class that searches for the Console implementations. Refactoring the init code not to use lambda/stream would reduce the (initial) number of loaded classes by about 100 for java.base implementations. This would become relevant when the java.io.IO (JEP 477) uses Console as the underlying framework.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8333103: Re-examine the console provider loading (Bug - P4)

Reviewers

Claes Redestad (@cl4es - Reviewer) ⚠️ Review applies to ae06bb2c
Jaikiran Pai (@jaikiran - Reviewer) ⚠️ Review applies to ae06bb2c

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/19467/head:pull/19467
$ git checkout pull/19467

Update a local copy of the PR:
$ git checkout pull/19467
$ git pull https://git.openjdk.org/jdk.git pull/19467/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 19467

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/19467.diff

Webrev

Link to Webrev Comment

bridgekeeper · 2024-05-29T19:52:30Z

👋 Welcome back naoto! 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 · 2024-05-29T19:53:14Z

@naotoj 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:

8333103: Re-examine the console provider loading

Reviewed-by: redestad, jpai

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 41 new commits pushed to the master branch:

ec88c6a: 8332917: failure_handler should execute gdb "info threads" command on linux
b3e29db: 8333108: Update vmTestbase/nsk/share/DebugeeProcess.java to don't use finalization
11e926c: 8332777: Update JCStress test suite
44c1845: 8330852: All callers of JvmtiEnvBase::get_threadOop_and_JavaThread should pass current thread explicitly
922e312: 8328611: Thread safety issue in com.sun.tools.jdi.ReferenceTypeImpl::classObject
1d889e5: 8332487: Regression in Crypto-AESGCMBench.encrypt (and others) after JDK-8328181
32636dc: 8333105: Shenandoah: Results of concurrent mark may be lost for degenerated cycle
7071542: 8331189: Implementation of Scoped Values (Third Preview)
4acafb8: 8333107: javac fails with an exception when processing broken lambda
921860d: 8333264: Remove unused resolve_sub_helper declaration after JDK-8322630
... and 31 more: https://git.openjdk.org/jdk/compare/b8f2ec9091f9f7e5f4611991d04dd8aa113b94fd...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.

openjdk · 2024-05-29T19:53:31Z

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

core-libs

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 · 2024-05-29T20:33:08Z

Webrevs

01: Full - Incremental (42a45323)
00: Full (ae06bb2c)

liach · 2024-05-29T21:27:30Z

src/java.base/share/classes/java/io/Console.java

+                            if (jc != null) {
+                                return new ProxyingConsole(jc);
+                            }
+                            break;


Suggested change

break;

The original findAny is only after filter(Objects::nonNull) meaning we don't return null on a null jcp.console result.

Yes, break guarantees that the search completes one way or another once the module name has been matched. This is not how it used to be done.

Right. Since findAny is after the module name matching, there is at most 1 match. In the case we didn't find any, the final orElse(null) eventually returns null. So the refactored code is semantically the same.

It's only semantically the same if we assume a module can only provide a single JdkConsoleProvider, no? The break; disallows multiple providers (for disjoint sets of charsets) in a single module.

Claes has described the issue well. Like I said, break short-circuits the search. If a module can provide more than one console provider, the suggested stream-less replacement is not equivalent.

If a module can provide more than one console provider, not only the code needs to be fixed, but a relevant test should be added too. The test should be in this PR, but tagged with the initial bug, 8295803, not this (performance) bug.

In fact, this started simply for incorporating JLine implementation into Console, and using ServiceLoader was a mere means to load its impl as it resides outside the java.base module. So yes, module and its console implementation is 1:1, which is reflected by the system property jdk.console that takes the module name. So that break; effectively shortcut the unnecessary looping (I don't think it would happen btw).
That said, I think it needs to be described in the comment above that piece of code. I will add it shortly.

So, it's the previous (stream) version that was more permissive and inadvertently so, yes?

Yes, correct. If hypothetically Jline provided two impls, it were not specified which impl was used. Now we only use the first one found.

jaikiran

The change to replace lambda with anonymous class looks OK to me.

AlanBateman · 2024-05-30T10:02:25Z

Would it be possible to provide more context/background here? This is not code that is used during startup. Is there benchmark data to share for first use of java.io.IO ?

cl4es · 2024-05-30T10:07:55Z

Would it be possible to provide more context/background here? This is not code that is used during startup. Is there benchmark data to share for first use of java.io.IO ?

I think this was brought to the fore by https://openjdk.org/jeps/477 where running the example implicit main:

void main() {
    println("Hello, World!");
}

.. brings in this code. This PR is one of several to try and smoothen a few rough edges that makes the startup of that quite a bit heavier than the baseline non-implicit Hello World.

lahodaj · 2024-05-30T10:29:57Z

Just to clarify a bit, this is only vaguely related to the implicitly declared classes. The actual issue is that having two programs:

public class SystemPrint {
    public static void main(String... args) {
        System.err.println("Hello!");
    }
}

and:

public class IOPrint {
    public static void main(String... args) {
        java.io.IO.println("Hello!");
    }
}

(note there are no implicitly declared classes, and no implicit imports in the samples), there is a considerable difference in the runtime of (compiled versions) of these classes. E.g. on my laptop:

$ time java -classpath /tmp SystemPrint 
Hello!

real    0m0,035s
user    0m0,019s
sys     0m0,019s

$ time java -classpath /tmp --enable-preview IOPrint 
Hello!

real    0m0,165s
user    0m0,324s
sys     0m0,042s

(Vast) majority of the time is spent in JLine initialization. There's https://bugs.openjdk.org/browse/JDK-8333086 for that, where the intent is to avoid initializing JLine for simple printing. There may be other opportunities to make JLine initialization faster in case we need to initialize it.

naotoj · 2024-06-03T15:50:55Z

/integrate
Thank you for your reviews!

openjdk · 2024-06-03T15:52:22Z

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

4de6207: 8333229: Parallel: Rename ParMarkBitMap::_region_start to _heap_start
1f9e629: 8333434: IGV: Print loop node for PHASE_BEFORE/AFTER_CLOOPS
27af19d: 8332586: Avoid cloning empty arrays in java.lang.reflect.{Method,Constructor}
1c514b3: 8325435: [macos] Menu or JPopupMenu not closed when main window is resized
d07e530: 8333128: Linux x86_32 configure fail with --with-hsdis=binutils --with-binutils-src
f0bffbc: 8333301: Remove static builds using --enable-static-build
b101586: 8332514: Allow class space size to be larger than 3GB
5ed0d52: 8332936: Test vmTestbase/metaspace/gc/watermark_70_80/TestDescription.java fails with no GC's recorded
91101f0: 8333353: Delete extra empty line in CodeBlob.java
e0ac824: 8332959: C2: ZGC fails with 'Incorrect load shift' when invoking Object.clone() reflectively on an array
... and 83 more: https://git.openjdk.org/jdk/compare/b8f2ec9091f9f7e5f4611991d04dd8aa113b94fd...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-06-03T15:52:28Z

@naotoj Pushed as commit 9686e80.

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

initial commit

ae06bb2

openjdk bot added the core-libs core-libs-dev@openjdk.org label May 29, 2024

naotoj marked this pull request as ready for review May 29, 2024 20:30

openjdk bot added the rfr Pull request is ready for review label May 29, 2024

cl4es approved these changes May 29, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label May 29, 2024

liach reviewed May 29, 2024

View reviewed changes

jaikiran approved these changes May 30, 2024

View reviewed changes

lahodaj mentioned this pull request May 30, 2024

8333086: Using Console.println is unnecessarily slow due to JLine initalization #19479

Closed

3 tasks

Clarify the comment for multiple impls in the module case

42a4532

openjdk bot added the integrated Pull request has been integrated label Jun 3, 2024

openjdk bot closed this Jun 3, 2024

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jun 3, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8333103: Re-examine the console provider loading #19467

8333103: Re-examine the console provider loading #19467

naotoj commented May 29, 2024 •

edited by openjdk bot

Loading

bridgekeeper bot commented May 29, 2024

openjdk bot commented May 29, 2024 •

edited

Loading

openjdk bot commented May 29, 2024

mlbridge bot commented May 29, 2024 •

edited

Loading

liach May 29, 2024

pavelrappo May 29, 2024

naotoj May 29, 2024

cl4es May 30, 2024

pavelrappo May 30, 2024

naotoj May 30, 2024

pavelrappo May 30, 2024

naotoj May 30, 2024

jaikiran left a comment

AlanBateman commented May 30, 2024

cl4es commented May 30, 2024 •

edited

Loading

lahodaj commented May 30, 2024

naotoj commented Jun 3, 2024

openjdk bot commented Jun 3, 2024

openjdk bot commented Jun 3, 2024

8333103: Re-examine the console provider loading #19467

8333103: Re-examine the console provider loading #19467

Conversation

naotoj commented May 29, 2024 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented May 29, 2024

openjdk bot commented May 29, 2024 • edited Loading

openjdk bot commented May 29, 2024

mlbridge bot commented May 29, 2024 • edited Loading

Webrevs

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

jaikiran left a comment

Choose a reason for hiding this comment

AlanBateman commented May 30, 2024

cl4es commented May 30, 2024 • edited Loading

lahodaj commented May 30, 2024

naotoj commented Jun 3, 2024

openjdk bot commented Jun 3, 2024

openjdk bot commented Jun 3, 2024

naotoj commented May 29, 2024 •

edited by openjdk bot

Loading

openjdk bot commented May 29, 2024 •

edited

Loading

mlbridge bot commented May 29, 2024 •

edited

Loading

cl4es commented May 30, 2024 •

edited

Loading