8341566: Add Reader.of(CharSequence) by mkarg · Pull Request #21371 · openjdk/jdk

mkarg · 2024-10-05T16:32:39Z

This Pull Requests proposes an implementation for JDK-8341566: Adding the new method public static Reader Reader.of(CharSequence) will return an anonymous, non-synchronized implementation of a Reader for each kind of CharSequence implementation. It is optimized for String, StringBuilder, StringBuffer and CharBuffer.

In addition, this Pull Request proposes to replace the implementation of StringReader to become a simple synchronized wrapper around Reader.of(CharSequence) for the case of String sources. To ensure correctness, this PR...

...simply moved the original code of StringBuilder to become the de-facto implementation of Reader.of(), then stripped synchronized from it on the left hand, but kept just a synchronized wrapper on the right hand. Then added a switch for optimizations within the original code, at the exact location where previously just an optimization for String lived in.
...added tests for all methods (Of.java), and applied that test upon the modified StringBuilder.

Wherever new JavaDocs were added, existing phrases from other code locations have been copied and adapted, to best match the same wording.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change requires CSR request JDK-8341596 to be approved
Change must be properly reviewed (2 reviews required, with at least 2 Reviewers)

Issues

JDK-8341566: Add Reader.of(CharSequence) (Enhancement - P4)
JDK-8341596: Add Reader.of(CharSequence) (CSR)

Reviewers

Roger Riggs (@RogerRiggs - Reviewer)
Jaikiran Pai (@jaikiran - Reviewer)
Chen Liang (@liach - Reviewer)
Alan Bateman (@AlanBateman - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 21371

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

Using diff file

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

Webrev

Link to Webrev Comment

bridgekeeper · 2024-10-05T16:33:19Z

👋 Welcome back mkarg! 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-10-05T16:34:04Z

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

8341566: Add Reader.of(CharSequence)

Reviewed-by: rriggs, jpai, liach, alanb

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

afb62f7: 8342683: Use non-short forward jump when passing stop()
964d8d2: 8340445: [PPC64] Wrong ConditionRegister used in ppc64.ad: flagsRegCR0 cr1
7131f05: 8342043: Split Opaque4Node into OpaqueTemplateAssertionPredicateNode and OpaqueNotNullNode
37cfaa8: 8338449: ubsan: division by zero in sharedRuntimeTrans.cpp
a1ef818: 8342825: Fix order of @param tags in module java.desktop
cdad728: 8342646: JTREG_TEST_THREAD_FACTORY in testing.md should be TEST_THREAD_FACTORY
018db8c: 8342809: C2 hits "assert(is_If()) failed: invalid node class: Con" during IGVN due to unhandled top
f1f1537: 8341453: java/awt/a11y/AccessibleJTableTest.java fails in some cases where the test tables are not visible
476d0f1: 8339309: unused-variable warnings happen in libfontmanager
d6eddcd: 8327624: Remove VM implementation that bypass verification for core reflection
... and 243 more: https://git.openjdk.org/jdk/compare/f8db3a831b61bb585c5494a7a8657e37000892b4...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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@RogerRiggs, @jaikiran, @liach, @AlanBateman) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

openjdk · 2024-10-05T16:34:55Z

@mkarg 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-10-05T16:37:49Z

Webrevs

test/jdk/java/io/Reader/Of.java

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

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

AlanBateman · 2024-10-06T05:55:20Z

/csr

openjdk · 2024-10-06T06:36:50Z

@AlanBateman has indicated that a compatibility and specification (CSR) request is needed for this pull request.

@mkarg please create a CSR request for issue JDK-8341566 with the correct fix version. This pull request cannot be integrated until the CSR request is approved.

AlanBateman · 2024-10-06T13:14:32Z

Reader.of(CharSequence) looks much better than introducing CharSequenceReader. It won't have an equivalent on Writer but I think that is okay. Also it means that the user will need to deal with close throwing IOException but anything using Reader has to do this already.

I think it would be better to drop "API compatible with StringReader" from the method description. An apiNote in StringReader can direct readers to the static factory method.

Also I think drop the "lock" field from the API docs as it's a protected field and only interesting to subclasses. The Reader class does not specify if Reader is thread-safe so the method description doesn't need to say too much. For clarify then it could just say something like "the resulting Reader is not safe for use by multiple concurrent threads. If the Reader is to be used by more than one thread it should be controlled by appropriate synchronization".

The parameter name is currently "c", maybe you mean "cs"? The method will need to specify NPE for the of(null) case.

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

@APinote

Updated JavaDocs according to Alan Bateman's review comments: * Dropped "API compatible with StringReader" from description * @APinote in StringReader refers to static factory method * Dropped "lock" field from API docs * Added "The resulting Reader is not safe for use by multiple concurrent threads. If the Reader is to be used by more than one thread it should be controlled by appropriate synchronization" * Deviating from your proposal, I renamed parameter "c" to "source" for clarity as the name "cs" already exists as an internal variable * Method specifies NPE for `of(null)` case I addition, JavaDocs now says "reader", not "stream" anymore.

mkarg · 2024-10-06T15:23:27Z

Thank you, Alan! I have updated the JavaDocs according to your comments:

Dropped "API compatible with StringReader" from description
@APinote in StringReader refers to static factory method
Dropped "lock" field from API docs
Added "The resulting Reader is not safe for use by multiple concurrent threads. If the Reader is to be used by more than one thread it should be controlled by appropriate synchronization"
Deviating from your proposal, I renamed parameter "c" to "source" for clarity as the name "cs" already exists as an internal variable
Method specifies NPE for of(null) case

I addition, JavaDocs now says "reader", not "stream" anymore.

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

test/jdk/java/io/Reader/Of.java

Dropping non-public JavaDocs

mkarg · 2024-10-06T17:44:44Z

/csr

@AlanBateman Can you please review the CSR request so I can finish it? Thanks!

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

test/jdk/java/io/Reader/Of.java

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

test/jdk/java/io/Reader/Of.java

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

…rts the mark and reset ... could move up to become the second paragraph.'

mkarg · 2024-10-23T09:02:05Z

Thank you, Alan. I now have adjusted the JavaDocs to your exact proposals.

Kindly requesting final reviews from everybody, so we can finally move this PR through the door. Thank you! :-)

mkarg · 2024-10-23T09:08:12Z

@liach Alan's latest minor edit request auto-removed your approval from this PR. Kindly asking to reapply your approval. Thanks.

liach · 2024-10-23T11:41:33Z

The javadoc text changes look fine; since this does not constitute specification changes, we don't need to re-draft our CSR to reflect the text changes.

liach

The diff from Jaikiran and Roger's latest review is: 487b938...97e3144

There's no implementation change, so assuming the previous reviewers' approvals hold.

mkarg · 2024-10-23T12:18:03Z

I do share Chen's assumption, therefore requesting integration now.

/integrate

openjdk · 2024-10-23T12:18:39Z

@mkarg
Your change (at version 97e3144) is now ready to be sponsored by a Committer.

liach · 2024-10-23T12:26:11Z

Please allow us to run some internal testing to ensure there won't be intermittent test failures before anyone sponsors this patch.

jaikiran

This looks good to me.

kevinb9n · 2024-10-23T22:15:34Z

An issue with the simple name of is that we may encounter calls like this:

Reader.of("/usr/share/dict/words")

... that may look like they do something different. I can't say whether we should consider this a serious issue or not, though. I don't know if it justifies a big ugly name like ofCharSequence or not.

liach · 2024-10-23T23:44:27Z

I think this scenario has a close analogy with java.util.Scanner constructors. Does such a fault happen to Scanner often?

mkarg · 2024-10-24T14:00:23Z

An issue with the simple name of is that we may encounter calls like this:

Reader.of("/usr/share/dict/words")

... that may look like they do something different. I can't say whether we should consider this a serious issue or not, though. I don't know if it justifies a big ugly name like ofCharSequence or not.

There always will be a small group this misunderstands things and does not read JavaDocs, we can never find "the perfect API" to prevent this completely. The question is if we need to make the API worse in turn and bother all others with ugly lengthy names? I personally think the actual risk here is negligible, and the solution ("RTFM") is rather simple to implement. ;-)

To sum up, IMHO as soon as Chen finished the internal tests, we should close this discussion and merge this PR as-is.

liach · 2024-10-24T14:33:53Z

Thanks Markus! The java.io tests succeed with repeats and the only test failures in tier 1-3 were ones problem-listed by f7a61fc.

FYI, besides Kevin's question, there was another concern about this API's interaction with BufferedReader (as you know, many Reader users wrap any Reader into a BufferedReader, especially you want to read all lines from a reader). We currently decide that we might explore line reading capability to Reader, but anyways that will not affect this patch.

And I have consulted with Kevin in private; he thinks this minor confusion is fine.

Sponsoring.

/sponsor

openjdk · 2024-10-24T14:35:08Z

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

b0ac633: 8342075: HttpClient: improve HTTP/2 flow control checks
85774b7: 8342882: RISC-V: Unify handling of jumps to runtime
2c31c8e: 8339730: Windows regression after removing ObjectMonitor Responsible
f0b130e: 8339296: Record deconstruction pattern in switch fails to compile
e96b4cf: 8342387: C2 SuperWord: refactor and improve compiler/loopopts/superword/TestDependencyOffsets.java
f7a61fc: 8342931: ProblemList failing tests from JDK-8335912
25c2f48: 8338544: Dedicated Array class descriptor implementation
158b93d: 8335912: Add an operation mode to the jar command when extracting to not overwriting existing files
28d23ad: 8340177: Malformed system classes loaded by bootloader crash the JVM in product builds
98403b7: 8342854: [JVMCI] Block secondary thread reporting a JVMCI fatal error
... and 260 more: https://git.openjdk.org/jdk/compare/f8db3a831b61bb585c5494a7a8657e37000892b4...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-10-24T14:35:19Z

@liach @mkarg Pushed as commit 3c14c2b.

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

mkarg · 2024-11-23T15:08:38Z

I have drafted a release note for JDK-8341566 in JDK-8344910. Kindly asking for reviews! :-)

AlanBateman · 2024-11-24T14:47:11Z

I have drafted a release note for JDK-8341566 in JDK-8344910. Kindly asking for reviews! :-)

Thanks for creating this. I've edited it so that it is now focused on highlighting the new API and nothing else. Chen had some good suggestions too.

mlbridge · 2024-11-24T16:14:08Z

Mailing list message from Markus Karg on core-libs-dev:

Thank you so much for your kind reviews, Alan and Chen! :-)

I have slighly extended Alan's draft to let people better understand the differences, hence to be able to actively chose between StringReader and Reader.of(CharSequence):

"The static factory method `java.io.Reader.of(CharSequence)` has been added in get a `Reader` that reads characters from a `CharSequence` (`String` and `StringBuilder` are examples of a `CharSequence`). The returned `Reader` is more efficient than using a `java.io.StringReader` in some cases, as the latter requires conversion to `String` and synchronization."

Is that OK? I see the risk that people cannot make a well-founded decision without.

I am not a native English speaker. Do we need to add "TO" in "...added in TO get a Reader..."?

-Markus

-----Urspr?ngliche Nachricht-----
Von: core-libs-dev [mailto:core-libs-dev-retn at openjdk.org] Im Auftrag von Alan Bateman
Gesendet: Sonntag, 24. November 2024 15:50
An: core-libs-dev at openjdk.org
Betreff: Re: RFR: 8341566: Add Reader.of(CharSequence) [v15]

On Sat, 23 Nov 2024 15:08:38 GMT, Markus KARG <duke at openjdk.org> wrote:

I have drafted a release note for [JDK-8341566](https://bugs.openjdk.org/browse/JDK-8341566) in [JDK-8344910](https://bugs.openjdk.org/browse/JDK-8344910). Kindly asking for reviews! :-)

Thanks for creating this. I've edited it so that it is now focused on highlighting the new API and nothing else. Chen had some good suggestions too.

-------------

PR Comment: https://git.openjdk.org/jdk/pull/21371#issuecomment-2496042639

mlbridge · 2024-11-24T18:36:08Z

Mailing list message from Roger Riggs on core-libs-dev:

Hi Markus,

Thanks for drafting the release note and improvments.

Slight update "add in get a" -> "added to get a".

Regards, Roger

p.s.? An Oracle Tech writer also reviews and may do minor cleanup to
each release note to give a consistent use of terms and format.

On 11/24/24 11:12 AM, Markus Karg wrote:

mkarg added 2 commits October 5, 2024 16:13

Reader.of(String)

03b59a1

StringReader delegates to Reader.of(String)

dc7dc81

openjdk bot added the rfr Pull request is ready for review label Oct 5, 2024

openjdk bot added the core-libs core-libs-dev@openjdk.org label Oct 5, 2024

mkarg commented Oct 5, 2024

View reviewed changes

test/jdk/java/io/Reader/Of.java Show resolved Hide resolved

mkarg commented Oct 5, 2024

View reviewed changes

src/java.base/share/classes/java/io/StringReader.java Show resolved Hide resolved

mkarg commented Oct 5, 2024

View reviewed changes

src/java.base/share/classes/java/io/Reader.java Show resolved Hide resolved

openjdk bot added the csr Pull request needs approved CSR before integration label Oct 6, 2024

liach reviewed Oct 6, 2024

View reviewed changes

fixup! Reader.of(String)

ae9f584

Dropping non-public JavaDocs

ecki reviewed Oct 6, 2024

View reviewed changes

src/java.base/share/classes/java/io/Reader.java Show resolved Hide resolved

ecki reviewed Oct 6, 2024

View reviewed changes

test/jdk/java/io/Reader/Of.java Outdated Show resolved Hide resolved

AlanBateman reviewed Oct 6, 2024

View reviewed changes

src/java.base/share/classes/java/io/Reader.java Outdated Show resolved Hide resolved

src/java.base/share/classes/java/io/Reader.java Outdated Show resolved Hide resolved

AlanBateman reviewed Oct 6, 2024

View reviewed changes

src/java.base/share/classes/java/io/Reader.java Outdated Show resolved Hide resolved

ecki reviewed Oct 6, 2024

View reviewed changes

test/jdk/java/io/Reader/Of.java Outdated Show resolved Hide resolved

bokken reviewed Oct 6, 2024

View reviewed changes

src/java.base/share/classes/java/io/Reader.java Show resolved Hide resolved

ExE-Boss reviewed Oct 7, 2024

View reviewed changes

src/java.base/share/classes/java/io/Reader.java Show resolved Hide resolved

mkarg added 2 commits October 7, 2024 17:44

Remove useless test

f089436

test for generic read(char, int, int) case

f3c72cf

Fixed wording as proposed by Alan Bateman: 'The returned reader suppo…

97e3144

…rts the mark and reset ... could move up to become the second paragraph.'

AlanBateman approved these changes Oct 23, 2024

View reviewed changes

liach approved these changes Oct 23, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Oct 23, 2024

openjdk bot added the sponsor Pull request is ready to be sponsored label Oct 23, 2024

jaikiran approved these changes Oct 23, 2024

View reviewed changes

RogerRiggs approved these changes Oct 23, 2024

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Oct 24, 2024

openjdk bot closed this Oct 24, 2024

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Oct 24, 2024

mkarg deleted the charsequencereader branch October 24, 2024 15:44

mkarg mentioned this pull request Mar 23, 2025

8343110: Add getChars(int, int, char[], int) to CharSequence and CharBuffer #21730

Closed

4 tasks

Conversation

mkarg commented Oct 5, 2024 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issues

Reviewers

Reviewing

Webrev

Uh oh!

bridgekeeper bot commented Oct 5, 2024

Uh oh!

openjdk bot commented Oct 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Oct 5, 2024

Uh oh!

mlbridge bot commented Oct 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AlanBateman commented Oct 6, 2024

Uh oh!

openjdk bot commented Oct 6, 2024

Uh oh!

AlanBateman commented Oct 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mkarg commented Oct 6, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mkarg commented Oct 6, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mkarg commented Oct 23, 2024

Uh oh!

mkarg commented Oct 23, 2024

Uh oh!

liach commented Oct 23, 2024

Uh oh!

liach left a comment

Choose a reason for hiding this comment

Uh oh!

mkarg commented Oct 23, 2024

Uh oh!

openjdk bot commented Oct 23, 2024

Uh oh!

liach commented Oct 23, 2024

Uh oh!

jaikiran left a comment

Choose a reason for hiding this comment

Uh oh!

kevinb9n commented Oct 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

liach commented Oct 23, 2024

Uh oh!

mkarg commented Oct 24, 2024

Uh oh!

liach commented Oct 24, 2024

Uh oh!

openjdk bot commented Oct 24, 2024

Uh oh!

mkarg commented Oct 5, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Oct 5, 2024 •

edited

Loading

mlbridge bot commented Oct 5, 2024 •

edited

Loading

AlanBateman commented Oct 6, 2024 •

edited

Loading

kevinb9n commented Oct 23, 2024 •

edited

Loading